Crashes in xfrm_lookup

Crashes in xfrm_lookup

[Mark Brown]

With -next as of today I'm experiencing crashes in the xfrm_lookup code
when attempting to boot from an NFS root on a SMDK6410 (an ARM based
development board).  I'm currently investigating what's gone wrong, but
thought it was better to report early in case it's obvious to someone
familiar with the code or there's a fix already.

Sample backtrace below:

[    6.540000] Unable to handle kernel NULL pointer dereference at virtual addrc
[    6.540000] pgd = c0004000                                                   
[    6.550000] [0000003c] *pgd=00000000                                         
[    6.550000] Internal error: Oops: 5 [#1]                                     
[    6.550000] last sysfs file:                                                 
[    6.550000] Modules linked in:                                               
[    6.550000] CPU: 0    Not tainted (2.6.34-rc3-next-20100408-00011-g3d8ee7a-)
[    6.550000] PC is at __xfrm_lookup+0x308/0x3a8                               
[    6.550000] LR is at ip_route_output_flow+0x7c/0x210                         

...

[    6.550000] [<c02c1394>] (__xfrm_lookup+0x308/0x3a8) from [<c02889a4>] (ip_r)
[    6.550000] [<c02889a4>] (ip_route_output_flow+0x7c/0x210) from [<c02ade9c>])
[    6.550000] [<c02ade9c>] (udp_sendmsg+0x320/0x5ec) from [<c02b3b20>] (inet_s)
[    6.550000] [<c02b3b20>] (inet_sendmsg+0x58/0x64) from [<c02612d8>] (sock_se)
[    6.550000] [<c02612d8>] (sock_sendmsg+0x88/0xa8) from [<c0261338>] (kernel_)
[    6.550000] [<c0261338>] (kernel_sendmsg+0x40/0x7c) from [<c02d0dd0>] (xs_se)
[    6.550000] [<c02d0dd0>] (xs_send_kvec+0x94/0xa4) from [<c02d0e70>] (xs_send)
[    6.550000] [<c02d0e70>] (xs_sendpages+0x90/0x204) from [<c02d1268>] (xs_udp)
[    6.550000] [<c02d1268>] (xs_udp_send_request+0x3c/0x120) from [<c02cf254>] )
[    6.550000] [<c02cf254>] (xprt_transmit+0x158/0x25c) from [<c02cc9c0>] (call)
[    6.550000] [<c02cc9c0>] (call_transmit+0x204/0x284) from [<c02d380c>] (__rp)
[    6.550000] [<c02d380c>] (__rpc_execute+0x94/0x2c4) from [<c02cd35c>] (rpc_r)
[    6.550000] [<c02cd35c>] (rpc_run_task+0x60/0x6c) from [<c02cd4a0>] (rpc_cal)
[    6.550000] [<c02cd4a0>] (rpc_call_sync+0x58/0x84) from [<c02cd51c>] (rpc_pi)
[    6.550000] [<c02cd51c>] (rpc_ping+0x50/0x70) from [<c02cded0>] (rpc_create+)
[    6.550000] [<c02cded0>] (rpc_create+0x3e4/0x498) from [<c013c194>] (nfs_mou)
[    6.550000] [<c013c194>] (nfs_mount+0xd8/0x1c4) from [<c0016d14>] (nfs_root_)
[    6.550000] [<c0016d14>] (nfs_root_data+0x2cc/0x398) from [<c0008fb8>] (moun)
[    6.550000] [<c0008fb8>] (mount_root+0x1c/0x104) from [<c0009204>] (prepare_)
[    6.550000] [<c0009204>] (prepare_namespace+0x164/0x1c8) from [<c0008478>] ()
[    6.550000] [<c0008478>] (kernel_init+0x108/0x148) from [<c002df5c>] (kernel)

Re: Crashes in xfrm_lookup

[Timo Teräs]

Mark Brown wrote:
> With -next as of today I'm experiencing crashes in the xfrm_lookup code
> when attempting to boot from an NFS root on a SMDK6410 (an ARM based
> development board).  I'm currently investigating what's gone wrong, but
> thought it was better to report early in case it's obvious to someone
> familiar with the code or there's a fix already.

Probably the same as http://marc.info/?t=127071006600005&r=1&w=2

Happens because CONFIG_XFRM_SUB_POLICY is not enabled, and one of
the helper functions I used did unexpected things in that case.

Try the following:

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 625dd61..cccb049 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -735,19 +735,12 @@ static inline void xfrm_pol_put(struct xfrm_policy *policy)
 		xfrm_policy_destroy(policy);
 }
 
