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

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

[Nicolas Dichtel]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]

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.


Regards,

-- 
Nicolas DICHTEL
6WIND
R&D Engineer

Tel: +33 1 39 30 92 10
Fax: +33 1 39 30 92 11
nicolas.dichtel@6wind.com
www.6wind.com
Join the Multicore Packet Processing Forum: www.multicorepacketprocessing.com

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou 
ses destinataires. Il contient des informations confidentielles qui sont la 
propriété de 6WIND. Toute révélation, distribution ou copie des informations 
qu'il contient est strictement interdite. Si vous avez reçu ce message par 
erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les 
données reçues.

This e-mail message, including any attachments, is for the sole use of the 
intended recipient(s) and contains information that is confidential and 
proprietary to 6WIND. All unauthorized review, use, disclosure or distribution 
is prohibited. If you are not the intended recipient, please contact the sender 
by reply e-mail and destroy all copies of the original message.

[-- Attachment #2: 0001-dst-check-if-dst-is-freed-in-dst_check.patch --]
[-- Type: text/x-diff, Size: 772 bytes --]

>From 69990a516f4b5b48608b0ea283dfac6f1fa110b3 Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 20 Jul 2010 11:35:53 +0200
Subject: [PATCH] dst: check if dst is freed in dst_check()

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/dst.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

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;
-- 
1.5.4.5

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

[Eric Dumazet]

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;
}

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Jul 2010 04:28:08 +0200

> Le mardi 20 juillet 2010 à 11:49 +0200, Nicolas Dichtel a écrit :
>> 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.

Right, last time I was snooping around in here I came to the
same conclusion.  In fact I think I'm the author of that
enormous comment in xfrm_dst_check(). :-)

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

[Nicolas Dichtel]

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;
> }