[openib-general] [PATCH] IB/ipoib: Add field dev to struct ipoib_neigh

[Moni Shoua]

Michael S. Tsirkin wrote:
>>>
>>>Just to clarify - you previously mentionned you saw problems with 2.6.16
>>>backport. Is this an issue you see with 2.6.20 as well?
>>
>>Yes, the same thing happens with kernel 2.6.20. However, the patch for 2.6.20
>>looks a little bit different. I will post it today or tommorow.
> 
> 
> Let's see that first. I prefer to first look at upstream code, then think
> about backporting.
> 
OK, I will post this patch today.

> But this would hardly help if ipoib module is unloaded while neighbour
> for bonding device is still around and has a pointer to ipoib_neigh_destructor.
> 
> 
>>For later kernels, bond device "borrows" the slave's neigh_setup
>>function in the bond's setup function.
>>
>> ==> bond_dev->neigh_setup = slave_dev->neigh_setup;
>>
>>So even if the beighbour points to bond device the
>>ipoib_neigh_destructor will be called.
> 
> 
> Same applies here.
> 
This is a good point. The right solution in my opinion is to enforce a correct order 
of unloading the modules. First bonding and than IPoIB. We still have to think how do 
we want to implement this. 
> Further, in both cases, it seems that accessing data at to_ipoib_neigh on a neighbour for
> non-ipoib device can cause a crash if hardware address is !=0 at offset 20.
> 
I don't see such risk. the ipoib_neigh_destructor is called only for neighbours that were passed 
as an argument to ipoib_neigh_alloc (for kernels <= 2.6.16) or for devices that set their neigh_setup
function to ipoib_neigh_setup_dev (for bigger kernels). The only one (besides IPoIB of course) that does 
that is bonding and bonding cannot enslave devices of different types. So, once bonding sets its neigh_setup 
to ipoib_neigh_setup_dev, it means it enslaves an IPoIB device and won't enslave devices of other types.
However, it might be good idea to change the condition in bonding to "borrow" the neigh_setup function.
Currently it is (slave_type != Ethernet) but should be (slave_type == IPoIB).