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@xxxxxxxxxxxxx>
>> ---
>> 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
>>
