From: arno@natisbad•org (Arnaud Ebalard)
To: Herbert Xu <herbert@gondor•apana.org.au>
Cc: David Miller <davem@davemloft•net>,
eric.dumazet@gmail•com, yoshfuji@linux-ipv6•org,
netdev@vger•kernel.org
Subject: Re: [PATCHv3 net-next-2.6 3/5] XFRM,IPv6: Add IRO src/dst address remapping XFRM types and i/o handlers
Date: Sun, 03 Oct 2010 23:25:44 +0200 [thread overview]
Message-ID: <87lj6fvuhz.fsf@small.ssi.corp> (raw)
In-Reply-To: <20101003151202.GA11963@gondor.apana.org.au> (Herbert Xu's message of "Sun, 3 Oct 2010 23:12:02 +0800")
Hello,
Herbert Xu <herbert@gondor•apana.org.au> writes:
> On Sun, Oct 03, 2010 at 03:41:04PM +0200, Arnaud Ebalard wrote:
>>
>> After your reply, I took a (too long) look at the history of
>> xfrm6_input_addr() to understand why it is as it is. If it can spare you
>> some time, here is what I think happened:
>
> ...
>
>> - Then, as you wrote, the state lock was moved in all input handlers
>> (commit 0ebea8ef, Nov 13 2007), including RH2/HAO ones:
>
> ...
>
>> With that commit, I think a deadlock was introduced in MIPv6 code
>> because xfm6_input_addr() was left unchanged, i.e. x->type->input()
>> was called with the lock held. Am I correct?
>>
>> - The code of xfrm6_input_addr() was then optimized by commit a002c6fd
>> in such a way that x->type->input() was then put outside the
>> protection of the lock, which (if I am not mistaken) removed the
>> deadlock:
>
> ...
>
>> I don't know if this is was intentional.
>
> Indeed MIPv6 was completely out of action for three months and
> nobody noticed :)
hehe ;-) Just to correct a missing waypoint in my history, which is in
fact the real fix for the deadlock:
commit 9473e1f631de339c50bde1e3bd09e1045fe90fd5
Author: Masahide NAKAMURA <nakam@linux-ipv6•org>
Date: Thu Dec 20 20:41:57 2007 -0800
[XFRM] MIPv6: Fix to input RO state correctly.
Disable spin_lock during xfrm_type.input() function.
Follow design as IPsec inbound does.
Signed-off-by: Masahide NAKAMURA <nakam@linux-ipv6•org>
Signed-off-by: David S. Miller <davem@davemloft•net>
>> But the main question remains on the position of the lock. Here,
>> checks are done on the status of the state, lock is released,
>> reacquired in the input handler to do additional check and then
>> released again, to be reacquired later in the function to act on
>> statistics. Is my reading of the code correct?
>
> When I moved the lock down I chose the safest option and added
> it to every single input function. So it may well be the case
> that the lock isn't needed at all on the MIPv6 path.
I don't have any technical argument to support the removal of the locks,
i.e. don't see what would prevent changes during the check. I will try
and spend more time on it, but meanwhile I think it's safe to keep
things the way they are.
Thanks for your time, Herbert.
Cheers,
a+
next prev parent reply other threads:[~2010-10-03 21:25 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-29 9:05 [PATCHv3 net-next-2.6 0/5] XFRM,IPv6: Removal of RH2/HAO from IPsec-protected MIPv6 traffic Arnaud Ebalard
2010-09-29 9:05 ` [PATCHv3 net-next-2.6 1/5] XFRM,IPv6: Remove xfrm_spi_hash() dependency on destination address Arnaud Ebalard
2010-09-29 9:05 ` [PATCHv3 net-next-2.6 2/5] XFRM,IPv6: Introduce receive sockopts to access IRO remapped src/dst addresses Arnaud Ebalard
2010-09-29 9:05 ` [PATCHv3 net-next-2.6 3/5] XFRM,IPv6: Add IRO src/dst address remapping XFRM types and i/o handlers Arnaud Ebalard
2010-09-30 3:16 ` David Miller
2010-10-02 10:17 ` Arnaud Ebalard
2010-10-02 10:32 ` Herbert Xu
2010-10-03 13:41 ` Arnaud Ebalard
2010-10-03 15:12 ` Herbert Xu
2010-10-03 21:25 ` Arnaud Ebalard [this message]
2010-09-29 9:05 ` [PATCHv3 net-next-2.6 4/5] XFRM,IPv6: Add IRO remapping hook in xfrm_input() Arnaud Ebalard
2010-09-30 3:17 ` David Miller
2010-09-29 9:06 ` [PATCHv3 net-next-2.6 5/5] XFRM,IPv6: Add IRO remapping capability via socket ancillary data path Arnaud Ebalard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lj6fvuhz.fsf@small.ssi.corp \
--to=arno@natisbad$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=netdev@vger$(echo .)kernel.org \
--cc=yoshfuji@linux-ipv6$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox