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).
> 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
> > 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>
