Re: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path
> > > @@ -249,44 +252,26 @@ static int __path_add(struct net_device *dev, > > > struct ipoib_path *path) > > > n = &pn->rb_left; > > > else if (ret > 0) > > > n = &pn->rb_right; > > > -else > > > -return -EEXIST; > > > +else /* Should never happen since we always search > > > first */ > > > +return; > > > } > > > > Why not remove the last else and change the "else if" into else? > > I don't understand. This is left, right, or return. > I'm only changing the return value to void since it is > never used. It would probably be better to split that cleanup out into a separate patch. And since we have a "should never happen" condition in the code, I guess we should either trust our code and just assume that it really never happens (ie have just an if-else as Eli suggests), or add something like a WARN_ONCE() if it actually does happen (probably safer) -- Roland Dreier || For corporate legal information go to: http://www.cisco.com/web/about/doing_business/legal/cri/index.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path
On Sun, 2010-03-28 at 09:02 -0700, Eli Cohen wrote:
> On Thu, Mar 04, 2010 at 10:58:27AM -0800, Ralph Campbell wrote:
> > Subject: [PATCH v3] IB/ipoib: fix dangling pointer references to
> > ipoib_neigh and ipoib_path
> >
> > When using connected mode, ipoib_cm_create_tx() kmallocs a
> > struct ipoib_cm_tx which contains pointers to ipoib_neigh and
> > ipoib_path. If the paths are flushed or the struct neighbour is
> > destroyed, the pointers held by struct ipoib_cm_tx can reference
> > freed memory.
> >
> I could use some more content here as this is quite a large patch.
> Anyway below are some comments. I think besides reviewing this patch
> we need to make sure it has been checked in real life.
Do you mean more details about how ipoib_cm_tx can be used after
being freed or more about how the changes fix the problem?
I have been waiting for our internal regression tests to finish
and to collect review feedback before sending out the final version
of the patch.
> > +/*
> > + * Search the list of connections to be started and remove any entries
> > + * which match the path being destroyed.
> > + *
> > + * This should be called with netif_tx_lock_bh() and priv->lock held.
> > + */
> > +void ipoib_cm_flush_path(struct net_device *dev, struct ipoib_path *path)
> > +{
> > + struct ipoib_dev_priv *priv = netdev_priv(dev);
> > + struct ipoib_cm_tx *tx;
> > +
> > + list_for_each_entry(tx, &priv->cm.start_list, list) {
> > + tx = list_entry(priv->cm.start_list.next, typeof(*tx), list);
> > + if (tx->path == path) {
> > + tx->path = NULL;
> > + list_del_init(&tx->list);
> > + break;
> > + }
> > }
> > }
>
> Looks to me like we may find struct ipoib_cm_tx objects hanging and
> not freed. This could happen if they were just added to start_list and
> removed by ipoib_cm_flush_path() before being processed by
> ipoib_cm_tx_start().
Quite right. I should also use list_for_each_entry_safe().
I will fix this.
> > -static int __path_add(struct net_device *dev, struct ipoib_path *path)
> > +static void __path_add(struct net_device *dev, struct ipoib_path *path)
> > {
> > struct ipoib_dev_priv *priv = netdev_priv(dev);
> > struct rb_node **n = &priv->path_tree.rb_node;
> > @@ -249,44 +252,26 @@ static int __path_add(struct net_device *dev, struct
> > ipoib_path *path)
> > n = &pn->rb_left;
> > else if (ret > 0)
> > n = &pn->rb_right;
> > - else
> > - return -EEXIST;
> > + else /* Should never happen since we always search first */
> > + return;
> > }
>
> Why not remove the last else and change the "else if" into else?
I don't understand. This is left, right, or return.
I'm only changing the return value to void since it is
never used.
> > }
> > @@ -460,19 +446,13 @@ static void path_rec_completion(int status,
> > memcpy(&neigh->dgid.raw, &path->pathrec.dgid.raw,
> >sizeof(union ib_gid));
> >
> > - if (ipoib_cm_enabled(dev, neigh->neighbour)) {
> > - if (!ipoib_cm_get(neigh))
> > - ipoib_cm_set(neigh,
> > ipoib_cm_create_tx(dev,
> > -
> > path,
> > -
> > neigh));
> > - if (!ipoib_cm_get(neigh)) {
> > - list_del(&neigh->list);
> > - if (neigh->ah)
> > - ipoib_put_ah(neigh->ah);
> > - ipoib_neigh_free(dev, neigh);
> > - continue;
> > - }
> > - }
> ipoib_cm_set() is called only once in neigh_alloc(), setting neigh->cm
> to NULL, and never again so all calls to ipoib_cm_get() will return
> NULL.
ipoib_cm_set() is only called once since I changed
ipoib_cm_create_tx() to initialize neigh->cm and return void.
To me, it is more clear to let ipoib_cm.c do as much of
the CM specific work as possible instead of defining two
versions of a function for when CM is compiled in or not.
I guess I could change neigh_alloc() to call kzmalloc()
and remove ip
Re: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path
On Thu, Mar 04, 2010 at 10:58:27AM -0800, Ralph Campbell wrote:
> Subject: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh
> and ipoib_path
>
> When using connected mode, ipoib_cm_create_tx() kmallocs a
> struct ipoib_cm_tx which contains pointers to ipoib_neigh and
> ipoib_path. If the paths are flushed or the struct neighbour is
> destroyed, the pointers held by struct ipoib_cm_tx can reference
> freed memory.
>
I could use some more content here as this is quite a large patch.
Anyway below are some comments. I think besides reviewing this patch
we need to make sure it has been checked in real life.
> +/*
> + * Search the list of connections to be started and remove any entries
> + * which match the path being destroyed.
> + *
> + * This should be called with netif_tx_lock_bh() and priv->lock held.
> + */
> +void ipoib_cm_flush_path(struct net_device *dev, struct ipoib_path *path)
> +{
> + struct ipoib_dev_priv *priv = netdev_priv(dev);
> + struct ipoib_cm_tx *tx;
> +
> + list_for_each_entry(tx, &priv->cm.start_list, list) {
> + tx = list_entry(priv->cm.start_list.next, typeof(*tx), list);
> + if (tx->path == path) {
> + tx->path = NULL;
> + list_del_init(&tx->list);
> + break;
> + }
> }
> }
Looks to me like we may find struct ipoib_cm_tx objects hanging and
not freed. This could happen if they were just added to start_list and
removed by ipoib_cm_flush_path() before being processed by
ipoib_cm_tx_start().
> -static int __path_add(struct net_device *dev, struct ipoib_path *path)
> +static void __path_add(struct net_device *dev, struct ipoib_path *path)
> {
> struct ipoib_dev_priv *priv = netdev_priv(dev);
> struct rb_node **n = &priv->path_tree.rb_node;
> @@ -249,44 +252,26 @@ static int __path_add(struct net_device *dev, struct
> ipoib_path *path)
> n = &pn->rb_left;
> else if (ret > 0)
> n = &pn->rb_right;
> - else
> - return -EEXIST;
> + else /* Should never happen since we always search first */
> + return;
> }
Why not remove the last else and change the "else if" into else?
> }
> @@ -460,19 +446,13 @@ static void path_rec_completion(int status,
> memcpy(&neigh->dgid.raw, &path->pathrec.dgid.raw,
> sizeof(union ib_gid));
>
> - if (ipoib_cm_enabled(dev, neigh->neighbour)) {
> - if (!ipoib_cm_get(neigh))
> - ipoib_cm_set(neigh,
> ipoib_cm_create_tx(dev,
> -
> path,
> -
> neigh));
> - if (!ipoib_cm_get(neigh)) {
> - list_del(&neigh->list);
> - if (neigh->ah)
> - ipoib_put_ah(neigh->ah);
> - ipoib_neigh_free(dev, neigh);
> - continue;
> - }
> - }
ipoib_cm_set() is called only once in neigh_alloc(), setting neigh->cm
to NULL, and never again so all calls to ipoib_cm_get() will return
NULL.
> + /*
> + * If connected mode is enabled but not started,
> + * start getting a connection.
> + */
> + if (ipoib_cm_enabled(dev, neigh->neighbour) &&
> + !ipoib_cm_get(neigh))
> + ipoib_cm_create_tx(dev, path, neigh);
>
> while ((skb = __skb_dequeue(&neigh->queue)))
> __skb_queue_tail(&skqueue, skb);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path
Subject: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh
and ipoib_path
When using connected mode, ipoib_cm_create_tx() kmallocs a
struct ipoib_cm_tx which contains pointers to ipoib_neigh and
ipoib_path. If the paths are flushed or the struct neighbour is
destroyed, the pointers held by struct ipoib_cm_tx can reference
freed memory.
Signed-off-by: Ralph Campbell
---
Changes from V1 were to remove the kref_t and avoid the extra locking.
Changes from V2 were to fix a race where a call to ipoib_cm_destroy_tx()
while the struct ipoib_cm_tx was being processed by ipoib_cm_tx_start()
could cause a use after free.
drivers/infiniband/ulp/ipoib/ipoib.h | 24 ++-
drivers/infiniband/ulp/ipoib/ipoib_cm.c| 108 +-
drivers/infiniband/ulp/ipoib/ipoib_main.c | 267 +++-
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 95 +++--
4 files changed, 225 insertions(+), 269 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h
b/drivers/infiniband/ulp/ipoib/ipoib.h
index 753a983..84bb561 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -415,9 +415,7 @@ static inline struct ipoib_neigh **to_ipoib_neigh(struct
neighbour *neigh)
INFINIBAND_ALEN, sizeof(void *));
}
-struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neigh,
- struct net_device *dev);
-void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh);
+void ipoib_neigh_flush(struct ipoib_neigh *neigh);
extern struct workqueue_struct *ipoib_workqueue;
@@ -464,7 +462,8 @@ void ipoib_dev_cleanup(struct net_device *dev);
void ipoib_mcast_join_task(struct work_struct *work);
void ipoib_mcast_carrier_on_task(struct work_struct *work);
-void ipoib_mcast_send(struct net_device *dev, void *mgid, struct sk_buff *skb);
+void ipoib_mcast_send(struct net_device *dev, void *mgid, struct sk_buff *skb,
+ struct ipoib_neigh *neigh);
void ipoib_mcast_restart_task(struct work_struct *work);
int ipoib_mcast_start_thread(struct net_device *dev);
@@ -567,9 +566,10 @@ void ipoib_cm_dev_stop(struct net_device *dev);
int ipoib_cm_dev_init(struct net_device *dev);
int ipoib_cm_add_mode_attr(struct net_device *dev);
void ipoib_cm_dev_cleanup(struct net_device *dev);
-struct ipoib_cm_tx *ipoib_cm_create_tx(struct net_device *dev, struct
ipoib_path *path,
- struct ipoib_neigh *neigh);
+void ipoib_cm_create_tx(struct net_device *dev, struct ipoib_path *path,
+ struct ipoib_neigh *neigh);
void ipoib_cm_destroy_tx(struct ipoib_cm_tx *tx);
+void ipoib_cm_flush_path(struct net_device *dev, struct ipoib_path *path);
void ipoib_cm_skb_too_long(struct net_device *dev, struct sk_buff *skb,
unsigned int mtu);
void ipoib_cm_handle_rx_wc(struct net_device *dev, struct ib_wc *wc);
@@ -646,10 +646,10 @@ void ipoib_cm_dev_cleanup(struct net_device *dev)
}
static inline
-struct ipoib_cm_tx *ipoib_cm_create_tx(struct net_device *dev, struct
ipoib_path *path,
- struct ipoib_neigh *neigh)
+void ipoib_cm_create_tx(struct net_device *dev, struct ipoib_path *path,
+ struct ipoib_neigh *neigh)
{
- return NULL;
+ return;
}
static inline
@@ -659,6 +659,12 @@ void ipoib_cm_destroy_tx(struct ipoib_cm_tx *tx)
}
static inline
+void ipoib_cm_flush_path(struct net_device *dev, struct ipoib_path *path)
+{
+ return;
+}
+
+static inline
int ipoib_cm_add_mode_attr(struct net_device *dev)
{
return 0;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 30bdf42..27f494d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -794,31 +794,14 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct
ib_wc *wc)
if (wc->status != IB_WC_SUCCESS &&
wc->status != IB_WC_WR_FLUSH_ERR) {
- struct ipoib_neigh *neigh;
-
ipoib_dbg(priv, "failed cm send event "
"(status=%d, wrid=%d vend_err %x)\n",
wc->status, wr_id, wc->vendor_err);
spin_lock_irqsave(&priv->lock, flags);
- neigh = tx->neigh;
-
- if (neigh) {
- neigh->cm = NULL;
- list_del(&neigh->list);
- if (neigh->ah)
- ipoib_put_ah(neigh->ah);
- ipoib_neigh_free(dev, neigh);
-
- tx->neigh = NULL;
- }
-
- if (test_and_clear_bit(IPOIB_FLAG_INITIALIZED, &tx->flags)) {
- list_move(&tx->list, &priv-
