Re: [RFC PATCH] dst: check if dst is freed in dst_check()

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

Hi Eric,

I was thinking to use this function in sctp, but I misread xfrm part.
Sorry for the noise.


Regards,
Nicolas

Le 21.07.2010 04:28, Eric Dumazet a écrit :
> Le mardi 20 juillet 2010 à 11:49 +0200, Nicolas Dichtel a écrit :
>> Hi,
>>
>> I probably missed something, but I cannot find where obsolete field is checked 
>> when dst_check() is called. If dst->obsolete is > 1, dst cannot be used!
>>
>> Attached is a proposal to fix this issue.
>>
>>
> 
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 81d1413..7bf4f9a 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -319,6 +319,8 @@ static inline int dst_input(struct sk_buff *skb)
>>  
>>  static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>>  {
>> +	if (dst->obsolete > 1)
>> +		return NULL;
>>  	if (dst->obsolete)
>>  		dst = dst->ops->check(dst, cookie);
>>  	return dst;
> 
> I believe this is not needed and redundant.
> 
> In what case do you think this matters ?
> 
> To my knowledge dst_check() is only used by net/xfrm/xfrm_policy.c
> 
> And xfrm_dst_check() does the necessary checks.
> 
> static struct dst_entry *xfrm_dst_check(struct dst_entry *dst, u32 cookie)
> {
>         /* Code (such as __xfrm4_bundle_create()) sets dst->obsolete
>          * to "-1" to force all XFRM destinations to get validated by
>          * dst_ops->check on every use.  We do this because when a
>          * normal route referenced by an XFRM dst is obsoleted we do
>          * not go looking around for all parent referencing XFRM dsts
>          * so that we can invalidate them.  It is just too much work.
>          * Instead we make the checks here on every use.  For example:
>          *
>          *      XFRM dst A --> IPv4 dst X
>          *
>          * X is the "xdst->route" of A (X is also the "dst->path" of A
>          * in this example).  If X is marked obsolete, "A" will not
>          * notice.  That's what we are validating here via the
>          * stale_bundle() check.
>          *
>          * When a policy's bundle is pruned, we dst_free() the XFRM
>          * dst which causes it's ->obsolete field to be set to a
>          * positive non-zero integer.  If an XFRM dst has been pruned
>          * like this, we want to force a new route lookup.
>          */
>         if (dst->obsolete < 0 && !stale_bundle(dst))
>                 return dst;
> 
>         return NULL;
> }