Re: Crashes in xfrm_lookup

["Timo Teräs" <timo.teras@iki.fi>]

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.