| Message ID | 4BBDBCF9.5060906@iki.fi |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 *);