Re: [PATCH net-next v1 4/5] ipv6: enable IFA_IF_NETNSID for RTM_GETADDR

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 03.09.2018 07:37, Christian Brauner wrote:
> - Backwards Compatibility:
>   If userspace wants to determine whether ipv6 RTM_GETADDR requests support
>   the new IFA_IF_NETNSID property they should verify that the reply after
>   sending a request includes the IFA_IF_NETNSID property. If it does not
>   userspace should assume that IFA_IF_NETNSID is not supported for ipv6
>   RTM_GETADDR requests on this kernel.
> - From what I gather from current userspace tools that make use of
>   RTM_GETADDR requests some of them pass down struct ifinfomsg when they
>   should actually pass down struct ifaddrmsg. To not break existing
>   tools that pass down the wrong struct we will do the same as for
>   RTM_GETLINK | NLM_F_DUMP requests and not error out when the
>   nlmsg_parse() fails.
> 
> - Security:
>   Callers must have CAP_NET_ADMIN in the owning user namespace of the
>   target network namespace.
> 
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v0->v1:
> - unchanged
> ---
>  net/ipv6/addrconf.c | 70 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index d51a8c0b3372..00f1af374150 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4491,6 +4491,7 @@ static const struct nla_policy ifa_ipv6_policy[IFA_MAX+1] = {
>  	[IFA_CACHEINFO]		= { .len = sizeof(struct ifa_cacheinfo) },
>  	[IFA_FLAGS]		= { .len = sizeof(u32) },
>  	[IFA_RT_PRIORITY]	= { .len = sizeof(u32) },
> +	[IFA_IF_NETNSID]	= { .type = NLA_S32 },
>  };
>  
>  static int
> @@ -4794,7 +4795,8 @@ static inline int inet6_ifaddr_msgsize(void)
>  }
>  
>  static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
> -			     u32 portid, u32 seq, int event, unsigned int flags)
> +			     u32 portid, u32 seq, int event, unsigned int flags,
> +			     int netnsid)
>  {
>  	struct nlmsghdr  *nlh;
>  	u32 preferred, valid;
> @@ -4806,6 +4808,9 @@ static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
>  	put_ifaddrmsg(nlh, ifa->prefix_len, ifa->flags, rt_scope(ifa->scope),
>  		      ifa->idev->dev->ifindex);
>  
> +	if (netnsid >= 0 && nla_put_s32(skb, IFA_IF_NETNSID, netnsid))
> +		goto error;
> +
>  	if (!((ifa->flags&IFA_F_PERMANENT) &&
>  	      (ifa->prefered_lft == INFINITY_LIFE_TIME))) {
>  		preferred = ifa->prefered_lft;
> @@ -4855,7 +4860,8 @@ static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
>  }
>  
>  static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct ifmcaddr6 *ifmca,
> -				u32 portid, u32 seq, int event, u16 flags)
> +			       u32 portid, u32 seq, int event, u16 flags,
> +			       int netnsid)
>  {
>  	struct nlmsghdr  *nlh;
>  	u8 scope = RT_SCOPE_UNIVERSE;
> @@ -4868,6 +4874,9 @@ static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct ifmcaddr6 *ifmca,
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> +	if (netnsid >= 0 && nla_put_s32(skb, IFA_IF_NETNSID, netnsid))
> +		return -EMSGSIZE;
> +
>  	put_ifaddrmsg(nlh, 128, IFA_F_PERMANENT, scope, ifindex);
>  	if (nla_put_in6_addr(skb, IFA_MULTICAST, &ifmca->mca_addr) < 0 ||
>  	    put_cacheinfo(skb, ifmca->mca_cstamp, ifmca->mca_tstamp,
> @@ -4881,7 +4890,8 @@ static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct ifmcaddr6 *ifmca,
>  }
>  
>  static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
> -				u32 portid, u32 seq, int event, unsigned int flags)
> +			       u32 portid, u32 seq, int event,
> +			       unsigned int flags, int netnsid)

