Re: [PATCHv3 net-next-2.6 3/5] XFRM,IPv6: Add IRO src/dst address remapping XFRM types and i/o handlers

[arno@natisbad.org (Arnaud Ebalard)]

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+