Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp3104384pxj; Mon, 7 Jun 2021 02:19:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw0hLcHgk/TK7PUAywhqErpanoXV30hCBS0MWc4Kh2bDg3uCQj3CwqbuQ04kvxGuldA6zFe X-Received: by 2002:a05:6402:152:: with SMTP id s18mr18495162edu.221.1623057548357; Mon, 07 Jun 2021 02:19:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623057548; cv=none; d=google.com; s=arc-20160816; b=ZMlStFsMYt2TVAEXVDDbV/2Z+y2P8u5bxWT0RmeUgnAeiG9FgegFq530DsFTsXxbkl ABC6Me6vGEnr8aw3mfGiWlO11pipdoyVHEJOt8BsgV+QEXlcGXPO5TtCGvIzTyHN6wUl qk9p5wf6lh92jb+sNogi/SRU8FuCdj5huZOklg0gPxFlVtkKX363Nb3T3tnfmv21Xl74 ZRBh/cstkBMa1UeVhRy4jhnST+MLjjouh/DfRrU3VYkoXc9mtkwyn9QWD4JNHFi2GVk2 gruYygbabVuSx5EBu+vBiVK+sZUFgDViJkuyA9ew7VqKpGUMJAhKRNLp/v/km38Fzym9 st8Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=7xZ6N4Z84C19yNJ9q6+6nEW7tzE0IG83LWIvEfgSUVU=; b=KIVXuywf0PPbtIGhR8Cb+J8KNjS7QrAbxdGI3b5l7Ax+06VwBBSpXdNK753Daam5KO INdn00sW1mfJFD04lnIZ35HsPHbLnTjMY1Y+Ju8nvwXSsp9fbunzjBsPkMwAVtp7IkSi knE3iN726YiszObxepvOp5Wh14ig7E+lHTeas4CL+RrMEAutk6Mkq3W85iZOGPuUya0I IjN0vvwhOsvVCvmQ6wBYqRg4S1tv/hDq5ylgHJCYrmGchYsr4Gq95a4cfxYtPSxNNqpf 7I89f2GPzbP/FM/BlQOTFZ7FYPA3inz2sIhaRQ7u+eb8XBZJOxIIJZQ/oS759sr9A9a9 zBjA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id f13si12100011edy.493.2021.06.07.02.18.46; Mon, 07 Jun 2021 02:19:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230294AbhFGJSu (ORCPT + 99 others); Mon, 7 Jun 2021 05:18:50 -0400 Received: from mail.kernel.org ([198.145.29.99]:40162 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230246AbhFGJSo (ORCPT ); Mon, 7 Jun 2021 05:18:44 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 0A2AC61002; Mon, 7 Jun 2021 09:16:49 +0000 (UTC) Date: Mon, 7 Jun 2021 11:16:47 +0200 From: Christian Brauner To: Changbin Du Cc: Cong Wang , Jakub Kicinski , Alexander Viro , "David S. Miller" , Linux Kernel Network Developers , LKML , linux-fsdevel , stable , David Laight Subject: Re: [PATCH] nsfs: fix oops when ns->ops is not provided Message-ID: <20210607091647.pzqyarxbupvnbxyw@wittgenstein> References: <20210531153410.93150-1-changbin.du@gmail.com> <20210531220128.26c0cb36@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net> <20210602091451.kbdul6nhobilwqvi@wittgenstein> <20210604095451.nkfgpsibm5nrqt3f@wittgenstein> <20210606224322.yxr47tgdqis35dcl@mail.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210606224322.yxr47tgdqis35dcl@mail.google.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 07, 2021 at 06:43:41AM +0800, Changbin Du wrote: > On Fri, Jun 04, 2021 at 11:54:51AM +0200, Christian Brauner wrote: > > On Thu, Jun 03, 2021 at 03:52:29PM -0700, Cong Wang wrote: > > > On Wed, Jun 2, 2021 at 2:14 AM Christian Brauner > > > wrote: > > > > But the point is that ns->ops should never be accessed when that > > > > namespace type is disabled. Or in other words, the bug is that something > > > > in netns makes use of namespace features when they are disabled. If we > > > > handle ->ops being NULL we might be tapering over a real bug somewhere. > > > > > > It is merely a protocol between fs/nsfs.c and other namespace users, > > > so there is certainly no right or wrong here, the only question is which > > > one is better. > > > > > > > > > > > Jakub's proposal in the other mail makes sense and falls in line with > > > > how the rest of the netns getters are implemented. For example > > > > get_net_ns_fd_fd(): > > > > > > It does not make any sense to me. get_net_ns() merely increases > > > the netns refcount, which is certainly fine for init_net too, no matter > > > CONFIG_NET_NS is enabled or disabled. Returning EOPNOTSUPP > > > there is literally saying we do not support increasing init_net refcount, > > > which is of course false. > > > > > > > struct net *get_net_ns_by_fd(int fd) > > > > { > > > > return ERR_PTR(-EINVAL); > > > > } > > > > > > There is a huge difference between just increasing netns refcount > > > and retrieving it by fd, right? I have no idea why you bring this up, > > > calling them getters is missing their difference. > > > > This argument doesn't hold up. All netns helpers ultimately increase the > > reference count of the net namespace they find. And if any of them > > perform operations where they are called in environments wherey they > > need CONFIG_NET_NS they handle this case at compile time. > > > > (Pluse they are defined in a central place in net/net_namespace.{c,h}. > > That includes the low-level get_net() function and all the others. > > get_net_ns() is the only one that's defined out of band. So get_net_ns() > > currently is arguably also misplaced.) > > > Ihe get_net_ns() was a static helper function and then sb made it exported > but didn't move it. See commit d8d211a2a0 ('net: Make extern and export get_net_ns()'). > > > The problem I have with fixing this in nsfs is that it gives the > > impression that this is a bug in nsfs whereas it isn't and it > > potentially helps tapering over other bugs. > > > > get_net_ns() is only called for codepaths that call into nsfs via > > open_related_ns() and it's the only namespace that does this. But > > open_related_ns() is only well defined if CONFIG_ is > > set. For example, none of the procfs namespace f_ops will be set for > > !CONFIG_NET_NS. So clearly the socket specific getter here is buggy as > > it doesn't account for !CONFIG_NET_NS and it should be fixed. > I agree with Cong that a pure getter returns a generic error is a bit weird. > And get_net_ns() is to get the ns_common which always exists indepent of > CONFIG_NET_NS. For get_net_ns_by_fd(), I think it is a 'findder + getter'. > > So maybe we can rollback to patch V1 to fix all code called into > open_related_ns()? > https://lore.kernel.org/netdev/CAM_iQpWwApLVg39rUkyXxnhsiP0SZf=0ft6vsq=VxFtJ2SumAQ@mail.gmail.com/T/ > > --- a/net/socket.c > +++ b/net/socket.c > @@ -1149,11 +1149,15 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg) > mutex_unlock(&vlan_ioctl_mutex); > break; > case SIOCGSKNS: > +#ifdef CONFIG_NET_NS > err = -EPERM; > if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > break; > > err = open_related_ns(&net->ns, get_net_ns); > +#else > + err = -ENOTSUPP; > +#endif If Jakub is fine with it I don't really care much but then you need to fix the other places in tun.c as well. I'm just not sure what's so special about get_net_ns() that it can't simply get an ifdef !CONFIG_NET_NS section. But it seems that there's magic semantics deeply hidden in this helper that btw only exists for open_related_ns() that makes this necessary: drivers/net/tun.c: return open_related_ns(&net->ns, get_net_ns); drivers/net/tun.c: ret = open_related_ns(&net->ns, get_net_ns); include/linux/socket.h:extern struct ns_common *get_net_ns(struct ns_common *ns); net/socket.c: * get_net_ns - increment the refcount of the network namespace net/socket.c:struct ns_common *get_net_ns(struct ns_common *ns) net/socket.c:EXPORT_SYMBOL_GPL(get_net_ns); net/socket.c: err = open_related_ns(&net->ns, get_net_ns); tools/perf/trace/beauty/include/linux/socket.h:extern struct ns_common *get_net_ns(struct ns_common *ns); > > > > > Plus your fix leaks references to init netns without fixing get_net_ns() > > too. > > You succeed to increase the refcount of init netns in get_net_ns() but > > then you return in __ns_get_path() because ns->ops aren't set before > > ns->ops->put() can be called. But you also _can't_ call it since it's > > not set because !CONFIG_NET_NS. So everytime you call any of those > > ioctls you increas the refcount of init net ns without decrementing it > > on failure. So the fix is buggy as it is too and would suggest you to > > fixup get_net_ns() too. > Yes, it is a problem. Can be put a BUG_ON() in nsfs so that such bug (calling > into nsfs without ops) can be catched early? Maybe place a WARN_ON() in there. Christian