Received: by 10.223.176.5 with SMTP id f5csp6632wra; Mon, 5 Feb 2018 15:25:48 -0800 (PST) X-Google-Smtp-Source: AH8x224DpgG70fZb50DJdx3JPN6Mc/1vSHFrUBdz3UhdmPLHODR1lXYdgPPh3M1F6At5hBprEWQR X-Received: by 10.101.90.193 with SMTP id d1mr337094pgt.366.1517873148346; Mon, 05 Feb 2018 15:25:48 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517873148; cv=none; d=google.com; s=arc-20160816; b=EuO5DgB4BOmMR6x8q/9Yv2f8gbN1FSYurErQbBq0b+3Ka/plAJvRJMaSSIe2+RO7Cf N8esdMXoep3+Os6TwdFrff+JRjo63ZkPnWOabggyUpGbHsgbxdopQR/Wi0xYENbS3GOD gGk2RpW+lx+7s/gCOEo1ky9KTnJxWuus/dPXej7BG+8y8zsxt0Ja4nQHXnX1606EE4T5 rOfnDw1jSRSX/WYiJwkhX3/M0EgAxMzaS9xRKW5QOXgYCxiKsV2rCucP6OJuUVED20Bb uFvFFeCTmE5SGkXu8eIfBa94SaIDziKd65nlkb0g8ltuwtNYifyJ7XotdQSbhmtFii4x jmTw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=rqgsfufyz8r6J7x54aOkc5EfCZzbq3OgQcS9YKgWbRc=; b=qwAu0RwH/NsBxqgligUb8w4c2H/kpaFmZL++S8B9663/i1SrYWS8eSMke+znsCdHNI 3VCzmSiam7RmrdtU0RZuihy1SVAUQjtNI2QM2y19pIMIOAn5E4f4ciul6nEb0FGBDY0H AI7DDB0wSwZ3+UpN7cDglV0HQylK/a2n/1+hQAntYQriHOnQ286gopISusX0rEilnc77 PmDZeZfxXgaZNSxWCRLGGRNPgBqaHUoYS+AwHbaNXgwG1kDI98JyHKBx+3hrSAKxj6nK Jf0veC/WfhJZx6GbnS8epnklmavJnQY4HSWcGK9eScF+kNlyzgZuoLvZMNq7cxo3WhGF 7uqw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p1si199099pfb.357.2018.02.05.15.25.33; Mon, 05 Feb 2018 15:25:48 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751956AbeBEXYv (ORCPT + 99 others); Mon, 5 Feb 2018 18:24:51 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:35557 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751774AbeBEXYn (ORCPT ); Mon, 5 Feb 2018 18:24:43 -0500 Received: from mail-wr0-f198.google.com ([209.85.128.198]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1eiq7t-0006EW-QS for linux-kernel@vger.kernel.org; Mon, 05 Feb 2018 23:24:41 +0000 Received: by mail-wr0-f198.google.com with SMTP id s18so34436wrg.5 for ; Mon, 05 Feb 2018 15:24:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=rqgsfufyz8r6J7x54aOkc5EfCZzbq3OgQcS9YKgWbRc=; b=kZrkDgqNqRuMkwIJXBUdmZU5QAe9NP1Ow55oN9Bica0jpqZsfWXq4gLhHLYa8bAm9D HhnopTxajNi43WzxfKmwOGPhRr07+IJOdJM0vSy3vUqOv7QD1bXkwfaZn0dgO8sRFrBd s6gsJyElvxRO3lV6DzIMgbepwXWXFwgQ68V3Q7tdB9nQKbPedmZJfsuyFicocWl98Sn4 yK/om7Ztb7b9TbSZIOOKLoGP3FhaB1xhbUVo12kvJEPtMty063rjYfl6hPrGjBPSJTWQ xJWjXmEeEGdcAHtqRVM2DmZXkRYBom/ckfpd+v646Vnrkuw+KUAPrQKPyQ/wqARJfRuV 8jUA== X-Gm-Message-State: APf1xPCp29lRcOELO5bbMiZEKrjNz3OMP0bUMgQedFanXu/YsQCl+P2A smkXFZ7XNv6RxV+yQ6c+3T/hfhPYsT8hwzzlTqf2L6u4nzmZ23GFgssYczUSAmxFAqHZQ/3HqXk DzWg/Mx6VoNgkl6SapxBLL0/LfLWpAjE/OCcuifVzCQ== X-Received: by 10.28.239.3 with SMTP id n3mr297816wmh.88.1517873081324; Mon, 05 Feb 2018 15:24:41 -0800 (PST) X-Received: by 10.28.239.3 with SMTP id n3mr297802wmh.88.1517873081029; Mon, 05 Feb 2018 15:24:41 -0800 (PST) Received: from gmail.com ([2a02:8070:8895:9700:f817:224e:7728:4274]) by smtp.gmail.com with ESMTPSA id n24sm10290098wmi.21.2018.02.05.15.24.39 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 05 Feb 2018 15:24:40 -0800 (PST) Date: Tue, 6 Feb 2018 00:24:39 +0100 From: Christian Brauner To: Kirill Tkhai Cc: Christian Brauner , netdev@vger.kernel.org, stephen@networkplumber.org, w.bumiller@proxmox.com, ebiederm@xmission.com, jbenc@redhat.com, nicolas.dichtel@6wind.com, linux-kernel@vger.kernel.org, dsahern@gmail.com, davem@davemloft.net Subject: Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier Message-ID: <20180205232438.GA8695@gmail.com> References: <20180205155550.21432-1-christian.brauner@ubuntu.com> <20180205155550.21432-2-christian.brauner@ubuntu.com> <2eac607b-e847-1b21-b3cb-6a45130138ee@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2eac607b-e847-1b21-b3cb-6a45130138ee@virtuozzo.com> User-Agent: Mutt/1.9.3 (2018-01-21) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 06, 2018 at 12:47:46AM +0300, Kirill Tkhai wrote: > On 05.02.2018 18:55, Christian Brauner wrote: > > Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK > > it is possible for userspace to send us requests with three different > > properties to identify a target network namespace. This affects at least > > RTM_{NEW,SET}LINK. Each of them could potentially refer to a different > > network namespace which is confusing. For legacy reasons the kernel will > > pick the IFLA_NET_NS_PID property first and then look for the > > IFLA_NET_NS_FD property but there is no reason to extend this type of > > behavior to network namespace ids. The regression potential is quite > > minimal since the rtnetlink requests in question either won't allow > > IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't > > support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place. > >> Signed-off-by: Christian Brauner > > --- > > ChangeLog v1->v2: > > * return errno when the specified network namespace id is invalid > > * fill in struct netlink_ext_ack if the network namespace id is invalid > > * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to > > indicate that a request without any network namespace identifying attributes > > is also considered valid. > > > > ChangeLog v0->v1: > > * report a descriptive error to userspace via struct netlink_ext_ack > > * do not fail when multiple properties specifiy the same network namespace > > --- > > net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > > index 56af8e41abfc..c096c4ff9a00 100644 > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb, > > return net; > > } > > > > +/* Verify that rtnetlink requests supporting network namespace ids > > + * do not pass additional properties referring to different network > > + * namespaces. > > + */ > > +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[], > > + struct netlink_ext_ack *extack) > > +{ > > + int ret = -EINVAL; > > + struct net *net = NULL, *unique_net = NULL; > > + > > + /* Requests without network namespace ids have been able to specify > > + * multiple properties referring to different network namespaces so > > + * don't regress them. > > + */ > > + if (!tb[IFLA_IF_NETNSID]) > > + return 0; > > + > > + /* Caller operates on the current network namespace. */ > > + if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD]) > > + return 0; > > + > > + unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID])); > > + if (!unique_net) { > > + NL_SET_ERR_MSG(extack, "invalid network namespace id"); > > + return ret; > > + } > > + > > + if (tb[IFLA_NET_NS_PID]) { > > + net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID])); > > + if (net != unique_net) > > + goto on_error; > > + } > > + > > + if (tb[IFLA_NET_NS_FD]) { > > + net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD])); > > + if (net != unique_net) > > + goto on_error; > > + } > > + > > + ret = 0; > > + > > +on_error: > > + put_net(unique_net); > > + > > + if (net && !IS_ERR(net)) > > + put_net(net); > > 1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing > to the same net, this function increments net::count in get_net_ns_by_pid() and > in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called. > So, after this function net::count will be incremented by 1, and it never will > die. Thanks for spotting this, Kirill. > > 2)The whole approach does not seem good for me. The first reason is it's racy. > Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that > tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later, > as the pid may die or do setns(). Racy check is worse than no check at all. > > The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/ > get_net_ns_by_fd() will be called twice: the first time is in your check > and the second time is where they are actually used. This is not good for > performance. If this is really a performance problem we can simply fix this by performing the check when the target network namespace is retrieved in each request. The intention for doing it in one function at the beginning of each request was to make it generic and easily understandable. > > What is the problem people pass several different tb[xxx] in one call? We > may just describe the order of tb[xxx] in man page and their priorities, > and ignore the rest after the first not zero tb[xxx] is found, and do that > in the place, where net from tb[xxx] in actually used. This is the thing > we already do. > > Comparing to classic Linux interface such as syscalls, it's usual behavior > for them to ignore one argument, when another is set. Nobody confuses. From what I gather from recent discussions I had here using pids and fds to perform operations on network namespaces in netlink requests is not the future. Specifically, using pids and fds will not be extended to existing or future requests that do not already support it. It also very much smells like a security liability if what you've outlined above is true: a user sends a request with a pid and the task dies and the pid gets recycled. Now, we can't easily fix this by simply ignoring pids and fds from here on since this would likely break a bunch of userspace programs but we can ensure that if a network namespace identifier is passed that no other way of retrieving the target network namespace is passed. Especially with requests that already support pids and fds. It's either that or reversing the order meaning that if a network namespace identifier is passed then it should take precedence over the other identifiers. Furthermore, this would also clearly indicate that netns ids are the preferred way to perform operations on network namespaces via netlink requests. I also do not think that your suggestion of making guarantees in what order additional netlink properties are evaluated is a good one. I don't think we want to give userspace the impression that sticking a pid, fd, and netnsid into the same netlink request is something that we actively support. What is certainly a good point is that if pids and fds are as you said inherently racy then we shouldn't perform the check but do what my original patch did and simply refuse to combine netns ids with pids and/or fds. Thanks for the comments! Christian