| Message ID | 20200113155233.20771-4-nikolay@cumulusnetworks.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bridge: add vlan notifications and rtm support | expand |
On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: > +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) > +{ > + int idx = 0, err = 0, s_idx = cb->args[0]; > + struct net *net = sock_net(skb->sk); > + struct br_vlan_msg *bvm; > + struct net_device *dev; > + > + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { I wonder if it'd be useful to make this a strict != check? At least when strict validation is on? Perhaps we'll one day want to extend the request? > + NL_SET_ERR_MSG_MOD(cb->extack, "Invalid header for vlan dump request"); > + return -EINVAL; > + }
On 1/14/20 6:55 AM, Jakub Kicinski wrote: > On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >> +{ >> + int idx = 0, err = 0, s_idx = cb->args[0]; >> + struct net *net = sock_net(skb->sk); >> + struct br_vlan_msg *bvm; >> + struct net_device *dev; >> + >> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { > > I wonder if it'd be useful to make this a strict != check? At least > when strict validation is on? Perhaps we'll one day want to extend > the request? > +1. All new code should be using the strict checks.
On 14/01/2020 17:34, David Ahern wrote: > On 1/14/20 6:55 AM, Jakub Kicinski wrote: >> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>> +{ >>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>> + struct net *net = sock_net(skb->sk); >>> + struct br_vlan_msg *bvm; >>> + struct net_device *dev; >>> + >>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >> >> I wonder if it'd be useful to make this a strict != check? At least >> when strict validation is on? Perhaps we'll one day want to extend >> the request? >> > > +1. All new code should be using the strict checks. > IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() instead and all will be taken care of. I'll respin v2 with that change. Thanks, Nik
On 14/01/2020 18:36, Nikolay Aleksandrov wrote: > On 14/01/2020 17:34, David Ahern wrote: >> On 1/14/20 6:55 AM, Jakub Kicinski wrote: >>> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>>> +{ >>>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>>> + struct net *net = sock_net(skb->sk); >>>> + struct br_vlan_msg *bvm; >>>> + struct net_device *dev; >>>> + >>>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >>> >>> I wonder if it'd be useful to make this a strict != check? At least >>> when strict validation is on? Perhaps we'll one day want to extend >>> the request? >>> >> >> +1. All new code should be using the strict checks. >> > > IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() > instead and all will be taken care of. > I'll respin v2 with that change. > > Thanks, > Nik > Actually nlmsg_parse() uses the same "<" check for the size before parsing. :) If I change to it and with no attributes to parse would be essentially equal to the current situation, but if I make it strict "!=" then we won't be able to add filter attributes later as we won't be backwards compatible. I'll continue looking into it, but IMO we should leave it as it is in order to be able to add the filtering later. Thoughts ?
On 1/14/20 9:45 AM, Nikolay Aleksandrov wrote: > On 14/01/2020 18:36, Nikolay Aleksandrov wrote: >> On 14/01/2020 17:34, David Ahern wrote: >>> On 1/14/20 6:55 AM, Jakub Kicinski wrote: >>>> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>>>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>>>> +{ >>>>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>>>> + struct net *net = sock_net(skb->sk); >>>>> + struct br_vlan_msg *bvm; >>>>> + struct net_device *dev; >>>>> + >>>>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >>>> >>>> I wonder if it'd be useful to make this a strict != check? At least >>>> when strict validation is on? Perhaps we'll one day want to extend >>>> the request? >>>> >>> >>> +1. All new code should be using the strict checks. >>> >> >> IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() >> instead and all will be taken care of. >> I'll respin v2 with that change. >> >> Thanks, >> Nik >> > > Actually nlmsg_parse() uses the same "<" check for the size before parsing. :) > If I change to it and with no attributes to parse would be essentially equal to the > current situation, but if I make it strict "!=" then we won't be able to add > filter attributes later as we won't be backwards compatible. I'll continue looking > into it, but IMO we should leave it as it is in order to be able to add the filtering later. > > Thoughts ? > > > > If the header is > sizeof(*bvm) I expect this part of __nla_validate_parse() to kick in: if (unlikely(rem > 0)) { pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n", rem, current->comm); NL_SET_ERR_MSG(extack, "bytes leftover after parsing attributes"); if (validate & NL_VALIDATE_TRAILING) return -EINVAL; }
On 14/01/2020 18:49, David Ahern wrote: > On 1/14/20 9:45 AM, Nikolay Aleksandrov wrote: >> On 14/01/2020 18:36, Nikolay Aleksandrov wrote: >>> On 14/01/2020 17:34, David Ahern wrote: >>>> On 1/14/20 6:55 AM, Jakub Kicinski wrote: >>>>> On Mon, 13 Jan 2020 17:52:28 +0200, Nikolay Aleksandrov wrote: >>>>>> +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >>>>>> +{ >>>>>> + int idx = 0, err = 0, s_idx = cb->args[0]; >>>>>> + struct net *net = sock_net(skb->sk); >>>>>> + struct br_vlan_msg *bvm; >>>>>> + struct net_device *dev; >>>>>> + >>>>>> + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { >>>>> >>>>> I wonder if it'd be useful to make this a strict != check? At least >>>>> when strict validation is on? Perhaps we'll one day want to extend >>>>> the request? >>>>> >>>> >>>> +1. All new code should be using the strict checks. >>>> >>> >>> IIRC, I did it to be able to add filter attributes later, but it should just use nlmsg_parse() >>> instead and all will be taken care of. >>> I'll respin v2 with that change. >>> >>> Thanks, >>> Nik >>> >> >> Actually nlmsg_parse() uses the same "<" check for the size before parsing. :) >> If I change to it and with no attributes to parse would be essentially equal to the >> current situation, but if I make it strict "!=" then we won't be able to add >> filter attributes later as we won't be backwards compatible. I'll continue looking >> into it, but IMO we should leave it as it is in order to be able to add the filtering later. >> >> Thoughts ? >> >> >> >> > > If the header is > sizeof(*bvm) I expect this part of > __nla_validate_parse() to kick in: > > if (unlikely(rem > 0)) { > pr_warn_ratelimited("netlink: %d bytes leftover after > parsing attributes in process `%s'.\n", > rem, current->comm); > NL_SET_ERR_MSG(extack, "bytes leftover after parsing > attributes"); > if (validate & NL_VALIDATE_TRAILING) > return -EINVAL; > } > Ah fair enough, so nlmsg_parse() would be better even without attrs.
On 1/14/20 9:50 AM, Nikolay Aleksandrov wrote:
> Ah fair enough, so nlmsg_parse() would be better even without attrs.
that was the intention. It would be a good verification of the theory if
you could run a test with a larger ancillary header.
On 14/01/2020 18:53, David Ahern wrote: > On 1/14/20 9:50 AM, Nikolay Aleksandrov wrote: >> Ah fair enough, so nlmsg_parse() would be better even without attrs. > > that was the intention. It would be a good verification of the theory if > you could run a test with a larger ancillary header. > Just did, works like expected. Tested all sorts of wrong messages and attributes, they get properly validated and thrown out. Sending v2 in a minute. :) Thanks!
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h index 4a58e3d7de46..4da04f77d9ee 100644 --- a/include/uapi/linux/if_bridge.h +++ b/include/uapi/linux/if_bridge.h @@ -165,6 +165,34 @@ struct bridge_stp_xstats { __u64 tx_tcn; }; +/* Bridge vlan RTM header */ +struct br_vlan_msg { + __u8 family; + __u8 reserved1; + __u16 reserved2; + __u32 ifindex; +}; + +/* Bridge vlan RTM attributes + * [BRIDGE_VLANDB_ENTRY] = { + * [BRIDGE_VLANDB_ENTRY_INFO] + * ... + * } + */ +enum { + BRIDGE_VLANDB_UNSPEC, + BRIDGE_VLANDB_ENTRY, + __BRIDGE_VLANDB_MAX, +}; +#define BRIDGE_VLANDB_MAX (__BRIDGE_VLANDB_MAX - 1) + +enum { + BRIDGE_VLANDB_ENTRY_UNSPEC, + BRIDGE_VLANDB_ENTRY_INFO, + __BRIDGE_VLANDB_ENTRY_MAX, +}; +#define BRIDGE_VLANDB_ENTRY_MAX (__BRIDGE_VLANDB_ENTRY_MAX - 1) + /* Bridge multicast database attributes * [MDBA_MDB] = { * [MDBA_MDB_ENTRY] = { diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 1418a8362bb7..e06e3e09a1b4 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -171,6 +171,13 @@ enum { RTM_GETLINKPROP, #define RTM_GETLINKPROP RTM_GETLINKPROP + RTM_NEWVLAN = 112, +#define RTM_NEWNVLAN RTM_NEWVLAN + RTM_DELVLAN, +#define RTM_DELVLAN RTM_DELVLAN + RTM_GETVLAN, +#define RTM_GETVLAN RTM_GETVLAN + __RTM_MAX, #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1) }; diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 40942cece51a..75a7ecf95d7f 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -1657,6 +1657,7 @@ int __init br_netlink_init(void) int err; br_mdb_init(); + br_vlan_rtnl_init(); rtnl_af_register(&br_af_ops); err = rtnl_link_register(&br_link_ops); @@ -1674,6 +1675,7 @@ int __init br_netlink_init(void) void br_netlink_fini(void) { br_mdb_uninit(); + br_vlan_rtnl_uninit(); rtnl_af_unregister(&br_af_ops); rtnl_link_unregister(&br_link_ops); } diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index a7dddc5d7790..1c00411ae938 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -958,6 +958,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v, void br_vlan_port_event(struct net_bridge_port *p, unsigned long event); int br_vlan_bridge_event(struct net_device *dev, unsigned long event, void *ptr); +void br_vlan_rtnl_init(void); +void br_vlan_rtnl_uninit(void); static inline struct net_bridge_vlan_group *br_vlan_group( const struct net_bridge *br) @@ -1009,6 +1011,10 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg) return vg->pvid; } +static inline u16 br_vlan_flags(const struct net_bridge_vlan *v, u16 pvid) +{ + return v->vid == pvid ? v->flags | BRIDGE_VLAN_INFO_PVID : v->flags; +} #else static inline bool br_allowed_ingress(const struct net_bridge *br, struct net_bridge_vlan_group *vg, @@ -1152,6 +1158,14 @@ static inline int br_vlan_bridge_event(struct net_device *dev, { return 0; } + +static inline void br_vlan_rtnl_init(void) +{ +} + +static inline void br_vlan_rtnl_uninit(void) +{ +} #endif struct nf_br_ops { diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index bb98984cd27d..0135a67f50a7 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -1505,3 +1505,152 @@ void br_vlan_port_event(struct net_bridge_port *p, unsigned long event) break; } } + +static bool br_vlan_fill_vids(struct sk_buff *skb, u16 vid, u16 flags) +{ + struct bridge_vlan_info info; + struct nlattr *nest; + + nest = nla_nest_start(skb, BRIDGE_VLANDB_ENTRY); + if (!nest) + return false; + + memset(&info, 0, sizeof(info)); + info.vid = vid; + if (flags & BRIDGE_VLAN_INFO_UNTAGGED) + info.flags |= BRIDGE_VLAN_INFO_UNTAGGED; + if (flags & BRIDGE_VLAN_INFO_PVID) + info.flags |= BRIDGE_VLAN_INFO_PVID; + + if (nla_put(skb, BRIDGE_VLANDB_ENTRY_INFO, sizeof(info), &info)) + goto out_err; + + nla_nest_end(skb, nest); + + return true; + +out_err: + nla_nest_cancel(skb, nest); + return false; +} + +static int br_vlan_dump_dev(const struct net_device *dev, + struct sk_buff *skb, + struct netlink_callback *cb) +{ + struct net_bridge_vlan_group *vg; + int idx = 0, s_idx = cb->args[1]; + struct nlmsghdr *nlh = NULL; + struct net_bridge_vlan *v; + struct net_bridge_port *p; + struct br_vlan_msg *bvm; + struct net_bridge *br; + int err = 0; + u16 pvid; + + if (!netif_is_bridge_master(dev) && !netif_is_bridge_port(dev)) + return -EINVAL; + + if (netif_is_bridge_master(dev)) { + br = netdev_priv(dev); + vg = br_vlan_group_rcu(br); + p = NULL; + } else { + p = br_port_get_rcu(dev); + if (WARN_ON(!p)) + return -EINVAL; + vg = nbp_vlan_group_rcu(p); + br = p->br; + } + + if (!vg) + return 0; + + nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, + RTM_NEWVLAN, sizeof(*bvm), NLM_F_MULTI); + if (!nlh) + return -EMSGSIZE; + bvm = nlmsg_data(nlh); + memset(bvm, 0, sizeof(*bvm)); + bvm->family = PF_BRIDGE; + bvm->ifindex = dev->ifindex; + pvid = br_get_pvid(vg); + + list_for_each_entry_rcu(v, &vg->vlan_list, vlist) { + if (!br_vlan_should_use(v)) + continue; + if (idx < s_idx) + goto skip; + if (!br_vlan_fill_vids(skb, v->vid, br_vlan_flags(v, pvid))) { + err = -EMSGSIZE; + break; + } +skip: + idx++; + } + if (err) + cb->args[1] = idx; + else + cb->args[1] = 0; + nlmsg_end(skb, nlh); + + return err; +} + +static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) +{ + int idx = 0, err = 0, s_idx = cb->args[0]; + struct net *net = sock_net(skb->sk); + struct br_vlan_msg *bvm; + struct net_device *dev; + + if (cb->nlh->nlmsg_len < nlmsg_msg_size(sizeof(*bvm))) { + NL_SET_ERR_MSG_MOD(cb->extack, "Invalid header for vlan dump request"); + return -EINVAL; + } + + bvm = nlmsg_data(cb->nlh); + + rcu_read_lock(); + if (bvm->ifindex) { + dev = dev_get_by_index_rcu(net, bvm->ifindex); + if (!dev) { + err = -ENODEV; + goto out_err; + } + err = br_vlan_dump_dev(dev, skb, cb); + if (err && err != -EMSGSIZE) + goto out_err; + } else { + for_each_netdev_rcu(net, dev) { + if (idx < s_idx) + goto skip; + + err = br_vlan_dump_dev(dev, skb, cb); + if (err == -EMSGSIZE) + break; +skip: + idx++; + } + } + cb->args[0] = idx; + rcu_read_unlock(); + + return skb->len; + +out_err: + rcu_read_unlock(); + + return err; +} + +void br_vlan_rtnl_init(void) +{ + rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETVLAN, NULL, + br_vlan_rtm_dump, 0); +} + +void br_vlan_rtnl_uninit(void) +{ + rtnl_unregister(PF_BRIDGE, RTM_GETVLAN); +} diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index c97fdae8f71b..b69231918686 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -85,6 +85,9 @@ static const struct nlmsg_perm nlmsg_route_perms[] = { RTM_GETNEXTHOP, NETLINK_ROUTE_SOCKET__NLMSG_READ }, { RTM_NEWLINKPROP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, { RTM_DELLINKPROP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, + { RTM_NEWVLAN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, + { RTM_DELVLAN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, + { RTM_GETVLAN, NETLINK_ROUTE_SOCKET__NLMSG_READ }, }; static const struct nlmsg_perm nlmsg_tcpdiag_perms[] = @@ -168,7 +171,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) * structures at the top of this file with the new mappings * before updating the BUILD_BUG_ON() macro! */ - BUILD_BUG_ON(RTM_MAX != (RTM_NEWLINKPROP + 3)); + BUILD_BUG_ON(RTM_MAX != (RTM_NEWVLAN + 3)); err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, sizeof(nlmsg_route_perms)); break;
This patch adds vlan rtm definitions: - NEWVLAN: to be used for creating vlans, setting options and notifications - DELVLAN: to be used for deleting vlans - GETVLAN: used for dumping vlan information Dumping vlans which can span multiple messages is added now with basic information (vid and flags). Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- include/uapi/linux/if_bridge.h | 28 +++++++ include/uapi/linux/rtnetlink.h | 7 ++ net/bridge/br_netlink.c | 2 + net/bridge/br_private.h | 14 ++++ net/bridge/br_vlan.c | 149 +++++++++++++++++++++++++++++++++ security/selinux/nlmsgtab.c | 5 +- 6 files changed, 204 insertions(+), 1 deletion(-)