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

From: Kirill Tkhai
Date: Mon Sep 03 2018 - 10:57:12 EST

Next message: tip-bot for Juergen Gross: "[tip:x86/paravirt] x86/paravirt: Make paravirt_patch_call() and paravirt_patch_jmp() static"
Previous message: tip-bot for Juergen Gross: "[tip:x86/paravirt] x86/xen: Add SPDX identifier in arch/x86/xen files"
In reply to: Christian Brauner: "[PATCH net-next v1 4/5] ipv6: enable IFA_IF_NETNSID for RTM_GETADDR"
Next in thread: Christian Brauner: "[PATCH net-next v1 5/5] rtnetlink: move type calculation out of loop"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
> 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

Next message: tip-bot for Juergen Gross: "[tip:x86/paravirt] x86/paravirt: Make paravirt_patch_call() and paravirt_patch_jmp() static"
Previous message: tip-bot for Juergen Gross: "[tip:x86/paravirt] x86/xen: Add SPDX identifier in arch/x86/xen files"
In reply to: Christian Brauner: "[PATCH net-next v1 4/5] ipv6: enable IFA_IF_NETNSID for RTM_GETADDR"
Next in thread: Christian Brauner: "[PATCH net-next v1 5/5] rtnetlink: move type calculation out of loop"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]