On Wed, Jun 10, 2009 at 10:52:29PM +0300, Yossi Etigin wrote:
> [email protected] wrote:
> > @@ -810,9 +810,6 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev,
> > struct ib_wc *wc)
> > 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)) {
>
> Can't calling ipoib_put_ah() and keeping the neighbour alive cause neigh->ah
> pointer become invalid? What about doing list_del(&neigh->list) twice?
>
Yeah, looks like there's a hole there. By changing the test from:
if (neigh) {
neigh->cm = NULL;
to:
if (neigh && neigh->cm) {
neigh->cm = NULL;
it can be closed.
> ......
>
> The piece of code that does '*to_ipoib_neigh(neigh->neighbour) = NULL' and
> releases skb's will appear 3 times in ipoib. I think it can be put in a
> separate function.
OK.
>
>
> > @@ -1343,7 +1351,23 @@ static void ipoib_cm_tx_reap(struct work_struct
> > *work)
> > spin_lock_irqsave(&priv->lock, flags);
> >
> > while (!list_empty(&priv->cm.reap_list)) {
> > + struct ipoib_neigh *neigh;
> > + struct sk_buff *skb;
> > p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
> > + neigh = p->neigh;
> > + if (neigh) {
> > + *to_ipoib_neigh(neigh->neighbour) = NULL;
> > + ipoib_dbg(priv, "%s: releasing ref to %pI6 "
> > + "(refcnt: %d)\n", __func__, neigh->dgid.raw,
> > + atomic_read(&neigh->neighbour->refcnt));
> > + neigh_release(neigh->neighbour);
> > + while ((skb = __skb_dequeue(&neigh->queue))) {
> > + ++dev->stats.tx_dropped;
> > + dev_kfree_skb_any(skb);
> > + }
> > + kfree(neigh);
> > + }
> > + p->neigh = NULL;
> > list_del(&p->list);
> > spin_unlock_irqrestore(&priv->lock, flags);
> > netif_tx_unlock_bh(dev);
>
> Couldn't this race with ipoib_neigh_cleanup()?
>
Generally the fact that we hold a reference should prevent
ipoib_neigh_cleanup() from being invoked.
But, alas, there are a couple of cases where the neighbour can
be have the neigh_cleanup() method invoked even when there are
references held (e.g., when the interface goes down).
So, yeah, there seems to be a hole there. A smaller hole, but
still....
--
Arthur
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
