Skip to contentSkip to conversation

dst: check if dst is freed in dst_check()

RFC PATCH In review Nicolas Dichtel 3 participants 4 messages Jul 20, 2010 – Jul 21, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Nicolas Dichtel

author
#0 lore raw

RFC PATCH 1/1 dst: check if dst is freed in dst_check()

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,

Signature · 13 lines
Nicolas DICHTEL
6WIND
R&D Engineer

Tel: +33 1 39 30 92 10
Fax: +33 1 39 30 92 11
[email protected]
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.
Quoted 1 line

From 69990a516f4b5b48608b0ea283dfac6f1fa110b3 Mon Sep 17 00:00:00 2001

From: Nicolas Dichtel <[email protected]>
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 <[email protected]>
---
 include/net/dst.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
include/net/dst.h +2 -0
index 81d1413..7bf4f9a 100644
@@ -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;
Signature · 1 line
1.5.4.5
#1 lore raw
Quoted 14 lines · Nicolas Dichtel

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.

include/net/dst.h +0 -0
index 81d1413..7bf4f9a 100644

include/net/dst.h static inline int dst_input(struct sk_buff *skb) · line 326

Quoted 8 lines
include/net/dst.h +2 -0
@@ -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;
}

maintainer
#2 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 21 Jul 2010 04:28:08 +0200

Quoted 17 lines · Nicolas Dichtel

include/net/dst.h static inline int dst_input(struct sk_buff *skb) · line 326

Quoted 13 lines
include/net/dst.h +2 -0
index 81d1413..7bf4f9a 100644
@@ -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.

Quoted 5 lines

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(). :-)

author
#3 lore raw
In reply to Eric Dumazet

Hi Eric,

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

Regards,
Nicolas

Quoted 58 lines · Eric Dumazet
Quoted 13 lines · Nicolas Dichtel

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.

include/net/dst.h +0 -0
index 81d1413..7bf4f9a 100644

include/net/dst.h static inline int dst_input(struct sk_buff *skb) · line 326

Quoted 8 lines
include/net/dst.h +2 -0
@@ -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;
}