Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

On 14/12/2018 19:43, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This patch adds support for fdb get similar to
> route get. arguments can be any of the following (similar to fdb add/del/dump):
> [bridge, mac, vlan] or
> [bridge_port, mac, vlan, flags=[NTF_MASTER]] or
> [dev, mac, [vni|vlan], flags=[NTF_SELF]]
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  include/linux/netdevice.h |   7 ++-
>  net/core/rtnetlink.c      | 107 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 112 insertions(+), 2 deletions(-)
> 

Just saw there will be a v2, a few style nits and one suggestion below for it. :)

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 811632d..1377d08 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1387,7 +1387,12 @@ struct net_device_ops {
>  						struct net_device *dev,
>  						struct net_device *filter_dev,
>  						int *idx);
> -
> +	int			(*ndo_fdb_get)(struct sk_buff *skb,
> +					       struct nlattr *tb[],
> +					       struct net_device *dev,
> +					       const unsigned char *addr,
> +					       u16 vid, u32 portid, u32 seq,
> +					       struct netlink_ext_ack *extack);
>  	int			(*ndo_bridge_setlink)(struct net_device *dev,
>  						      struct nlmsghdr *nlh,
>  						      u16 flags,
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f8bdb8ad..fcea76b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> +			struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = NULL;
> +	struct net *net = sock_net(in_skb->sk);
> +	struct net_device *dev = NULL, *br_dev = NULL;
^^
Reverse xmas, seems like this should be on top.

> +	struct nlattr *tb[NDA_MAX + 1];
> +	struct sk_buff *skb;
> +	struct ndmsg *ndm;
> +	int br_idx = 0;
> +	u8 *addr;
> +	u16 vid;
> +	int err;
> +
> +	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);
> +	if (err < 0)
> +		return err;
> +
> +	ndm = nlmsg_data(nlh);
> +	if (ndm->ndm_ifindex) {
> +		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
> +		if (!dev) {
> +			NL_SET_ERR_MSG(extack, "unknown dev ifindex");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> +		NL_SET_ERR_MSG(extack, "invalid address");
> +		return -EINVAL;
> +	}
> +
> +	addr = nla_data(tb[NDA_LLADDR]);
> +
> +	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
> +	if (err)
> +		return err;
> +
> +	if (tb[NDA_MASTER]) {
> +		if (dev) {
> +			NL_SET_ERR_MSG(extack, "master and dev are mutually exclusive");
> +			return -EINVAL;
> +		}
> +
> +		br_idx = nla_get_u32(tb[NDA_MASTER]);
> +		br_dev = __dev_get_by_index(net, br_idx);
> +		if (!br_dev) {
> +			NL_SET_ERR_MSG(extack, "invalid master ifindex");
> +			return -EINVAL;
> +		}
> +		ops = br_dev->netdev_ops;
> +	}
> +
> +	if (dev) {
> +		if (!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) {
^^
Please put the NTF_MASTER check in ().

> +			if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
> +				NL_SET_ERR_MSG(extack, "dev is not a bridge port");
> +				return -EINVAL;
> +			}
> +			br_dev = netdev_master_upper_dev_get(dev);
> +			if (!br_dev) {
> +				NL_SET_ERR_MSG(extack, "master of dev not found");
> +				return -EINVAL;
> +			}
> +			ops = br_dev->netdev_ops;
> +		} else {
> +			if (!(ndm->ndm_flags & NTF_SELF)) {
> +				NL_SET_ERR_MSG(extack, "missing NTF_SELF");
> +				return -EINVAL;
> +			}
> +			ops = dev->netdev_ops;
> +		}
> +	}
> +
> +	if (!br_dev && !dev) {
> +		NL_SET_ERR_MSG(extack, "Unable to find device");^^
I think the error could be improved, if I'm not missing something this case
can happen only if ndm_ifindex = 0 and NDA_MASTER is missing which means
no device was specified in the request, the other cases where the respective
devices are not found are handled above. Perhaps something like
"No master or port device specified".

> +		return -ENODEV;
> +	}
> +
> +	if (!ops || !ops->ndo_fdb_get) {
> +		NL_SET_ERR_MSG(extack, "fdb get operation not supported by device");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOBUFS;
> +
> +	if (br_dev)
> +		err = ops->ndo_fdb_get(skb, tb, br_dev, addr, vid,
> +				       NETLINK_CB(in_skb).portid,
> +				       nlh->nlmsg_seq, extack);
> +	else
> +		err = ops->ndo_fdb_get(skb, tb, dev, addr, vid,
> +				       NETLINK_CB(in_skb).portid,
> +				       nlh->nlmsg_seq, extack);
> +	if (err)
> +		goto out;
> +
> +	return rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +out:
> +	kfree_skb(skb);
> +	return err;
> +}
> +
>  static int brport_nla_put_flag(struct sk_buff *skb, u32 flags, u32 mask,
>  			       unsigned int attrnum, unsigned int flag)
>  {
> @@ -5081,7 +5186,7 @@ void __init rtnetlink_init(void)
>  
>  	rtnl_register(PF_BRIDGE, RTM_NEWNEIGH, rtnl_fdb_add, NULL, 0);
>  	rtnl_register(PF_BRIDGE, RTM_DELNEIGH, rtnl_fdb_del, NULL, 0);
> -	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, NULL, rtnl_fdb_dump, 0);
> +	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>  
>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
>  	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, 0);
>