Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 May 2023 09:00:20 +0200
From:   Christian Brauner <brauner@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Sherry Yang <sherry.yang@oracle.com>
Subject: Re: [PATCH] nfsd: use vfs setgid helper
Message-ID: <20230503-mehrverbrauch-spargel-258668d27f53@brauner>
References: <20230502-agenda-regeln-04d2573bd0fd@brauner>
 <77C7061E-2316-4C73-89E5-7C8CA0AEB6FD@oracle.com>
 <51805A56-F815-405F-8CDF-4CD04A17436C@oracle.com>
 <741D94D5-4058-452E-830B-49D3BEBD5D8E@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <741D94D5-4058-452E-830B-49D3BEBD5D8E@oracle.com>
Precedence: bulk

On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
> 
> 
> > On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> > 
> > 
> > 
> >> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> >> 
> >>> 
> >>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> wrote:
> >>> 
> >>> We've aligned setgid behavior over multiple kernel releases. The details
> >>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
> >>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
> >>> Consistent setgid stripping behavior is now encapsulated in the
> >>> setattr_should_drop_sgid() helper which is used by all filesystems that
> >>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
> >>> raised in e.g., chown_common() and is subject to the
> >>> setattr_should_drop_sgid() check to determine whether the setgid bit can
> >>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
> >>> will cause notify_change() to strip it even if the caller had the
> >>> necessary privileges to retain it. Ensure that nfsd only raises
> >>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
> >>> setgid bit.
> >>> 
> >>> Without this patch the setgid stripping tests in LTP will fail:
> >>> 
> >>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
> >>>> non-group-executable file while chown was invoked by super-user, while
> >>> 
> >>> [...]
> >>> 
> >>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>> 
> >>> [...]
> >>> 
> >>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>> 
> >>> With this patch all tests pass.
> >>> 
> >>> Reported-by: Sherry Yang <sherry.yang@oracle.com>
> >>> Signed-off-by: Christian Brauner <brauner@kernel.org>
> >> 
> >> There are some similar fstests failures this fix might address.
> >> 
> >> I've applied this patch to the nfsd-fixes tree for broader
> >> testing.
> > 
> > ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
> > 
> > Did I apply this patch to the wrong kernel?
> 
> setattr_should_drop_sgid() is not available to callers built as
> modules. It needs an EXPORT_SYMBOL or _GPL.

Hey Chuck,

Thanks for taking a look!
The required export is part of
commit 4f704d9a835 ("nfs: use vfs setgid helper")
which is in current mainline as of Monday this week which was part of my
v6.4/vfs.misc PR that was merged.

So this should all work fine on mainline. Seems I didn't use --base
which is why that info was missing.

Thanks!
Christian