[ofa-general] Re: IB/ipoib: Fix neigh destructor oops for kernels older than 2.6.21

[Or Gerlitz]

Eli Cohen wrote:
> IB/ipoib: Fix neigh destructor oops
>
> For kernels 2.6.20 and older, it may happen that the pointer to
> ipoib_neigh_cleanup() is called after IPoIB has been unloades,
> causing a kernel oops. This problem has been fixed for 2.6.21 with
> the following commit: ecbb416939da77c0d107409976499724baddce7b
Hi Eli,

Before looking into the solution, I'd like to slow down a little and 
understand the problem (how can ipoib_neigh_cleanup() called after IPoIB 
has been unloaded) and why does the commit below solve it, from its 
change-log I don't see any reference to this problem:

> commit ecbb416939da77c0d107409976499724baddce7b
> Author: Alexey Kuznetsov <kuznet at ms2.inr.ac.ru>
> Date:   Sat Mar 24 12:52:16 2007 -0700
>
>     [NET]: Fix neighbour destructor handling.
>     
>     ->neigh_destructor() is killed (not used), replaced with
>     ->neigh_cleanup(), which is called when neighbor entry goes to dead
>     state. At this point everything is still valid: neigh->dev,
>     neigh->parms etc.
>     
>     The device should guarantee that dead neighbor entries (neigh->dead !=
>     0) do not get private part initialized, otherwise nobody will cleanup
>     it.
>     
>     I think this is enough for ipoib which is the only user of this thing.
>     Initialization private part of neighbor entries happens in ipib
>     start_xmit routine, which is not reached when device is down.  But it
>     would be better to add explicit test for neigh->dead in any case.
>     
>     Signed-off-by: David S. Miller <davem at davemloft.net>
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 0741c6d..f2a40ae 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -814,7 +814,7 @@ static void ipoib_set_mcast_list(struct net_device *dev)
>  	queue_work(ipoib_workqueue, &priv->restart_task);
>  }
>  
> -static void ipoib_neigh_destructor(struct neighbour *n)
> +static void ipoib_neigh_cleanup(struct neighbour *n)
>  {
>  	struct ipoib_neigh *neigh;
>  	struct ipoib_dev_priv *priv = netdev_priv(n->dev);
> @@ -822,7 +822,7 @@ static void ipoib_neigh_destructor(struct neighbour *n)
>  	struct ipoib_ah *ah = NULL;
>  
>  	ipoib_dbg(priv,
> -		  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
> +		  "neigh_cleanup for %06x " IPOIB_GID_FMT "\n",
>  		  IPOIB_QPN(n->ha),
>  		  IPOIB_GID_RAW_ARG(n->ha + 4));
>  
> @@ -874,7 +874,7 @@ void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh)
>  
>  static int ipoib_neigh_setup_dev(struct net_device *dev, struct neigh_parms *parms)
>  {
> -	parms->neigh_destructor = ipoib_neigh_destructor;
> +	parms->neigh_cleanup = ipoib_neigh_cleanup;
>  
>  	return 0;
>  }
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 3725b93..ad7fe11 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -36,7 +36,7 @@ struct neigh_parms
>  	struct net_device *dev;
>  	struct neigh_parms *next;
>  	int	(*neigh_setup)(struct neighbour *);
> -	void	(*neigh_destructor)(struct neighbour *);
> +	void	(*neigh_cleanup)(struct neighbour *);
>  	struct neigh_table *tbl;
>  
>  	void	*sysctl_table;
> diff --git a/net/atm/clip.c b/net/atm/clip.c
> index ebb5d0c..8c38258 100644
> --- a/net/atm/clip.c
> +++ b/net/atm/clip.c
> @@ -261,14 +261,6 @@ static void clip_pop(struct atm_vcc *vcc, struct sk_buff *skb)
>  	spin_unlock_irqrestore(&PRIV(dev)->xoff_lock, flags);
>  }
>  
> -static void clip_neigh_destroy(struct neighbour *neigh)
> -{
> -	DPRINTK("clip_neigh_destroy (neigh %p)\n", neigh);
> -	if (NEIGH2ENTRY(neigh)->vccs)
> -		printk(KERN_CRIT "clip_neigh_destroy: vccs != NULL !!!\n");
> -	NEIGH2ENTRY(neigh)->vccs = (void *) NEIGHBOR_DEAD;
> -}
> -
>  static void clip_neigh_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  {
>  	DPRINTK("clip_neigh_solicit (neigh %p, skb %p)\n", neigh, skb);
> @@ -342,7 +334,6 @@ static struct neigh_table clip_tbl = {
>  	/* parameters are copied from ARP ... */
>  	.parms = {
>  		.tbl 			= &clip_tbl,
> -		.neigh_destructor	= clip_neigh_destroy,
>  		.base_reachable_time 	= 30 * HZ,
>  		.retrans_time 		= 1 * HZ,
>  		.gc_staletime 		= 60 * HZ,
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3183142..cfc6001 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -140,6 +140,8 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>  				n->dead = 1;
>  				shrunk	= 1;
>  				write_unlock(&n->lock);
> +				if (n->parms->neigh_cleanup)
> +					n->parms->neigh_cleanup(n);
>  				neigh_release(n);
>  				continue;
>  			}
> @@ -211,6 +213,8 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
>  				NEIGH_PRINTK2("neigh %p is stray.\n", n);
>  			}
>  			write_unlock(&n->lock);
> +			if (n->parms->neigh_cleanup)
> +				n->parms->neigh_cleanup(n);
>  			neigh_release(n);
>  		}
>  	}
> @@ -582,9 +586,6 @@ void neigh_destroy(struct neighbour *neigh)
>  			kfree(hh);
>  	}
>  
> -	if (neigh->parms->neigh_destructor)
> -		(neigh->parms->neigh_destructor)(neigh);
> -
>  	skb_queue_purge(&neigh->arp_queue);
>  
>  	dev_put(neigh->dev);
> @@ -675,6 +676,8 @@ static void neigh_periodic_timer(unsigned long arg)
>  			*np = n->next;
>  			n->dead = 1;
>  			write_unlock(&n->lock);
> +			if (n->parms->neigh_cleanup)
> +				n->parms->neigh_cleanup(n);
>  			neigh_release(n);
>  			continue;
>  		}
> @@ -2088,8 +2091,11 @@ void __neigh_for_each_release(struct neigh_table *tbl,
>  			} else
>  				np = &n->next;
>  			write_unlock(&n->lock);
> -			if (release)
> +			if (release) {
> +				if (n->parms->neigh_cleanup)
> +					n->parms->neigh_cleanup(n);
>  				neigh_release(n);
> +			}
>  		}
>  	}
>  }
>