| Message ID | 20230127120933.7056-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: don't hand out delegation on setuid files being opened for write | expand |
> On Jan 27, 2023, at 7:09 AM, Jeff Layton <jlayton@kernel.org> 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@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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
On Fri, 2023-01-27 at 15:21 +0000, Chuck Lever III wrote: > > > On Jan 27, 2023, at 7:09 AM, Jeff Layton <jlayton@kernel.org> 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@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > 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 > > >
> On Jan 27, 2023, at 10:30 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@redhat.com> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> 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@kernel.org> -- Chuck Lever
On Fri, 2023-01-27 at 15:35 +0000, Chuck Lever III wrote: > > > On Jan 27, 2023, at 10:30 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@redhat.com> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > 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@kernel.org> > > -- > Chuck Lever > > >
> On Jan 27, 2023, at 11:12 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@kernel.org> 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@redhat.com> >>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>>> --- >>>>> 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@kernel.org> >> >> -- >> Chuck Lever >> >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Fri, 2023-01-27 at 16:17 +0000, Chuck Lever III wrote: > > > On Jan 27, 2023, at 11:12 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@kernel.org> 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. > Thanks! > > > 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:. > I think that sounds reasonable. Setuid files are pretty rare overall, and this only affects the writing client's perception of them. It's hard to nail down a Fixes: for this. I suspect the regression came about once we stopped recalling delegations for activity by the same client, but I haven't tested that theory.
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)
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. Reported-by: Boyang Xue <bxue@redhat.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)