Ido Schimmel
2022-Apr-13 12:20 UTC
[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 >
Nikolay Aleksandrov
2022-Apr-13 12:21 UTC
[Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
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 >>