Messages in this thread Patch in this message | | | Date | Sun, 18 Mar 2007 23:20:46 +0200 | From | "Michael S. Tsirkin" <> | Subject | Re: dst_ifdown breaks infiniband? |
| |
> Quoting Michael S. Tsirkin <mst@dev.mellanox.co.il>: > Subject: Re: dst_ifdown breaks infiniband? > > > Quoting Michael S. Tsirkin <mst@dev.mellanox.co.il>: > > Subject: Re: dst_ifdown breaks infiniband? > > > > Quoting Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>: > > Subject: Re: dst_ifdown breaks infiniband? > > > > Can dst->neighbour be changed to point to NULL instead, and the neighbour > > > > released? > > > > > > It should be cleared and we should be sure it will not be destroyed > > > before quiescent state. > > > > > > Seems, this is the only correct solution, but to do this we have > > > to audit all the places where dst->neighbour is dereferenced for > > > RCU safety. > > > > > > Actually, it is very good you caught this eventually, the bug was > > > so _disgusting_ that it was "forgotten" all the time, waiting for > > > someone who will point out that the king is naked. :-) > > > > Actually that might not be too bad: > > $grep -rIi 'dst->neighbour' net/ | wc -l > > 36 > > > > I'll try to do it. > > Here's the list. Looks OK to me. What do you think? >
So Alexey, how does the following (lightly tested) patch look? Is this what you had in mind?
-----------------------------
Fix dst_ifdown for infiniband.
Changing dst->neighbour->dev is unsafe because neigh->parms callbacks are set up for specific device. We should drop the dst->neighbour reference instead.
Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>
---
diff --git a/net/core/dst.c b/net/core/dst.c index 764bccb..27091a5 100644 --- a/net/core/dst.c +++ b/net/core/dst.c @@ -15,6 +15,7 @@ #include <linux/skbuff.h> #include <linux/string.h> #include <linux/types.h> +#include <linux/if_arp.h> #include <net/dst.h> @@ -235,6 +236,8 @@ again: static inline void dst_ifdown(struct dst_entry *dst, struct net_device *dev, int unregister) { + struct neighbour *neigh; + if (dst->ops->ifdown) dst->ops->ifdown(dst, dev, unregister); @@ -245,13 +248,13 @@ static inline void dst_ifdown(struct dst_entry *dst, struct net_device *dev, dst->input = dst_discard_in; dst->output = dst_discard_out; } else { + neigh = dst->neighbour; dst->dev = &loopback_dev; dev_hold(&loopback_dev); dev_put(dev); - if (dst->neighbour && dst->neighbour->dev == dev) { - dst->neighbour->dev = &loopback_dev; - dev_put(dev); - dev_hold(&loopback_dev); + if (neigh && neigh->dev == dev) { + dst->neighbour = NULL; + neigh_release(neigh); } } }
-- MST - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |