From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37B5AC433E0 for ; Thu, 4 Jun 2020 03:39:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0946920829 for ; Thu, 4 Jun 2020 03:39:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=sargun.me header.i=@sargun.me header.b="BkAIZS5U" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726563AbgFDDjL (ORCPT ); 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-Google-Smtp-Source: ABdhPJzpJ/gMzj+cF7tx0wsNnght7qYo/ErxsBP4IEom49PMUnSj0R69oQgNuqTUaWsGxM0Rfd7pDA== 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.