Re: [RFC PATCH 2/3] net: expose ebridge FDB with priv flag IFF_OFFLOADED_FDB

[John Fastabend <john.r.fastabend@intel.com>]

On 3/2/2012 11:56 AM, Ben Hutchings wrote:
> On Tue, 2012-02-28 at 23:17 -0800, John Fastabend wrote:
>> This adds a new private interface flag IFF_OFFLOADED_FDB and an
>> additional ndmsg flag NTF_EMBEDDED.
>>
>> The private flag IFF_OFFLOADED_FDB should be set on devices to
>> indicate an embedded bridging component exists with a forwarding
>> database.
>>
>> With this set PF_BRIDGE:{RTM_NEWNEIGH|RTM_DELNEIGH|RTM_GETNEIGH}
>> netlink msgs can manage the unicast address list on these devices
>> by setting the NTF_EMBEDDED flag in ndm_flags.
>>
>> These commands are compatible with the SW bridge allowing the same
>> user space tools to be used with both SW bridges and HW bridges.
> [...]
>> index 9e70191..7b1a581 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -211,6 +211,57 @@ static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>>  	return 0;
>>  }
>>  
>> +static int rtnl_offloaded_fdb_add(struct nlmsghdr *nlh, struct net_device *dev)
>> +{
>> +	struct ndmsg *ndm;
>> +	struct netdev_hw_addr *ha;
>> +	struct nlattr *tb[NDA_MAX+1];
>> +	__u8 *addr;
>> +	int err;
>> +
>> +	ASSERT_RTNL();
>> +
>> +	if (!(dev->priv_flags & IFF_OFFLOADED_FDB))
>> +		return -ENODEV;
> 
> EOPNOTSUPP
> 

Right thanks.

[...]

>> +	if (is_multicast_ether_addr(addr))
>> +		return -EINVAL;
> 
> There is a pending change to the ndo_validate_addr interface which I
> think would allow us to replace the Ethernet-specific checks with a
> check against dev->addr_len and a call to ndo_validate_addr.  Not that I
> really care about anything other than Ethernet, but it would be a little
> more elegant.
> 
>> +	netif_addr_lock_bh(dev);
>> +	list_for_each_entry(ha, &dev->uc.list, list) {
>> +		if (!compare_ether_addr(ha->addr, addr)) {
>> +			netif_addr_unlock_bh(dev);
>> +			return -EEXIST;
>> +		}
>> +	}
>> +	netif_addr_unlock_bh(dev);
>> +
>> +	err = dev_uc_add(dev, addr);
> [...]
> 
> The software bridge makes the duplicate check conditional on NLM_F_EXCL.
> Shouldn't this be consistent?

Agreed it should be consistent.

> 
> Also, this seems racy.  If all manipulation of the UC address list is
> serialised by RTNL (which I think in practice it is) then there is no
> race and we also don't need to take the netif_addr_lock(), but if we're
> not allowed to assume that then we shouldn't drop the lock.  Perhaps the
> check can be moved into a dev_uc_add_exclusive() or something like that.
> 

Yep I'll at least make this consistent with other usages. And maybe a
follow up patch to clean up the usage throughout.

Thanks,
John