[openib-general] [PATCH] IB/ipoib get net_device from ipoib_neigh instead of linux neighbour

[Moni Shoua]

Michael S. Tsirkin wrote:
>>------------------------------------------------------------------------------
>>IPoIB uses a two layer neighboring scheme, such that for each struct neighbour
>>whose device is an ipoib one, there is a struct ipoib_neigh buddy which is
>>created on demand at the tx flow by an ipoib_neigh_alloc(skb->dst->neighbour)
>>call.
>>
>>When using the bonding driver, neighbours are created by the net stack on behalf
>>of the bonding (master) device. On the tx flow the bonding code gets an skb such
>>that skb->dev points to the master device, it changes this skb to point on the
>>slave device and calls the slave hard_start_xmit function.
>>
>>Combing these two flows, there is a hole if some code at ipoib
>>(ipoib_neigh_destructor) assumes that for each struct neighbour it gets, n->dev
>>is an ipoib device so for example netdev_priv(n->dev) would be of type struct
>>ipoib_dev_priv.
> 
> 
> Could you plese elaborate how ipoib_neigh_destructor comes to be called at all?
> At what point does ipoib_neigh_setup_dev get called?
> 
> 
The bond device uses its slave's neigh_setup function.
Please look at line 19 below from the bonding code.
static void bond_setup_by_slave(struct net_device *bond_dev,
     11 +               struct net_device *slave_dev)
     12 +{
     13 +   bond_dev->hard_header           = slave_dev->hard_header;
     14 +   bond_dev->rebuild_header        = slave_dev->rebuild_header;
     15 +   bond_dev->hard_header_cache = slave_dev->hard_header_cache;
     16 +   bond_dev->header_cache_update   = slave_dev->header_cache_update;
     17 +   bond_dev->hard_header_parse = slave_dev->hard_header_parse;
     18 +
     19 +   bond_dev->neigh_setup       = slave_dev->neigh_setup;
     20 +
     21 +   bond_dev->type          = slave_dev->type;
     22 +   bond_dev->hard_header_len   = slave_dev->hard_header_len;
     23 +   bond_dev->addr_len      = slave_dev->addr_len;
     24 +
     25 +   memcpy(bond_dev->broadcast, slave_dev->broadcast,
     26 +       slave_dev->addr_len);
     27 +}
>>To fix it, this patch adds a dev field to struct ipoib_neigh which is used
>>instead of the struct neighbour dev one.
> 
> 
> What I am concerned with is - if the master is not an IPoIB device,
> what guarantee do we have that to_ipoib_neigh will return 0
> and not part of an actual hardware address?
> 
> Without bonding, the reason is that dev points to an ipoib device,
> so we know hw address is 20 bytes.
> 

I guess you meant "if the slave is not an IPoIB device"...

The bond device doesn't allow devices of different types to be grouped
together as its slaves. Furthermore, bond_setup_by_slave is called only for non
Ethernet devices (we consider to change the logic to "called only for
IPoIB devices just for safety).