On Fri, 2023-01-27 at 15:35 +0000, Chuck Lever III wrote:
>
> > On Jan 27, 2023, at 10:30 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Fri, 2023-01-27 at 15:21 +0000, Chuck Lever III wrote:
> > >
> > > > On Jan 27, 2023, at 7:09 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > >
> > > > We had a bug report that xfstest generic/355 was failing on NFSv4.0.
> > > > This test sets various combinations of setuid/setgid modes and tests
> > > > whether DIO writes will cause them to be stripped.
> > > >
> > > > What I found was that the server did properly strip those bits, but
> > > > the client didn't notice because it held a delegation that was not
> > > > recalled. The recall didn't occur because the client itself was the
> > > > one generating the activity and we avoid recalls in that case.
> > > >
> > > > Clearing setuid bits is an "implicit" activity. The client didn't
> > > > specifically request that we do that, so we need the server to issue a
> > > > CB_RECALL, or avoid the situation entirely by not issuing a delegation.
> > > >
> > > > The easiest fix here is to simply not give out a delegation if the file
> > > > is being opened for write, and the mode has the setuid and/or setgid bit
> > > > set. Note that there is a potential race between the mode and lease
> > > > being set, so we test for this condition both before and after setting
> > > > the lease.
> > > >
> > > > This patch fixes generic/355, generic/683 and generic/684 for me.
> > >
> > > generic/355 2s ... 1s
> > >
> >
> > I should note that 355 only fails with vers=4.0. On 4.1+ the client
> > specifies that it doesn't want a delegation (as this test is doing DIO).
>
> I used a NFSv4.0 mount for the test.
>
>
> > > That's good.
> > >
> > > generic/683 2s ... [not run] xfs_io falloc failed (old kernel/wrong fs?)
> > > generic/684 2s ... [not run] xfs_io fpunch failed (old kernel/wrong fs?)
> > >
> > > What am I doing wrong?
> > >
> > >
> >
> > Not sure here. This test requires v4.2, but the client and server should
> > negotiate that. You might need to run the test by hand and see what it
> > outputs. i.e.:
> >
> > $ sudo ./tests/generic/683
>
> Then, these two failed only for NFSv4.2 and are not run for other
> minor versions. For some reason I thought this was an NFSv4.0-only
> bug.
>
Ok, that's expected. I should have been more clear. 355 only fails on
v4.0 only, but 683 and 684 require v4.2 to run and fail.
The cause of all 3 failures are the same though, and this patch should
fix it.
>
> > > > Reported-by: Boyang Xue <bxue@xxxxxxxxxx>
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > > fs/nfsd/nfs4state.c | 27 +++++++++++++++++++++++++++
> > > > 1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index e61b878a4b45..ace02fd0d590 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -5421,6 +5421,23 @@ nfsd4_verify_deleg_dentry(struct nfsd4_open *open, struct nfs4_file *fp,
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * We avoid breaking delegations held by a client due to its own activity, but
> > > > + * clearing setuid/setgid bits on a write is an implicit activity and the client
> > > > + * may not notice and continue using the old mode. Avoid giving out a delegation
> > > > + * on setuid/setgid files when the client is requesting an open for write.
> > > > + */
> > > > +static int
> > > > +nfsd4_verify_setuid_write(struct nfsd4_open *open, struct nfsd_file *nf)
> > > > +{
> > > > + struct inode *inode = file_inode(nf->nf_file);
> > > > +
> > > > + if ((open->op_share_access & NFS4_SHARE_ACCESS_WRITE) &&
> > > > + (inode->i_mode & (S_ISUID|S_ISGID)))
> > > > + return -EAGAIN;
> > > > + return 0;
> > > > +}
> > > > +
> > > > static struct nfs4_delegation *
> > > > nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > > > struct svc_fh *parent)
> > > > @@ -5454,6 +5471,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > > > spin_lock(&fp->fi_lock);
> > > > if (nfs4_delegation_exists(clp, fp))
> > > > status = -EAGAIN;
> > > > + else if (nfsd4_verify_setuid_write(open, nf))
> > > > + status = -EAGAIN;
> > > > else if (!fp->fi_deleg_file) {
> > > > fp->fi_deleg_file = nf;
> > > > /* increment early to prevent fi_deleg_file from being
> > > > @@ -5494,6 +5513,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > > > if (status)
> > > > goto out_unlock;
> > > >
> > > > + /*
> > > > + * Now that the deleg is set, check again to ensure that nothing
> > > > + * raced in and changed the mode while we weren't lookng.
> > > > + */
> > > > + status = nfsd4_verify_setuid_write(open, fp->fi_deleg_file);
> > > > + if (status)
> > > > + goto out_unlock;
> > > > +
> > > > spin_lock(&state_lock);
> > > > spin_lock(&fp->fi_lock);
> > > > if (fp->fi_had_conflict)
> > > > --
> > > > 2.39.1
> > > >
> > >
> > > --
> > > Chuck Lever
> > >
> > >
> > >
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
