[PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

[PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

[David Lamparter]

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>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Reviewed-by: David Ahern <dsahern@kernel.org>
---

v5: consistently use NL_SET_ERR_MSG_MOD for "ipv6: " prefix

v4: rename policy to indicate it is dedicated for getroute, remove
extra validation loop to reject attrs, and add missing "static".

v3: reorder/remove some redundant bits, fix style.  Thanks Nikolay for
pointing them out.  The "extra" validation loop is still there for the
time being;  happy to drop it if that's the consensus.

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 | 102 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ec6e1509fc7c..4aee3d8a6103 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,104 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
 	rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
 }
 
+static const struct nla_policy ip6mr_getroute_policy[RTA_MAX + 1] = {
+	[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 err;
+
+	err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, ip6mr_getroute_policy,
+			  extack);
+	if (err)
+		return err;
+
+	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_MOD(extack,
+				   "Invalid values in header for multicast route get request");
+		return -EINVAL;
+	}
+
+	if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
+	    (tb[RTA_DST] && !rtm->rtm_dst_len)) {
+		NL_SET_ERR_MSG_MOD(extack, "rtm_src_len and rtm_dst_len must be 128 for IPv6");
+		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 in6_addr src = {}, grp = {};
+	struct nlattr *tb[RTA_MAX + 1];
+	struct sk_buff *skb = NULL;
+	struct mfc6_cache *cache;
+	struct mr_table *mrt;
+	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, "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, "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;
-- 
2.36.1

Re: [PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

[Jakub Kicinski]

Few more nit picks, sorry..

On Thu,  7 Jul 2022 11:33:36 +0200 David Lamparter wrote:
> +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 in6_addr src = {}, grp = {};
> +	struct nlattr *tb[RTA_MAX + 1];
> +	struct sk_buff *skb = NULL;

Should be unnecessary if the code is right, let the compiler warn us
about uninitialized variables.

> +	struct mfc6_cache *cache;
> +	struct mr_table *mrt;
> +	u32 tableid;
> +	int err;
> +
> +	err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
> +	if (err < 0)
> +		goto errout;

Can we:

		return err;

? I don't know where the preference for jumping to the return statement
came from, old compilers? someone's "gut feeling"?

> +	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);

	tableid ? : RT_TABLE_DEFAULT

or

	tableid ?: RT_TABLE_DEFAULT

the abbreviated version of the ternary operator is used quite commonly
in the kernel.

> +	if (!mrt) {
> +		NL_SET_ERR_MSG_MOD(extack, "MR table does not exist");
> +		err = -ENOENT;
> +		goto errout_free;

Ditto, just return, if not goto errout; there's nothing to 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, "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;

now this is the only case which actually needs to free the skb

> +
> +	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +
> +errout:
> +	return err;

when the label is gone you can:

	return rtnl_unicast(...

directly.

> +
> +errout_free:
> +	kfree_skb(skb);
> +	goto errout;

and no need to do the funky backwards jump here either, IMO

> +}

Re: [PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

[David Lamparter]

On Fri, Jul 08, 2022 at 08:29:51PM -0700, Jakub Kicinski wrote:
> Few more nit picks, sorry..

Thanks for the feedback!  [opens editor]

> On Thu,  7 Jul 2022 11:33:36 +0200 David Lamparter wrote:
[...]
> > +	err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
> > +	if (err < 0)
> > +		goto errout;
> 
> Can we:
> 
> 		return err;
> 
> ? I don't know where the preference for jumping to the return statement
> came from, old compilers? someone's "gut feeling"?

If I were forced to find a justification, I'd say having a central
sequence of exit helps avoiding mistakes when some other resource
acquisition is added later.  Easy to add a cleanup call to an existing
cleanup block - easy to overlook a "return err;" that needs to be
changed to "goto errout;".

But I have absolutely no stake in this at all, I'll happily edit it to
whatever the consensus is.  This is just what the IPv4 code looks like
after being adapted for IPv6.

> > +errout:
> > +	return err;
[...]
> > +
> > +errout_free:
> > +	kfree_skb(skb);
> > +	goto errout;
> 
> and no need to do the funky backwards jump here either, IMO

"funky" is a nice description.


-equi/David

Re: [PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

[Jakub Kicinski]

On Sat, 9 Jul 2022 14:45:00 +0200 David Lamparter wrote:
> > > +	err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
> > > +	if (err < 0)
> > > +		goto errout;  
> > 
> > Can we:
> > 
> > 		return err;
> > 
> > ? I don't know where the preference for jumping to the return statement
> > came from, old compilers? someone's "gut feeling"?  
> 
> If I were forced to find a justification, I'd say having a central
> sequence of exit helps avoiding mistakes when some other resource
> acquisition is added later.  Easy to add a cleanup call to an existing
> cleanup block - easy to overlook a "return err;" that needs to be
> changed to "goto errout;".

That only works if the label's name is meaningless, if the label is
named after what it points to you have to rename the label and all the
jumps anyway. Can as well replace returns with a goto.

> But I have absolutely no stake in this at all, I'll happily edit it to
> whatever the consensus is.  This is just what the IPv4 code looks like
> after being adapted for IPv6.

Ah, I looked around other getroute implementations but not specifically
ipmr. I'd rather refactor ipmr.c as well than keep its strangeness.
The fact that we jump to the error path which tries to free the skb
without ever allocating the skb feels particularly off.