| Message ID | 4C457120.9070105@6wind.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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; } -- 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: 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(). :-) -- 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
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; > } -- 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 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