-#ifdef CONFIG_XFRM_SUB_POLICY
 static inline void xfrm_pols_put(struct xfrm_policy **pols, int npols)
 {
 	int i;
 	for (i = npols - 1; i >= 0; --i)
 		xfrm_pol_put(pols[i]);
 }
-#else
-static inline void xfrm_pols_put(struct xfrm_policy **pols, int npols)
-{
-	xfrm_pol_put(pols[0]);
-}
-#endif
 
 extern void __xfrm_state_destroy(struct xfrm_state *);
 

Re: Crashes in xfrm_lookup

[Mark Brown]

On Thu, Apr 08, 2010 at 02:24:41PM +0300, Timo Teräs wrote:

> Probably the same as http://marc.info/?t=127071006600005&r=1&w=2

> Happens because CONFIG_XFRM_SUB_POLICY is not enabled, and one of
> the helper functions I used did unexpected things in that case.

> Try the following:

Yes, that seems to make things much happier - thanks!  I do have one
patch I came up with while I was looking at the code (though I'm not
sure it's correct), I'll post that once I've had lunch.

Re: Crashes in xfrm_lookup

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Thu, 08 Apr 2010 14:24:41 +0300

> Mark Brown wrote:
>> With -next as of today I'm experiencing crashes in the xfrm_lookup
>> code
>> when attempting to boot from an NFS root on a SMDK6410 (an ARM based
>> development board).  I'm currently investigating what's gone wrong,
>> but
>> thought it was better to report early in case it's obvious to someone
>> familiar with the code or there's a fix already.
> 
> Probably the same as http://marc.info/?t=127071006600005&r=1&w=2
> 
> Happens because CONFIG_XFRM_SUB_POLICY is not enabled, and one of
> the helper functions I used did unexpected things in that case.
> 
> Try the following:

Since this fix has been confirmed I'm applying it to net-next-2.6,
thanks!

Re: Crashes in xfrm_lookup

[Herbert Xu]

Timo Teräs <timo.teras@iki.fi> wrote:
>
> Happens because CONFIG_XFRM_SUB_POLICY is not enabled, and one of
> the helper functions I used did unexpected things in that case.
> 
> Try the following:

Ugh, can we fix this some other way?

The policy array should really only exist if SUB_POLICY is defined.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Crashes in xfrm_lookup

[Timo Teräs]

Herbert Xu wrote:
> Timo Teräs <timo.teras@iki.fi> wrote:
>> Happens because CONFIG_XFRM_SUB_POLICY is not enabled, and one of
>> the helper functions I used did unexpected things in that case.
>>
>> Try the following:
> 
> Ugh, can we fix this some other way?
> 
> The policy array should really only exist if SUB_POLICY is defined.

The problem was that my code could call it with zero polcies
assuming it'd do the right thing.

It's still really misleading to have generic function that does not
do the expected thing based on some config. Compiler should know
how to optimize the for loop away if it's being called with fixed
array size.

Re: Crashes in xfrm_lookup

[Herbert Xu]

On Fri, Apr 09, 2010 at 11:11:48AM +0300, Timo Teräs wrote:
>
> It's still really misleading to have generic function that does not
> do the expected thing based on some config. Compiler should know
> how to optimize the for loop away if it's being called with fixed
> array size.

But the compiler doesn't know because your patch makes it an
array unconditionally.

The SUB_POLICY stuff was a hack from the very start, but at least
you could compile it out previously.  Now it'll pollute things
regardless of the configuration.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Crashes in xfrm_lookup

[Timo Teräs]

Herbert Xu wrote:
> On Fri, Apr 09, 2010 at 11:11:48AM +0300, Timo Teräs wrote:
>> It's still really misleading to have generic function that does not
>> do the expected thing based on some config. Compiler should know
>> how to optimize the for loop away if it's being called with fixed
>> array size.
> 
> But the compiler doesn't know because your patch makes it an
> array unconditionally.
> 
> The SUB_POLICY stuff was a hack from the very start, but at least
> you could compile it out previously.  Now it'll pollute things
> regardless of the configuration.

It has been array all along. The only difference was that only
the first element was used if SUB_POLICY was not defined.

I still think xfrm_pols_put should do always what the function
name says it's doing.

If we want to further optimize non-SUB_POLICY stuff, we should
probably make XFRM_POLICY_TYPE_MAX = 1 and arrange rest of code
so that the compiler can optimize things properly.

But the fact is, that in the new code we need to do conditional
xfrm_policy_put depending on if we had per-socket or global policy
which we matched. Thus we either end up with "if (x)" or the
inline functions for loop's implicit test. Or do you have better
ideas how to avoid that?

Re: Crashes in xfrm_lookup

[Herbert Xu]

On Fri, Apr 09, 2010 at 11:30:49AM +0300, Timo Teräs wrote:
>
> It has been array all along. The only difference was that only
> the first element was used if SUB_POLICY was not defined.

It was an array but prior to your patch it only had a single
element when SUB_POLICY is not defined.  Your patch made it
contain XFRM_POLICY_TYPE_MAX elements unconditionally.

> I still think xfrm_pols_put should do always what the function
> name says it's doing.
>
> If we want to further optimize non-SUB_POLICY stuff, we should
> probably make XFRM_POLICY_TYPE_MAX = 1 and arrange rest of code
> so that the compiler can optimize things properly.

Anyway, the fact is prior to your patch SUB_POLICY had a minimal
impact on people who don't like it (like me), and now its effect
is being forced on everyone.

> But the fact is, that in the new code we need to do conditional
> xfrm_policy_put depending on if we had per-socket or global policy
> which we matched. Thus we either end up with "if (x)" or the
> inline functions for loop's implicit test. Or do you have better
> ideas how to avoid that?

Which particular piece of code are you referring to?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Crashes in xfrm_lookup

[Timo Teräs]

Herbert Xu wrote:
> On Fri, Apr 09, 2010 at 11:30:49AM +0300, Timo Teräs wrote:
>> It has been array all along. The only difference was that only
>> the first element was used if SUB_POLICY was not defined.
> 
> It was an array but prior to your patch it only had a single
> element when SUB_POLICY is not defined.  Your patch made it
> contain XFRM_POLICY_TYPE_MAX elements unconditionally.

No. Prior it had one element unconditionally. My patch made
it have zero or one element. The non-SUB_POLICY case crashed
because xfrm_pols_put(xxx, 0) unconditionally calls
xfrm_policy_put on unused pointer.

>> I still think xfrm_pols_put should do always what the function
>> name says it's doing.
>>
>> If we want to further optimize non-SUB_POLICY stuff, we should
>> probably make XFRM_POLICY_TYPE_MAX = 1 and arrange rest of code
>> so that the compiler can optimize things properly.
> 
> Anyway, the fact is prior to your patch SUB_POLICY had a minimal
> impact on people who don't like it (like me), and now its effect
> is being forced on everyone.

No. The effect is because the policies are now cached in bundles,
and lookup function should not anymore drop references to policies
which are kept in cache.

>> But the fact is, that in the new code we need to do conditional
>> xfrm_policy_put depending on if we had per-socket or global policy
>> which we matched. Thus we either end up with "if (x)" or the
>> inline functions for loop's implicit test. Or do you have better
>> ideas how to avoid that?
> 
> Which particular piece of code are you referring to?

__xfrm_lookup(). In the end it uses: "xfrm_pols_put(pols, drop_pols)"
to free up policies that are looked up with xfrm_sk_policy_lookup().
The only major code path there is, if per-socket policy has no
transformations (which is common case, ike daemons do this so they
can talk IKE without transformations).

If we have cached bundle, the policies are referenced to from the
bundle and we do not need to reference, or release them in the
lookup function.

It is a bit icky. But it's the only way to do it, since no one
wanted to cache per-socket bundles in the flow cache.

Re: Crashes in xfrm_lookup

[Herbert Xu]

On Fri, Apr 09, 2010 at 11:47:12AM +0300, Timo Teräs wrote:
>
> No. Prior it had one element unconditionally. My patch made
> it have zero or one element. The non-SUB_POLICY case crashed
> because xfrm_pols_put(xxx, 0) unconditionally calls
> xfrm_policy_put on unused pointer.

OK I was mistaken.

> __xfrm_lookup(). In the end it uses: "xfrm_pols_put(pols, drop_pols)"
> to free up policies that are looked up with xfrm_sk_policy_lookup().
> The only major code path there is, if per-socket policy has no
> transformations (which is common case, ike daemons do this so they
> can talk IKE without transformations).
>
> If we have cached bundle, the policies are referenced to from the
> bundle and we do not need to reference, or release them in the
> lookup function.
>
> It is a bit icky. But it's the only way to do it, since no one
> wanted to cache per-socket bundles in the flow cache.

BTW, socket policies cannot be sub-policies so you don't need
to expand policies for them.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt