Messages in this thread | | | Subject | Re: [PATCH net-next v1 4/5] ipv6: enable IFA_IF_NETNSID for RTM_GETADDR | From | Kirill Tkhai <> | Date | Mon, 3 Sep 2018 17:56:56 +0300 |
| |
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
| |