Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp993773ybg; Wed, 3 Jun 2020 20:42:15 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwAKZ3VT9SwdRsx9SI5bWdlClDzcJdXl1ale1b71hz24+oPDgBJbIhG9d2HSKj+eF7b/cQi X-Received: by 2002:a17:906:2c07:: with SMTP id e7mr2153290ejh.172.1591242135801; Wed, 03 Jun 2020 20:42:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591242135; cv=none; d=google.com; s=arc-20160816; b=dzpQ9JldcW3R8nSkJz1ZFEQ5TZaBgyTa4UHahMBvDNgXBgBRTJcEkcBPu7acNkkwkg fOSoYAljKHw2E3ydrSUZ0Qk4Ta8aU/GMh4w796F6/+SKMuU8vGNgzd9VtIlP9Dx4SWj5 0llTBSOp2UX3ExwBEmJltZXxbr5Hqu9vXIYHKdMbzWZMdxS0qmARnIt/7OhHQyRF/OTM kU8K25GytVq6eGZn4AX7d7NAUoXOt0S5F1MlaTMak7Da+TNLyMJaY5MtQ5ifMgvmCb43 WtBW3YA50cBSrz+NE9tUw4dSaUwPNR30wDJolH+l0mWc0Dzry1lVBuEXM2JpxMGv9SFS 3+7Q== 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:dkim-signature; bh=JL8KAHI97qtguZ9Tb2fscmILbngbHV5HZl94fH1QWiU=; b=AymZ21esLyhbkXbEAxaFfTXzldZ1RKxN3n65ueYWu/uvV0jkRJkxytv/BbOBBfL302 9VaLNWJhTKs8P6vkk9Hkh7jNRzo2CQ2l00ANv++0z52MUFjyPFYm1uxKKB5WEO5OKXKN lVfmghLIyg9ydqT2ozUAFmzBkjgkMgMRNQdvUt2XvRyf3BgqkQHRphC/MDdEr0N6dU6T zVN9tru9q45a811UxtPfuPUUv/K1pGDn0fBTPwkQhzKanY2b6NfJHQyK3Ojzznm0SEdR ySEKaY9P47dATFJH4K4JaSEUB3BhrPZ4hPp9wUhgjKmZNUDbd+p0m5FQmE9Y9wY/ESi8 8PSQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sargun.me header.s=google header.b=BkAIZS5U; 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 p11si917516edq.151.2020.06.03.20.41.52; Wed, 03 Jun 2020 20:42:15 -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; dkim=pass header.i=@sargun.me header.s=google header.b=BkAIZS5U; 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 S1726422AbgFDDjL (ORCPT + 99 others); Wed, 3 Jun 2020 23:39:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37438 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726047AbgFDDjK (ORCPT ); Wed, 3 Jun 2020 23:39:10 -0400 Received: from mail-il1-x141.google.com (mail-il1-x141.google.com [IPv6:2607:f8b0:4864:20::141]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9F292C03E96D for ; Wed, 3 Jun 2020 20:39:10 -0700 (PDT) Received: by mail-il1-x141.google.com with SMTP id d1so4690069ila.8 for ; Wed, 03 Jun 2020 20:39:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sargun.me; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=JL8KAHI97qtguZ9Tb2fscmILbngbHV5HZl94fH1QWiU=; b=BkAIZS5UfiiyWUPoA8i0jqjgQC/WADaVCEyErmWLwK8t3GHDHFMmZ0cWMKoqVrqQqf GO+t+UGX7bnWxdI5DqxuR9Q92NU7fHo24UZMJGMnMMJRKYKo4x/F7pwAsbhyN7Gn60ju n7q3PRtkSlmouoqf/iL9VnER2wtgOLpNaqOco= 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=JL8KAHI97qtguZ9Tb2fscmILbngbHV5HZl94fH1QWiU=; b=fKbVE1PXzIQVSkDDlSTAnfO+7l2Sr6hVIRYjnjsr9TN0kyjNlU8xv9nIE/jsSe4IXs uxll9HY5VzGrv1J1qjqcbKoa4Gx3Lx51Bcbknk0c8ScSm80j1ikvl27b5mZz0aYrkkkn Es6Hn1rMGpewSjX01ajR8D4TT/a4UeGqorrxOkkLvseu+hUs7Zm9xzPQ0SufR9lq39My Q51xChNcROS/I52mER1mIExd0wqpka9PGiC+DMuMxaG1FapcJYcH8Qfohce+E9CsE7oX 1mpb9JVB98nxPtIma8sedlL9kKZWc5bQTJJNIcrWZUrVLcjtEtPJCoJAkS4Qk18sKeN4 5+tQ== X-Gm-Message-State: AOAM531XWTCF/mWHoycUIUqEz30TeoOQgrD0paLvUI8vSOy5rU5MwnNL lcSvj/zNTgc0mANQLciVR0amJA== X-Received: by 2002:a92:c812:: with SMTP id v18mr2474993iln.178.1591241949645; Wed, 03 Jun 2020 20:39:09 -0700 (PDT) Received: from ircssh-2.c.rugged-nimbus-611.internal (80.60.198.104.bc.googleusercontent.com. [104.198.60.80]) by smtp.gmail.com with ESMTPSA id v20sm828328ilc.1.2020.06.03.20.39.09 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Wed, 03 Jun 2020 20:39:09 -0700 (PDT) Date: Thu, 4 Jun 2020 03:39:07 +0000 From: Sargun Dhillon To: Christian Brauner Cc: Kees Cook , linux-kernel@vger.kernel.org, Tycho Andersen , Matt Denton , Jann Horn , Chris Palmer , Aleksa Sarai , Robert Sesek , containers@lists.linux-foundation.org, Giuseppe Scrivano , Greg Kroah-Hartman , Al Viro , Daniel Wagner , "David S . Miller" , John Fastabend , Tejun Heo , stable@vger.kernel.org, cgroups@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Message-ID: <20200604033907.GA16025@ircssh-2.c.rugged-nimbus-611.internal> References: <20200603011044.7972-1-sargun@sargun.me> <20200603011044.7972-2-sargun@sargun.me> <20200604012452.vh33nufblowuxfed@wittgenstein> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200604012452.vh33nufblowuxfed@wittgenstein> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 04, 2020 at 03:24:52AM +0200, Christian Brauner wrote: > On Tue, Jun 02, 2020 at 06:10:41PM -0700, Sargun Dhillon wrote: > > Previously there were two chunks of code where the logic to receive file > > descriptors was duplicated in net. The compat version of copying > > file descriptors via SCM_RIGHTS did not have logic to update cgroups. > > Logic to change the cgroup data was added in: > > commit 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly") > > commit d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly") > > > > This was not copied to the compat path. This commit fixes that, and thus > > should be cherry-picked into stable. > > > > This introduces a helper (file_receive) which encapsulates the logic for > > handling calling security hooks as well as manipulating cgroup information. > > This helper can then be used other places in the kernel where file > > descriptors are copied between processes > > > > I tested cgroup classid setting on both the compat (x32) path, and the > > native path to ensure that when moving the file descriptor the classid > > is set. > > > > Signed-off-by: Sargun Dhillon > > Suggested-by: Kees Cook > > Cc: Al Viro > > Cc: Christian Brauner > > Cc: Daniel Wagner > > Cc: David S. Miller > > Cc: Jann Horn , > > Cc: John Fastabend > > Cc: Tejun Heo > > Cc: Tycho Andersen > > Cc: stable@vger.kernel.org > > Cc: cgroups@vger.kernel.org > > Cc: linux-fsdevel@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > fs/file.c | 35 +++++++++++++++++++++++++++++++++++ > > include/linux/file.h | 1 + > > net/compat.c | 10 +++++----- > > net/core/scm.c | 14 ++++---------- > > 4 files changed, 45 insertions(+), 15 deletions(-) > > > > diff --git a/fs/file.c b/fs/file.c > > index abb8b7081d7a..5afd76fca8c2 100644 > > --- a/fs/file.c > > +++ b/fs/file.c > > @@ -18,6 +18,9 @@ > > #include > > #include > > #include > > +#include > > +#include > > +#include > > > > unsigned int sysctl_nr_open __read_mostly = 1024*1024; > > unsigned int sysctl_nr_open_min = BITS_PER_LONG; > > @@ -931,6 +934,38 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) > > return err; > > } > > > > +/* > > + * File Receive - Receive a file from another process > > + * > > + * This function is designed to receive files from other tasks. It encapsulates > > + * logic around security and cgroups. The file descriptor provided must be a > > + * freshly allocated (unused) file descriptor. > > + * > > + * This helper does not consume a reference to the file, so the caller must put > > + * their reference. > > + * > > + * Returns 0 upon success. > > + */ > > +int file_receive(int fd, struct file *file) > > This is all just a remote version of fd_install(), yet it deviates from > fd_install()'s semantics and naming. That's not great imho. What about > naming this something like: > > fd_install_received() > > and move the get_file() out of there so it has the same semantics as > fd_install(). It seems rather dangerous to have a function like > fd_install() that consumes a reference once it returned and another > version of this that is basically the same thing but doesn't consume a > reference because it takes its own. Seems an invitation for confusion. > Does that make sense? > You're right. The reason for the difference in my mind is that fd_install always succeeds, whereas file_receive can fail. It's easier to do something like: fd_install(fd, get_file(f)) vs. if (file_receive(fd, get_file(f)) fput(f); Alternatively, if the reference was always consumed, it is somewhat easier. I'm fine either way, but just explaining my reasoning for the difference in behaviour.