Here we will have 7 arguments, while we have only 6 x86 registers. Can we do something
with this?

>  {
>  	struct net_device *dev = fib6_info_nh_dev(ifaca->aca_rt);
>  	int ifindex = dev ? dev->ifindex : 1;
> @@ -4895,6 +4905,9 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> +	if (netnsid >= 0 && nla_put_s32(skb, IFA_IF_NETNSID, netnsid))
> +		return -EMSGSIZE;
> +
>  	put_ifaddrmsg(nlh, 128, IFA_F_PERMANENT, scope, ifindex);
>  	if (nla_put_in6_addr(skb, IFA_ANYCAST, &ifaca->aca_addr) < 0 ||
>  	    put_cacheinfo(skb, ifaca->aca_cstamp, ifaca->aca_tstamp,
> @@ -4916,7 +4929,7 @@ enum addr_type_t {
>  /* called with rcu_read_lock() */
>  static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  			  struct netlink_callback *cb, enum addr_type_t type,
> -			  int s_ip_idx, int *p_ip_idx)
> +			  int s_ip_idx, int *p_ip_idx, int netnsid)
>  {
>  	struct ifmcaddr6 *ifmca;
>  	struct ifacaddr6 *ifaca;
> @@ -4936,7 +4949,7 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  						NETLINK_CB(cb->skb).portid,
>  						cb->nlh->nlmsg_seq,
>  						RTM_NEWADDR,
> -						NLM_F_MULTI);
> +						NLM_F_MULTI, netnsid);
>  			if (err < 0)
>  				break;
>  			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
> @@ -4953,7 +4966,7 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  						  NETLINK_CB(cb->skb).portid,
>  						  cb->nlh->nlmsg_seq,
>  						  RTM_GETMULTICAST,
> -						  NLM_F_MULTI);
> +						  NLM_F_MULTI, netnsid);
>  			if (err < 0)
>  				break;
>  		}
> @@ -4968,7 +4981,7 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  						  NETLINK_CB(cb->skb).portid,
>  						  cb->nlh->nlmsg_seq,
>  						  RTM_GETANYCAST,
> -						  NLM_F_MULTI);
> +						  NLM_F_MULTI, netnsid);
>  			if (err < 0)
>  				break;
>  		}
> @@ -4985,6 +4998,9 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			   enum addr_type_t type)
>  {
>  	struct net *net = sock_net(skb->sk);
> +	struct nlattr *tb[IFA_MAX+1];
> +	struct net *tgt_net = net;
> +	int netnsid = -1;

This function already has 10 local variables, while this patch adds 3 more.
Documentation/process/coding-style.rst says:

   Another measure of the function is the number of local variables.  They
   shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
   function, and split it into smaller pieces.  A human brain can
   generally easily keep track of about 7 different things, anything more
   and it gets confused.

Can we do something with this?

>  	int h, s_h;
>  	int idx, ip_idx;
>  	int s_idx, s_ip_idx;
> @@ -4996,11 +5012,22 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	s_idx = idx = cb->args[1];
>  	s_ip_idx = ip_idx = cb->args[2];
>  
> +	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> +			ifa_ipv6_policy, NULL) >= 0) {
> +		if (tb[IFA_IF_NETNSID]) {
> +			netnsid = nla_get_s32(tb[IFA_IF_NETNSID]);
> +
> +			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> +			if (IS_ERR(tgt_net))
> +				return PTR_ERR(tgt_net);
> +		}
> +	}
> +
>  	rcu_read_lock();
> -	cb->seq = atomic_read(&net->ipv6.dev_addr_genid) ^ net->dev_base_seq;
> +	cb->seq = atomic_read(&tgt_net->ipv6.dev_addr_genid) ^ tgt_net->dev_base_seq;
>  	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
>  		idx = 0;
> -		head = &net->dev_index_head[h];
> +		head = &tgt_net->dev_index_head[h];
>  		hlist_for_each_entry_rcu(dev, head, index_hlist) {
>  			if (idx < s_idx)
>  				goto cont;
> @@ -5012,7 +5039,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  				goto cont;
>  
>  			if (in6_dump_addrs(idev, skb, cb, type,
> -					   s_ip_idx, &ip_idx) < 0)
> +					   s_ip_idx, &ip_idx, netnsid) < 0)
>  				goto done;
>  cont:
>  			idx++;
> @@ -5023,6 +5050,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	cb->args[0] = h;
>  	cb->args[1] = idx;
>  	cb->args[2] = ip_idx;
> +	if (netnsid >= 0)
> +		put_net(tgt_net);
>  
>  	return skb->len;
>  }
> @@ -5053,12 +5082,14 @@ static int inet6_rtm_getaddr(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  			     struct netlink_ext_ack *extack)
>  {
>  	struct net *net = sock_net(in_skb->sk);
> +	struct net *tgt_net = net;
>  	struct ifaddrmsg *ifm;
>  	struct nlattr *tb[IFA_MAX+1];
>  	struct in6_addr *addr = NULL, *peer;
>  	struct net_device *dev = NULL;
>  	struct inet6_ifaddr *ifa;
>  	struct sk_buff *skb;
> +	int netnsid = -1;
>  	int err;
>  
>  	err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFA_MAX, ifa_ipv6_policy,
> @@ -5066,15 +5097,24 @@ static int inet6_rtm_getaddr(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  	if (err < 0)
>  		return err;
>  
> +	if (tb[IFA_IF_NETNSID]) {
> +		netnsid = nla_get_s32(tb[IFA_IF_NETNSID]);
> +
> +		tgt_net = rtnl_get_net_ns_capable(NETLINK_CB(in_skb).sk,
> +						  netnsid);
> +		if (IS_ERR(tgt_net))
> +			return PTR_ERR(tgt_net);
> +	}
> +
>  	addr = extract_addr(tb[IFA_ADDRESS], tb[IFA_LOCAL], &peer);
>  	if (!addr)
>  		return -EINVAL;
>  
>  	ifm = nlmsg_data(nlh);
>  	if (ifm->ifa_index)
> -		dev = dev_get_by_index(net, ifm->ifa_index);
> +		dev = dev_get_by_index(tgt_net, ifm->ifa_index);
>  
> -	ifa = ipv6_get_ifaddr(net, addr, dev, 1);
> +	ifa = ipv6_get_ifaddr(tgt_net, addr, dev, 1);
>  	if (!ifa) {
>  		err = -EADDRNOTAVAIL;
>  		goto errout;
> @@ -5087,14 +5127,14 @@ static int inet6_rtm_getaddr(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  	}
>  
>  	err = inet6_fill_ifaddr(skb, ifa, NETLINK_CB(in_skb).portid,
> -				nlh->nlmsg_seq, RTM_NEWADDR, 0);
> +				nlh->nlmsg_seq, RTM_NEWADDR, 0, netnsid);
>  	if (err < 0) {
>  		/* -EMSGSIZE implies BUG in inet6_ifaddr_msgsize() */
>  		WARN_ON(err == -EMSGSIZE);
>  		kfree_skb(skb);
>  		goto errout_ifa;
>  	}
> -	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +	err = rtnl_unicast(skb, tgt_net, NETLINK_CB(in_skb).portid);
>  errout_ifa:
>  	in6_ifa_put(ifa);
>  errout:
> @@ -5113,7 +5153,7 @@ static void inet6_ifa_notify(int event, struct inet6_ifaddr *ifa)
>  	if (!skb)
>  		goto errout;
>  
> -	err = inet6_fill_ifaddr(skb, ifa, 0, 0, event, 0);
> +	err = inet6_fill_ifaddr(skb, ifa, 0, 0, event, 0, -1);
>  	if (err < 0) {
>  		/* -EMSGSIZE implies BUG in inet6_ifaddr_msgsize() */
>  		WARN_ON(err == -EMSGSIZE);

Thanks,
Kirill