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

[David Lamparter <equinox@diac24.net>]

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