> On Jan 27, 2023, at 11:12 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> 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.
I'll add that clarification to the patch description.
> The cause of all 3 failures are the same though, and this patch should
> fix it.
Since this regression has been around for a bit, I plan to apply
the fix to nfsd-next. Anyone who wants it backported to stable can
apply it and test it -- I'm going to leave off a Cc: stable or Fixes:.
>>>>> 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>
--
Chuck Lever
