Received: by 10.223.176.5 with SMTP id f5csp664581wra; Wed, 7 Feb 2018 05:37:22 -0800 (PST) X-Google-Smtp-Source: AH8x226pCdRnXFFPmFp95FzKmScbaofDPjZQT8UF5RIBIvd+0cmf9aTtK0TZlxM5bZXircabGVfy X-Received: by 10.98.14.208 with SMTP id 77mr5937971pfo.99.1518010642577; Wed, 07 Feb 2018 05:37:22 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518010642; cv=none; d=google.com; s=arc-20160816; b=xLt4qD49cbedkF0WNGxkyeswPRsrX39gEXzId23I6+1inQWCpLRUg2YXA+VnfkqGek 3v5s0j9bHpP5QrmFpInlmPn0MNmb2u2mmcGgoQ85YUuVTxjfnOO2Freq0p3HxR5cxZd2 CNZWoEFMwYHuEgKQJV2ERg9B+lhXlQ7fFoEfO/NVrDcpbZoATyXoGSnFXZE8lE3MBS05 pQo8JdErhoFiPJJJGYuzzKnMbE7F7AFybfr9L8UEC7X3wJclti8sMYJpfu+FQUJzCmt5 DTwzRRGtqRJvX86rGCokB+8k6oqNm57e+bPAnwPmKXyE1MWMx/olM8yP7y5JCsya5X6U Q7Qg== 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=04VqCs9hcfHANKTpOB34feySTcU5oydaAtMb3DhGfso=; b=j6+6f9bpGbUzopvPh2xqLIW3gPzKHUj8uTShdxuPDTpT1zbukEQD2VWP4UaZjUNJ82 4KAMZRHckCl099H3JD2mowpy60DRj2W+1g3nnVe/9Y7UvCcpG2U7jmtnZUEz0OFVXzv0 y4s38sHAWK3++dTg8p2rJzwk02XrD8reSQua3/f2E6y9wPh6Ca+EzViYY4S+BE1nIjV5 btanx4fImPNJENEH79VoT/wRCYmNN1Tj9r59xAiMpBadUCsiAip3tAntPwOL+ZIQm05w HAkIOwtQvtM2PT4TwvT6zzElff/JfTCUCFMsUxwHzQ015jTKfrYxgCvnoIcrl1/Pbm1V BY5A== 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 z5-v6si1075247pln.677.2018.02.07.05.37.08; Wed, 07 Feb 2018 05:37:22 -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 S1754008AbeBGNg0 (ORCPT + 99 others); Wed, 7 Feb 2018 08:36:26 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:42689 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753643AbeBGNgY (ORCPT ); Wed, 7 Feb 2018 08:36:24 -0500 Received: from mail-wm0-f72.google.com ([74.125.82.72]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1ejPtf-0003Ke-Gh for linux-kernel@vger.kernel.org; Wed, 07 Feb 2018 13:36:23 +0000 Received: by mail-wm0-f72.google.com with SMTP id v14so815898wmd.3 for ; Wed, 07 Feb 2018 05:36:23 -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=04VqCs9hcfHANKTpOB34feySTcU5oydaAtMb3DhGfso=; b=CkqEJrzngbHKGrYQSYg7Lxx6cObkKRSmizporI9RBcplvCGl5kvAQXsyjLUzVBzW75 YSm9XZZiXUXTTz73V2RnmjbZT6QQIpbJkiQogjdgFufIeAEyEG8Z1zCF0m02Q86FEQ7H FNyt2TZ9CL9F3M84D0PnvULfwo910i7R34S5MUJo3gx9/XRHQ4r42tQ+QWoWuuX3S7eL 4wlldb7hiMh1kruK5jcD1hapDDQ+Ar0aOa25VSF6QKR0LADYTfN+StL+Y1iV8elvD760 sAoBtWnr8Je94yfEo83OGBRvZBgw470yIHm98rYaCmKAEpdceZG4Ud8J6VAm8VS6YzCq e+cg== X-Gm-Message-State: APf1xPDi4JPUFZtz5aSXXmvSNOEdxOT7nQP8VM2I65N6dpkHleTSZZ+T /YctKhPv2/gVmg4AxgaYlYT+MGyADxlxb0R1A2KOoYGk2vfZD6EtSQPWSUIGTIpWcGB7yJrKE1J PU2AaEkcjebjiFDUS397Ej6zKSPGzSDQAgl0sRhxxYg== X-Received: by 10.28.37.5 with SMTP id l5mr4931987wml.133.1518010582941; Wed, 07 Feb 2018 05:36:22 -0800 (PST) X-Received: by 10.28.37.5 with SMTP id l5mr4931961wml.133.1518010582624; Wed, 07 Feb 2018 05:36:22 -0800 (PST) Received: from gmail.com (eap108072.extern.uni-tuebingen.de. [134.2.108.72]) by smtp.gmail.com with ESMTPSA id a6sm3991863wri.10.2018.02.07.05.36.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 07 Feb 2018 05:36:21 -0800 (PST) Date: Wed, 7 Feb 2018 14:36:21 +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 v4] rtnetlink: require unique netns identifier Message-ID: <20180207133620.GA3589@gmail.com> References: <20180207125320.9103-1-christian.brauner@ubuntu.com> <20180207125320.9103-2-christian.brauner@ubuntu.com> <942f99e9-086d-25dc-e008-8de2c5c721b1@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <942f99e9-086d-25dc-e008-8de2c5c721b1@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 Wed, Feb 07, 2018 at 04:20:01PM +0300, Kirill Tkhai wrote: > > > On 07.02.2018 15:53, 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 v3->v4: > > * Based on discussions with Eric and Jiri: disallow passing multiple network > > namespace identifying properties for all requests, i.e. always enforce > > uniqueness. > > * disable passing IFLA_NET_NS_{FD,PID} for RTM_{DEL,GET}LINK completely since > > they never supported it > > ChangeLog v2->v3: > > * Specifying target network namespaces with pids or fds seems racy since the > > process might die and the pid get recycled or the process does a setns() in > > which case the tests would be invalid. So only check whether multiple > > properties are specified and report a helpful error in this case. > > 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > > index 56af8e41abfc..bc290413a49d 100644 > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -1951,6 +1951,38 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb, > > return net; > > } > > > > +/* Verify that rtnetlink requests do not pass additional properties > > + * potentially referring to different network namespaces. > > + */ > > +static int rtnl_ensure_unique_netns(struct nlattr *tb[], > > + struct netlink_ext_ack *extack, > > + bool netns_id_only) > > +{ > > + > > + if (netns_id_only) { > > + if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD]) > > + return 0; > > + > > + NL_SET_ERR_MSG(extack, "specified netns attribute not supported"); > > + return -EOPNOTSUPP; > > + } > > + > > + if (tb[IFLA_IF_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD])) > > + goto invalid_attr; > > + > > + if (tb[IFLA_NET_NS_PID] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_FD])) > > + goto invalid_attr; > > + > > + if (tb[IFLA_NET_NS_FD] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_PID])) > > + goto invalid_attr; > > Can't we write these 3 above branches more compact? Something like this: > > if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1) > return 0; I always prefer for conditions to be separate and readable even if it means additional code. But if others feel that there's value in avoiding two additional conditions I'm happy to adapt the patch. > > Also, do we really need two different error values and error messages? Before I added support for netns ids for additional requests Jiri made it so that all request that specified properties that they did not support returned ENOTSUPP instead of EINVAL. This just keeps things consistent. Users would now suddenly receive EINVAL. That's potentially confusing. As for the case of passing multiple netns identifying properties into the same request EINVAL seems the perfect candidate and the error message seems instructive to userspace programs. Thanks! Christian