| Message ID | 20220413105202.2616106-8-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9e83425993f38bb89e0ea07849ba0039a748e85b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: add flush filtering support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | success | Patch already applied to net-next |
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@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@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 >>
On Wed, Apr 13, 2022 at 03:21:54PM +0300, Nikolay Aleksandrov wrote: > 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@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(). Sorry, missed it :)
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 }, + [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);
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@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(-)