| Message ID | 20220704095845.365359-1-equinox@diac24.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: ip6mr: add RTM_GETROUTE netlink op | expand |
On 04/07/2022 12:58, David Lamparter wrote: > The IPv6 multicast routing code previously implemented only the dump > variant of RTM_GETROUTE. Implement single MFC item retrieval by copying > and adapting the respective IPv4 code. > > Tested against FRRouting's IPv6 PIM stack. > > Signed-off-by: David Lamparter <equinox@diac24.net> > Cc: David Ahern <dsahern@kernel.org> > --- > > v2: changeover to strict netlink attribute parsing. Doing so actually > exposed a bunch of other issues, first and foremost that rtm_ipv6_policy > does not have RTA_SRC or RTA_DST. This made reusing that policy rather > pointless so I changed it to use a separate rtm_ipv6_mr_policy. > > Thanks again dsahern@ for the feedback on the previous version! > > --- > net/ipv6/ip6mr.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 127 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c > index ec6e1509fc7c..95dc366a2d9b 100644 > --- a/net/ipv6/ip6mr.c > +++ b/net/ipv6/ip6mr.c > @@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, > static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc, > int cmd); > static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt); > +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > + struct netlink_ext_ack *extack); > static int ip6mr_rtm_dumproute(struct sk_buff *skb, > struct netlink_callback *cb); > static void mroute_clean_tables(struct mr_table *mrt, int flags); > @@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void) > } > #endif > err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE, > - NULL, ip6mr_rtm_dumproute, 0); > + ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0); > if (err == 0) > return 0; > > @@ -2510,6 +2512,130 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk > rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS); > } > > +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = { > + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC }, I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject it due to NL_VALIDATE_STRICT > + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), > + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), > + [RTA_TABLE] = { .type = NLA_U32 }, > +}; > + > +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb, > + const struct nlmsghdr *nlh, > + struct nlattr **tb, > + struct netlink_ext_ack *extack) > +{ > + struct rtmsg *rtm; > + int i, err; > + > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) { > + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request"); > + return -EINVAL; > + } I think you can drop this check if you... > + > + rtm = nlmsg_data(nlh); > + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || > + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || > + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || > + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { > + NL_SET_ERR_MSG(extack, > + "ipv6: Invalid values in header for multicast route get request"); > + return -EINVAL; > + } ...move these after nlmsg_parse() because it already does the hdrlen check for you > + > + err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy, > + extack); > + if (err) > + return err; > + > + if ((tb[RTA_SRC] && !rtm->rtm_src_len) || > + (tb[RTA_DST] && !rtm->rtm_dst_len)) { > + NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6"); > + return -EINVAL; > + } > + > + /* rtm_ipv6_mr_policy does not list other attributes right now, but > + * future changes may reuse rtm_ipv6_mr_policy with adding further > + * attrs. Enforce the subset. > + */ > + for (i = 0; i <= RTA_MAX; i++) { > + if (!tb[i]) > + continue; > + > + switch (i) { > + case RTA_SRC: > + case RTA_DST: > + case RTA_TABLE: > + break; > + default: > + NL_SET_ERR_MSG_ATTR(extack, tb[i], > + "ipv6: Unsupported attribute in multicast route get request"); > + return -EINVAL; > + } > + } I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC and you should get "Error: Unknown attribute type."). > + > + return 0; > +} > + > +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > + struct netlink_ext_ack *extack) > +{ > + struct net *net = sock_net(in_skb->sk); > + struct nlattr *tb[RTA_MAX + 1]; > + struct sk_buff *skb = NULL; > + struct mfc6_cache *cache; > + struct mr_table *mrt; > + struct in6_addr src = {}, grp = {}; reverse xmas tree order > + u32 tableid; > + int err; > + > + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); > + if (err < 0) > + goto errout; > + > + if (tb[RTA_SRC]) > + src = nla_get_in6_addr(tb[RTA_SRC]); > + if (tb[RTA_DST]) > + grp = nla_get_in6_addr(tb[RTA_DST]); > + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; > + > + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT); > + if (!mrt) { > + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist"); > + err = -ENOENT; > + goto errout_free; > + } > + > + /* entries are added/deleted only under RTNL */ > + rcu_read_lock(); > + cache = ip6mr_cache_find(mrt, &src, &grp); > + rcu_read_unlock(); > + if (!cache) { > + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found"); > + err = -ENOENT; > + goto errout_free; > + } > + > + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); > + if (!skb) { > + err = -ENOBUFS; > + goto errout_free; > + } > + > + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, > + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); > + if (err < 0) > + goto errout_free; > + > + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); > + > +errout: > + return err; > + > +errout_free: > + kfree_skb(skb); > + goto errout; > +} > + > static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb) > { > const struct nlmsghdr *nlh = cb->nlh;
On Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote: > On 04/07/2022 12:58, David Lamparter wrote: > > +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = { > > + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC }, > > I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject > it due to NL_VALIDATE_STRICT Will remove it. > > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) { > > + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request"); > > + return -EINVAL; > > + } > > I think you can drop this check if you... > > > + > > + rtm = nlmsg_data(nlh); > > + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || > > + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || > > + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || > > + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { > > + NL_SET_ERR_MSG(extack, > > + "ipv6: Invalid values in header for multicast route get request"); > > + return -EINVAL; > > + } > > ...move these after nlmsg_parse() because it already does the hdrlen > check for you Indeed it does. Moving it down. [...] > > + /* rtm_ipv6_mr_policy does not list other attributes right now, but > > + * future changes may reuse rtm_ipv6_mr_policy with adding further > > + * attrs. Enforce the subset. > > + */ > > + for (i = 0; i <= RTA_MAX; i++) { > > + if (!tb[i]) > > + continue; > > + > > + switch (i) { > > + case RTA_SRC: > > + case RTA_DST: > > + case RTA_TABLE: > > + break; > > + default: > > + NL_SET_ERR_MSG_ATTR(extack, tb[i], > > + "ipv6: Unsupported attribute in multicast route get request"); > > + return -EINVAL; > > + } > > + } > > I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that > don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC > and you should get "Error: Unknown attribute type."). I left it in with the comment above: > > + /* rtm_ipv6_mr_policy does not list other attributes right now, but > > + * future changes may reuse rtm_ipv6_mr_policy with adding further > > + * attrs. Enforce the subset. > > + */ ... to try and avoid silently starting to accept more attributes if/when future patches add other netlink operations reusing the same policy but with adding new attributes. But I don't feel particularly about this - shall I remove it? (just confirming with the rationale above) > > + struct net *net = sock_net(in_skb->sk); > > + struct nlattr *tb[RTA_MAX + 1]; > > + struct sk_buff *skb = NULL; > > + struct mfc6_cache *cache; > > + struct mr_table *mrt; > > + struct in6_addr src = {}, grp = {}; > > reverse xmas tree order Ah. Wasn't aware of that coding style aspect. Fixing. Thanks for the review! -David/equi
On 04/07/2022 13:38, David Lamparter wrote: > On Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote: >> On 04/07/2022 12:58, David Lamparter wrote: >>> +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = { >>> + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC }, >> >> I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject >> it due to NL_VALIDATE_STRICT > > Will remove it. > >>> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) { >>> + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request"); >>> + return -EINVAL; >>> + } >> >> I think you can drop this check if you... >> >>> + >>> + rtm = nlmsg_data(nlh); >>> + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || >>> + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || >>> + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || >>> + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { >>> + NL_SET_ERR_MSG(extack, >>> + "ipv6: Invalid values in header for multicast route get request"); >>> + return -EINVAL; >>> + } >> >> ...move these after nlmsg_parse() because it already does the hdrlen >> check for you > > Indeed it does. Moving it down. > > [...] >>> + /* rtm_ipv6_mr_policy does not list other attributes right now, but >>> + * future changes may reuse rtm_ipv6_mr_policy with adding further >>> + * attrs. Enforce the subset. >>> + */ >>> + for (i = 0; i <= RTA_MAX; i++) { >>> + if (!tb[i]) >>> + continue; >>> + >>> + switch (i) { >>> + case RTA_SRC: >>> + case RTA_DST: >>> + case RTA_TABLE: >>> + break; >>> + default: >>> + NL_SET_ERR_MSG_ATTR(extack, tb[i], >>> + "ipv6: Unsupported attribute in multicast route get request"); >>> + return -EINVAL; >>> + } >>> + } >> >> I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that >> don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC >> and you should get "Error: Unknown attribute type."). > > I left it in with the comment above: > >>> + /* rtm_ipv6_mr_policy does not list other attributes right now, but >>> + * future changes may reuse rtm_ipv6_mr_policy with adding further >>> + * attrs. Enforce the subset. >>> + */ > > ... to try and avoid silently starting to accept more attributes if/when > future patches add other netlink operations reusing the same policy but > with adding new attributes. > They really should be using policies specific to their actions with only the allowed attributes. Re-using this policy is ok only if those match, otherwise it's a bug IMO. > But I don't feel particularly about this - shall I remove it? (just > confirming with the rationale above) > I don't have a preference either, IMO if anyone re-uses this policy without making sure the same attributes and types are needed is adding buggy code. Actually the thing that I like about keeping the loop is the more specific error message, let's see what others think. >>> + struct net *net = sock_net(in_skb->sk); >>> + struct nlattr *tb[RTA_MAX + 1]; >>> + struct sk_buff *skb = NULL; >>> + struct mfc6_cache *cache; >>> + struct mr_table *mrt; >>> + struct in6_addr src = {}, grp = {}; >> >> reverse xmas tree order > > Ah. Wasn't aware of that coding style aspect. Fixing. > > Thanks for the review! > > > -David/equi
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lamparter/net-ip6mr-add-RTM_GETROUTE-netlink-op/20220704-180235
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d0bf1fe6454e976e39bc1524b9159fa2c0fcf321
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220705/202207051158.Vo7Qj6VM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/ab5f843c60bd5c7ef119d8be390e67f9c43d8d3b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Lamparter/net-ip6mr-add-RTM_GETROUTE-netlink-op/20220704-180235
git checkout ab5f843c60bd5c7ef119d8be390e67f9c43d8d3b
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash net/ipv6/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> net/ipv6/ip6mr.c:2515:25: sparse: sparse: symbol 'rtm_ipv6_mr_policy' was not declared. Should it be static?
net/ipv6/ip6mr.c:407:13: sparse: sparse: context imbalance in 'ip6mr_vif_seq_start' - different lock contexts for basic block
net/ipv6/ip6mr.c: note: in included file (through include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h, ...):
include/linux/rcupdate.h:726:9: sparse: sparse: context imbalance in 'ip6_mr_forward' - unexpected unlock
net/ipv6/ip6mr.c: note: in included file (through include/linux/mroute6.h):
include/linux/mroute_base.h:432:31: sparse: sparse: context imbalance in 'mr_mfc_seq_stop' - unexpected unlock
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index ec6e1509fc7c..95dc366a2d9b 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc, int cmd); static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt); +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, + struct netlink_ext_ack *extack); static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb); static void mroute_clean_tables(struct mr_table *mrt, int flags); @@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void) } #endif err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE, - NULL, ip6mr_rtm_dumproute, 0); + ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0); if (err == 0) return 0; @@ -2510,6 +2512,130 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS); } +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = { + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC }, + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), + [RTA_TABLE] = { .type = NLA_U32 }, +}; + +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb, + const struct nlmsghdr *nlh, + struct nlattr **tb, + struct netlink_ext_ack *extack) +{ + struct rtmsg *rtm; + int i, err; + + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) { + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request"); + return -EINVAL; + } + + rtm = nlmsg_data(nlh); + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) || + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) || + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol || + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) { + NL_SET_ERR_MSG(extack, + "ipv6: Invalid values in header for multicast route get request"); + return -EINVAL; + } + + err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy, + extack); + if (err) + return err; + + if ((tb[RTA_SRC] && !rtm->rtm_src_len) || + (tb[RTA_DST] && !rtm->rtm_dst_len)) { + NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6"); + return -EINVAL; + } + + /* rtm_ipv6_mr_policy does not list other attributes right now, but + * future changes may reuse rtm_ipv6_mr_policy with adding further + * attrs. Enforce the subset. + */ + for (i = 0; i <= RTA_MAX; i++) { + if (!tb[i]) + continue; + + switch (i) { + case RTA_SRC: + case RTA_DST: + case RTA_TABLE: + break; + default: + NL_SET_ERR_MSG_ATTR(extack, tb[i], + "ipv6: Unsupported attribute in multicast route get request"); + return -EINVAL; + } + } + + return 0; +} + +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, + struct netlink_ext_ack *extack) +{ + struct net *net = sock_net(in_skb->sk); + struct nlattr *tb[RTA_MAX + 1]; + struct sk_buff *skb = NULL; + struct mfc6_cache *cache; + struct mr_table *mrt; + struct in6_addr src = {}, grp = {}; + u32 tableid; + int err; + + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack); + if (err < 0) + goto errout; + + if (tb[RTA_SRC]) + src = nla_get_in6_addr(tb[RTA_SRC]); + if (tb[RTA_DST]) + grp = nla_get_in6_addr(tb[RTA_DST]); + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; + + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT); + if (!mrt) { + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist"); + err = -ENOENT; + goto errout_free; + } + + /* entries are added/deleted only under RTNL */ + rcu_read_lock(); + cache = ip6mr_cache_find(mrt, &src, &grp); + rcu_read_unlock(); + if (!cache) { + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found"); + err = -ENOENT; + goto errout_free; + } + + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); + if (!skb) { + err = -ENOBUFS; + goto errout_free; + } + + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); + if (err < 0) + goto errout_free; + + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid); + +errout: + return err; + +errout_free: + kfree_skb(skb); + goto errout; +} + static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb) { const struct nlmsghdr *nlh = cb->nlh;
The IPv6 multicast routing code previously implemented only the dump variant of RTM_GETROUTE. Implement single MFC item retrieval by copying and adapting the respective IPv4 code. Tested against FRRouting's IPv6 PIM stack. Signed-off-by: David Lamparter <equinox@diac24.net> Cc: David Ahern <dsahern@kernel.org> --- v2: changeover to strict netlink attribute parsing. Doing so actually exposed a bunch of other issues, first and foremost that rtm_ipv6_policy does not have RTA_SRC or RTA_DST. This made reusing that policy rather pointless so I changed it to use a separate rtm_ipv6_mr_policy. Thanks again dsahern@ for the feedback on the previous version! --- net/ipv6/ip6mr.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 127 insertions(+), 1 deletion(-)