Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

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

On 4/10/2012 8:23 PM, Ben Hutchings wrote:
> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
> [...]
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 1f77540..05822e5 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
> [...]
>> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
>>   *	feature set might be less than what was returned by ndo_fix_features()).
>>   *	Must return >0 or -errno if it changed dev->features itself.
>>   *
>> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
>> + *		      unsigned char *addr, u16 flags)
>> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
>> + *	if the dev->master FDB should be updated or the devices internal FDB.
> 
> I don't think the second sentence is helpful, as rtnl_fdb_add() will
> take care of those flags.
> 
>> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
>> + *		      unsigned char *addr)
>> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
>> + *	contains flags to indicate if the dev->master FDB should be
>> + *	updated or the devices internal FDB.
> 
> Similarly here.

agreed neither seem particularly helpful I'll remove them.

> 
>> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
>> + *		       struct net_device *dev, int idx)
>> + *	Used to add FDB entries to dump requests. Implementers should add
>> + *	entries to skb and update idx with the number of entries.
>>   */
> [...
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
> [...]
>> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>> +{
> [...]
>> +	err = -EOPNOTSUPP;
> 
> So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
> just setting one FDB?  Not sure that's really desirable though 
> 

It makes it easier to keep an embedded agent and the sw bridge in
sync if setting both flags adds the entry to both the SW bridge and
embedded bridge. But the error case gets a bit tricky as your comments
below indicate.

>> +	/* Support fdb on master device the net/bridge default case */
>> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
>> +		struct net_device *master = dev->master;
>> +
>> +		if (master->netdev_ops->ndo_fdb_add)
> 
> This operation is surely going to be mandatory for bridge devices, so
> this check should be omitted or changed to a BUG_ON().

I'll just remove the check.

> 
>> +			err = master->netdev_ops->ndo_fdb_add(ndm, dev, addr,
>> +							      nlh->nlmsg_flags);
> 
> Shoudn't we return early on error?

Agreed better to return an err here.

> 
>> +	}
>> +
>> +	/* Embedded bridge, macvlan, and any other device support */
>> +	if ((ndm->ndm_flags & NTF_SELF) &&
>> +	    dev->netdev_ops->ndo_fdb_add)
> 
> Error if !dev->netdev_ops->ndo_fdb_add.
> 
>> +		err = dev->netdev_ops->ndo_fdb_add(ndm, dev, addr,
>> +						   nlh->nlmsg_flags);
> 
> Wonder what we should do on error here if we've already successfully
> called ndo_fdb_add on the master?  Should we try to roll back the first
> addition?
> 

The problem with rolling back is the table is likely already updated and
traffic may already be being forwarded. So I think in this case the user
will have to query the device to learn what failed. It seems like the
simplest way to handle this. I think it is unwanted to have traffic being
forwarded one way for a short period of time then rolled back.

The other idea I just had is we could clear the NTF_ bit in ndm_flags after
the successful add, del command. I believe the nlmsg gets sent back to the
user on error I would need to check on this.

>> +	return err;
>> +}
>> +
>> +static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>> +{
> [...]
>> +	err = -EOPNOTSUPP;
>> +
>> +	/* Support fdb on master device the net/bridge default case */
>> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
>> +		struct net_device *master = dev->master;
>> +
>> +		if (master->netdev_ops->ndo_fdb_del)
>> +			err = master->netdev_ops->ndo_fdb_del(ndm, dev, addr);
>> +	}
>> +
>> +	/* Embedded bridge, macvlan, and any other device support */
>> +	if ((ndm->ndm_flags & NTF_SELF) &&
>> +	    dev->netdev_ops->ndo_fdb_del)
>> +		err = dev->netdev_ops->ndo_fdb_del(ndm, dev, addr);
>> +	return err;
>> +}
> [...]
> 
> This has the same issues.
> 

same comment as above

> Ben.
>