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