[ofa-general] Re: Kernel panic in IPoIB stability testing

[Jack Morgenstein]

On Wednesday 04 February 2009 17:45, Moni Shoua wrote:
> Besides the locking issue that I hadn't think about yet what if we this fix looks the right thing to do.
> But what if we leave the path without freeing it even if path_rec_start() fails?
> This would leave a path which is not valid in path_list which is not forbidden state as 
> I conclude (after all this is the state the function was called)
> In this way, I think that we don't  have to worry about locks.
> 
> and the code will look like this
> 
>         if (!path || !path->valid) {
>                 if (!path)
>                         path = path_rec_create(dev, phdr->hwaddr + 4);
>                 if (path) {
>                         /* put pseudoheader back on for next time */
>                         skb_push(skb, sizeof *phdr);
>                         __skb_queue_tail(&path->queue, skb);
> 
>                         if (!path->query && path_rec_start(dev, path)) {
>                                 spin_unlock_irqrestore(&priv->lock, flags);
> -                               path_free(dev, path);
>                                 return;
>                         } else
>                                 __path_add(dev, path);
>                 } else {
>                         ++priv->stats.tx_dropped;
>                         dev_kfree_skb_any(skb);
>                 }
> 
>                 spin_unlock_irqrestore(&priv->lock, flags);
>                 return;
>         }
> 
Still need some correction.  If the path did not exist previously (i.e, !path = TRUE,
and, below, had_path = 0), then need to call path_free or we will have a leak.

Maybe the correct patch is:
       path = __path_find(dev, phdr->hwaddr + 4);
        if (!path || !path->valid) {
                int had_path = 0;
                if (!path)
                        path = path_rec_create(dev, phdr->hwaddr + 4);
                else
                        had_path = 1;
                if (path) {
                        /* put pseudoheader back on for next time */
                        skb_push(skb, sizeof *phdr);
                        __skb_queue_tail(&path->queue, skb);

                        if (!path->query && path_rec_start(dev, path)) {
                                spin_unlock_irqrestore(&priv->lock, flags);
				if (!had_path)
                                	path_free(dev, path);
                                return;
                        } else
                                __path_add(dev, path);
                } else {
                        ++dev->stats.tx_dropped;
                        dev_kfree_skb_any(skb);
                }