Linux Ethernet Bridging - Apr 2022 - [Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del

On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov
wrote:
> When NLM_F_BULK is specified in a fdb del message we need to handle it
> differently. First since this is a new call we can strictly validate the
> passed attributes, at first only ifindex and vlan are allowed as these
> will be the initially supported filter attributes, any other attribute
> is rejected. The mac address is no longer mandatory, but we use it
> to error out in older kernels because it cannot be specified with bulk
> request (the attribute is not allowed) and then we have to dispatch
> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
> callback can do further validation of the attributes if necessary.
> 
> Signed-off-by: Nikolay Aleksandrov <razor at blackwall.org>
> ---
> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
> 
>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 63c7df52a667..520d50fcaaea 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>  }
>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>  
> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
> +	[NDA_VLAN]	= { .type = NLA_U16 },
In earlier versions br_vlan_valid_id() was used to validate the VLAN,
but I don't see it anymore. Maybe use 

NLA_POLICY_RANGE(1, VLAN_N_VID - 2)

?

I realize that invalid values won't do anything, but I think it's better
to only allow valid ranges.
> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
> +};
> +
>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			struct netlink_ext_ack *extack)
>  {
> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>  	struct net *net = sock_net(skb->sk);
> +	const struct net_device_ops *ops;
>  	struct ndmsg *ndm;
>  	struct nlattr *tb[NDA_MAX+1];
>  	struct net_device *dev;
> -	__u8 *addr;
> +	__u8 *addr = NULL;
>  	int err;
>  	u16 vid;
>  
>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
>  
> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
> -				     extack);
> +	if (!del_bulk) {
> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
> +					     NULL, extack);
> +	} else {
> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
> +				  fdb_del_bulk_policy, extack);
> +	}
>  	if (err < 0)
>  		return err;
>  
> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  		return -ENODEV;
>  	}
>  
> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> -		NL_SET_ERR_MSG(extack, "invalid address");
> -		return -EINVAL;
> +	if (!del_bulk) {
> +		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]);
>  	}
>  
>  	if (dev->type != ARPHRD_ETHER) {
> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  		return -EINVAL;
>  	}
>  
> -	addr = nla_data(tb[NDA_LLADDR]);
> -
>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>  	if (err)
>  		return err;
> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>  	    netif_is_bridge_port(dev)) {
>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>  
> -		if (ops->ndo_fdb_del)
> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = br_dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (err)
>  			goto out;
> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct
nlmsghdr *nlh,
>  
>  	/* Embedded bridge, macvlan, and any other device support */
>  	if (ndm->ndm_flags & NTF_SELF) {
> -		if (dev->netdev_ops->ndo_fdb_del)
> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
> -							   vid);
> -		else
> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +			else
> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			/* in case err was cleared by NTF_MASTER call */
> +			err = -EOPNOTSUPP;
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (!err) {
> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> -					ndm->ndm_state);
> +			if (!del_bulk)
> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> +						ndm->ndm_state);
>  			ndm->ndm_flags &= ~NTF_SELF;
>  		}
>  	}
> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>  
>  	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_DELNEIGH, rtnl_fdb_del, NULL,
> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>  
>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
> -- 
> 2.35.1
>

On 13/04/2022 15:20, Ido Schimmel wrote:
> On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
>> When NLM_F_BULK is specified in a fdb del message we need to handle it
>> differently. First since this is a new call we can strictly validate
the
>> passed attributes, at first only ifindex and vlan are allowed as these
>> will be the initially supported filter attributes, any other attribute
>> is rejected. The mac address is no longer mandatory, but we use it
>> to error out in older kernels because it cannot be specified with bulk
>> request (the attribute is not allowed) and then we have to dispatch
>> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
>> callback can do further validation of the attributes if necessary.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor at blackwall.org>
>> ---
>> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
>>
>>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 63c7df52a667..520d50fcaaea 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>>  }
>>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>>  
>> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
>> +	[NDA_VLAN]	= { .type = NLA_U16 },
> 
> In earlier versions br_vlan_valid_id() was used to validate the VLAN,
> but I don't see it anymore. Maybe use 
> 
> NLA_POLICY_RANGE(1, VLAN_N_VID - 2)
> 
> ?
> 
> I realize that invalid values won't do anything, but I think it's
better
> to only allow valid ranges.
> 
It's already validated below, see fdb_vid_parse().

>> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
>> +};
>> +
>>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  			struct netlink_ext_ack *extack)
>>  {
>> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>>  	struct net *net = sock_net(skb->sk);
>> +	const struct net_device_ops *ops;
>>  	struct ndmsg *ndm;
>>  	struct nlattr *tb[NDA_MAX+1];
>>  	struct net_device *dev;
>> -	__u8 *addr;
>> +	__u8 *addr = NULL;
>>  	int err;
>>  	u16 vid;
>>  
>>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>>  		return -EPERM;
>>  
>> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
>> -				     extack);
>> +	if (!del_bulk) {
>> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +					     NULL, extack);
>> +	} else {
>> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +				  fdb_del_bulk_policy, extack);
>> +	}
>>  	if (err < 0)
>>  		return err;
>>  
>> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb,
struct nlmsghdr *nlh,
>>  		return -ENODEV;
>>  	}
>>  
>> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
>> -		NL_SET_ERR_MSG(extack, "invalid address");
>> -		return -EINVAL;
>> +	if (!del_bulk) {
>> +		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]);
>>  	}
>>  
>>  	if (dev->type != ARPHRD_ETHER) {
>> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb,
struct nlmsghdr *nlh,
>>  		return -EINVAL;
>>  	}
>>  
>> -	addr = nla_data(tb[NDA_LLADDR]);
>> -
>>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>>  	if (err)
>>  		return err;
>> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb,
struct nlmsghdr *nlh,
>>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER)
&&
>>  	    netif_is_bridge_port(dev)) {
>>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
>> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>>  
>> -		if (ops->ndo_fdb_del)
>> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = br_dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (err)
>>  			goto out;
>> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb,
struct nlmsghdr *nlh,
>>  
>>  	/* Embedded bridge, macvlan, and any other device support */
>>  	if (ndm->ndm_flags & NTF_SELF) {
>> -		if (dev->netdev_ops->ndo_fdb_del)
>> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
>> -							   vid);
>> -		else
>> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +			else
>> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			/* in case err was cleared by NTF_MASTER call */
>> +			err = -EOPNOTSUPP;
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (!err) {
>> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> -					ndm->ndm_state);
>> +			if (!del_bulk)
>> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> +						ndm->ndm_state);
>>  			ndm->ndm_flags &= ~NTF_SELF;
>>  		}
>>  	}
>> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>>  
>>  	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_DELNEIGH, rtnl_fdb_del, NULL,
>> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump,
0);
>>  
>>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
>> -- 
>> 2.35.1
>>