| Message ID | 1351482620-11008-1-git-send-email-amwang@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-10-29 at 11:50 +0800, Cong Wang wrote: > In ipv6_del_addr(): > > if (rt != net->ipv6.ip6_null_entry && > addrconf_is_prefix_route(rt)) { > if (onlink == 0) { > ip6_del_rt(rt); > rt = NULL; > } else if (!(rt->rt6i_flags & RTF_EXPIRES)) { > rt6_set_expires(rt, expires); > } > } > dst_release(&rt->dst); > > obviously rt could be NULL'd before dst_release(), so > we have to check if rt is NULL before calling it. > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <amwang@redhat.com> > > --- > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 8f0b12a..c467dbb 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -951,7 +951,8 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) > rt6_set_expires(rt, expires); > } > } > - dst_release(&rt->dst); > + if (rt) > + dst_release(&rt->dst); > } > dst_release() is like kfree(), it accepts a NULL argument. -- 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
On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote: > > - dst_release(&rt->dst); > > + if (rt) > > + dst_release(&rt->dst); > > } > > > > dst_release() is like kfree(), it accepts a NULL argument. > 'rt->dst' already dereferences 'rt', no matter dst_release() accepts NULL or not. -- 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
On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote: > On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote: > > > - dst_release(&rt->dst); > > > + if (rt) > > > + dst_release(&rt->dst); > > > } > > > > > > > dst_release() is like kfree(), it accepts a NULL argument. > > > > 'rt->dst' already dereferences 'rt', no matter dst_release() accepts > NULL or not. > > &rt->dst doesnt dereference rt, you are quite mistaken. if rt is NULL, &rt->dst is also 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
On Mon, 2012-10-29 at 08:05 +0100, Eric Dumazet wrote: > On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote: > > On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote: > > > > - dst_release(&rt->dst); > > > > + if (rt) > > > > + dst_release(&rt->dst); > > > > } > > > > > > > > > > dst_release() is like kfree(), it accepts a NULL argument. > > > > > > > 'rt->dst' already dereferences 'rt', no matter dst_release() accepts > > NULL or not. > > > > > > &rt->dst doesnt dereference rt, you are quite mistaken. > > if rt is NULL, &rt->dst is also NULL > Oh, yeah, gcc should be smart enough to do calculation without deref it given it has the offset and the address. And dst happens to be first field of rt, so offset is 0, &rt->dst should be NULL too if rt is NULL. But this will be a problem if someone moved dst inside rt, as there is no comment saying dst has to be the first one? -- 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
On Mon, 2012-10-29 at 15:25 +0800, Cong Wang wrote: > Oh, yeah, gcc should be smart enough to do calculation without deref it > given it has the offset and the address. And dst happens to be first > field of rt, so offset is 0, &rt->dst should be NULL too if rt is NULL. > There is no dereference, even if gcc was dumb, since dst is an embedded struct, not a pointer to a struct. This wont change in a near future. > But this will be a problem if someone moved dst inside rt, as there is > no comment saying dst has to be the first one? I dont think this placement will change in a near future, it would break lot of things. I guess we can use a BUILD_BUG_ON() instead of a comment that could be ignored. Note we could move dst in rtable, if we change the NULL test in dst_release with some if ((unsigned long)dst < 4000) condition I'll send a patch against ip_rt_put() -- 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: Cong Wang <amwang@redhat.com> Date: Mon, 29 Oct 2012 14:49:26 +0800 > On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote: >> > - dst_release(&rt->dst); >> > + if (rt) >> > + dst_release(&rt->dst); >> > } >> > >> >> dst_release() is like kfree(), it accepts a NULL argument. >> > > 'rt->dst' already dereferences 'rt', no matter dst_release() accepts > NULL or not. It's taking the address of a struct member, it's not a dereference. You know what the difference is right? -- 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: Cong Wang <amwang@redhat.com> Date: Mon, 29 Oct 2012 15:25:23 +0800 > But this will be a problem if someone moved dst inside rt, as there > is no comment saying dst has to be the first one? If we move the initial dst member, many things that depend upon it being first will have to change. Your change was unnecessary and inappropriate, and let's just leave it at that. Thanks. -- 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
On Mon, 2012-10-29 at 13:29 -0400, David Miller wrote: > From: Cong Wang <amwang@redhat.com> > Date: Mon, 29 Oct 2012 15:25:23 +0800 > > > But this will be a problem if someone moved dst inside rt, as there > > is no comment saying dst has to be the first one? > > If we move the initial dst member, many things that depend upon it > being first will have to change. > > Your change was unnecessary and inappropriate, and let's just leave > it at that. Yeah, that is why I sent: http://marc.info/?l=linux-netdev&m=135150561014916&w=2 :) -- 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
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 8f0b12a..c467dbb 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -951,7 +951,8 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) rt6_set_expires(rt, expires); } } - dst_release(&rt->dst); + if (rt) + dst_release(&rt->dst); } /* clean up prefsrc entries */
In ipv6_del_addr(): if (rt != net->ipv6.ip6_null_entry && addrconf_is_prefix_route(rt)) { if (onlink == 0) { ip6_del_rt(rt); rt = NULL; } else if (!(rt->rt6i_flags & RTF_EXPIRES)) { rt6_set_expires(rt, expires); } } dst_release(&rt->dst); obviously rt could be NULL'd before dst_release(), so we have to check if rt is NULL before calling it. Reported-by: Fengguang Wu <fengguang.wu@intel.com> Cc: David S. Miller <davem@davemloft.net> Signed-off-by: Cong Wang <amwang@redhat.com> --- -- 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