> 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
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?
> 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
