diff mbox series

[RFC] nfsd: don't hand out write delegations on O_WRONLY opens

Message ID 20230731-wdeleg-v1-1-f8fe1ce11b36@kernel.org
State New
Headers
Series [RFC] nfsd: don't hand out write delegations on O_WRONLY opens |

Commit Message

Jeff Layton July 31, 2023, 8:27 p.m. UTC 
  I noticed that xfstests generic/001 was failing against linux-next nfsd.

The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
would hand out a write delegation. The client would then try to use that
write delegation as the source stateid in a COPY or CLONE operation, and
the server would respond with NFS4ERR_STALE.

The problem is that the struct file associated with the delegation does
not necessarily have read permissions. It's handing out a write
delegation on what is effectively an O_WRONLY open. RFC 8881 states:

 "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
  own, all opens."

Given that the client didn't request any read permissions, and that nfsd
didn't check for any, it seems wrong to give out a write delegation.

Don't hand out a delegation if the client didn't request
OPEN4_SHARE_ACCESS_BOTH.

This fixes xfstest generic/001.

Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 2 ++
 1 file changed, 2 insertions(+)


---
base-commit: ec89391563792edd11d138a853901bce76d11f44
change-id: 20230731-wdeleg-bbdb6b25a3c6

Best regards,

Comments

NeilBrown July 31, 2023, 10:11 p.m. UTC | #1 
On Tue, 01 Aug 2023, Jeff Layton wrote:
> I noticed that xfstests generic/001 was failing against linux-next nfsd.
> 
> The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> would hand out a write delegation. The client would then try to use that
> write delegation as the source stateid in a COPY or CLONE operation, and
> the server would respond with NFS4ERR_STALE.
> 
> The problem is that the struct file associated with the delegation does
> not necessarily have read permissions. It's handing out a write
> delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> 
>  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
>   own, all opens."
> 
> Given that the client didn't request any read permissions, and that nfsd
> didn't check for any, it seems wrong to give out a write delegation.
> 
> Don't hand out a delegation if the client didn't request
> OPEN4_SHARE_ACCESS_BOTH.
> 
> This fixes xfstest generic/001.
> 
> Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ef7118ebee00..9f1c90afed72 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5462,6 +5462,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		return ERR_PTR(-EAGAIN);
>  
>  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> +		if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ))
> +			return ERR_PTR(-EBADF);
<bikeshed>
The actual error code returned by nfs4_set_delegation() is ignored -
only the fact of an error is relevant.
Given that, how did you choose -EBADF.  nfsd doesn't use file
descriptors, and doesn't use EBADF anywhere else.
Given that you have just tested access, EACCES might be justifiable.
But I would prefer if nfs4_set_delegation() returns NULL if it could not
find or create a delegation, without bothering with giving a reason.
</bikeshed>

Reviewed-by: NeilBrown <neilb@suse.de>

NeilBrown

>  		nf = find_writeable_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
>  	} else {
> 
> ---
> base-commit: ec89391563792edd11d138a853901bce76d11f44
> change-id: 20230731-wdeleg-bbdb6b25a3c6
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
>
Chuck Lever July 31, 2023, 10:43 p.m. UTC | #2 
On Mon, Jul 31, 2023 at 04:27:30PM -0400, Jeff Layton wrote:
> I noticed that xfstests generic/001 was failing against linux-next nfsd.

Only on NFSv4.2 mounts, I presume?


> The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> would hand out a write delegation. The client would then try to use that
> write delegation as the source stateid in a COPY or CLONE operation, and
> the server would respond with NFS4ERR_STALE.
> 
> The problem is that the struct file associated with the delegation does
> not necessarily have read permissions. It's handing out a write
> delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> 
>  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
>   own, all opens."
> 
> Given that the client didn't request any read permissions, and that nfsd
> didn't check for any, it seems wrong to give out a write delegation.

A client is, in fact, permitted to use a write delegation stateid
in an otw READ operation. So, this makes sense to me.


> Don't hand out a delegation if the client didn't request
> OPEN4_SHARE_ACCESS_BOTH.
> 
> This fixes xfstest generic/001.
> 
> Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

I'm thinking this should be squashed into commit
68a593f24a35 ("NFSD: Enable write delegation support").


> ---
>  fs/nfsd/nfs4state.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ef7118ebee00..9f1c90afed72 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5462,6 +5462,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		return ERR_PTR(-EAGAIN);
>  
>  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> +		if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ))
> +			return ERR_PTR(-EBADF);

			return ERR_PTR(-EAGAIN);

might be more consistent with the other failure returns in this
function.


>  		nf = find_writeable_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
>  	} else {
> 
> ---
> base-commit: ec89391563792edd11d138a853901bce76d11f44
> change-id: 20230731-wdeleg-bbdb6b25a3c6
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
>
Jeff Layton July 31, 2023, 10:44 p.m. UTC | #3 
On Tue, 2023-08-01 at 08:11 +1000, NeilBrown wrote:
> On Tue, 01 Aug 2023, Jeff Layton wrote:
> > I noticed that xfstests generic/001 was failing against linux-next nfsd.
> > 
> > The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> > would hand out a write delegation. The client would then try to use that
> > write delegation as the source stateid in a COPY or CLONE operation, and
> > the server would respond with NFS4ERR_STALE.
> > 
> > The problem is that the struct file associated with the delegation does
> > not necessarily have read permissions. It's handing out a write
> > delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> > 
> >  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
> >   own, all opens."
> > 
> > Given that the client didn't request any read permissions, and that nfsd
> > didn't check for any, it seems wrong to give out a write delegation.
> > 
> > Don't hand out a delegation if the client didn't request
> > OPEN4_SHARE_ACCESS_BOTH.
> > 
> > This fixes xfstest generic/001.
> > 
> > Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ef7118ebee00..9f1c90afed72 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5462,6 +5462,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  		return ERR_PTR(-EAGAIN);
> >  
> >  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> > +		if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ))
> > +			return ERR_PTR(-EBADF);
> <bikeshed>
> The actual error code returned by nfs4_set_delegation() is ignored -
> only the fact of an error is relevant.
> Given that, how did you choose -EBADF.  nfsd doesn't use file
> descriptors, and doesn't use EBADF anywhere else.
> Given that you have just tested access, EACCES might be justifiable.
> But I would prefer if nfs4_set_delegation() returns NULL if it could not
> find or create a delegation, without bothering with giving a reason.
> </bikeshed>
> 

I chose EBADF because the fcntl code uses it for similar purposes. From
the manpage:

EBADF	cmd is F_SETLK or F_SETLKW and the file descriptor open       
	mode doesn't match with the type of lock requested.

We're requesting a "lock" here in a delegation, so this made some sense
to me. I'm not particular here though. If another error makes more
sense, then that's fine.


> Reviewed-by: NeilBrown <neilb@suse.de>
> 

Thanks!

> NeilBrown
> 
> >  		nf = find_writeable_file(fp);
> >  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> >  	} else {
> > 
> > ---
> > base-commit: ec89391563792edd11d138a853901bce76d11f44
> > change-id: 20230731-wdeleg-bbdb6b25a3c6
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
>
Jeff Layton July 31, 2023, 11:37 p.m. UTC | #4 
On Mon, 2023-07-31 at 18:43 -0400, Chuck Lever wrote:
> On Mon, Jul 31, 2023 at 04:27:30PM -0400, Jeff Layton wrote:
> > I noticed that xfstests generic/001 was failing against linux-next nfsd.
> 
> Only on NFSv4.2 mounts, I presume?
> 

Correct.

> 
> > The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> > would hand out a write delegation. The client would then try to use that
> > write delegation as the source stateid in a COPY or CLONE operation, and
> > the server would respond with NFS4ERR_STALE.
> > 
> > The problem is that the struct file associated with the delegation does
> > not necessarily have read permissions. It's handing out a write
> > delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> > 
> >  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
> >   own, all opens."
> > 
> > Given that the client didn't request any read permissions, and that nfsd
> > didn't check for any, it seems wrong to give out a write delegation.
> 
> A client is, in fact, permitted to use a write delegation stateid
> in an otw READ operation. So, this makes sense to me.
> 

Good.

> 
> > Don't hand out a delegation if the client didn't request
> > OPEN4_SHARE_ACCESS_BOTH.
> > 
> > This fixes xfstest generic/001.
> > 
> > Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> I'm thinking this should be squashed into commit
> 68a593f24a35 ("NFSD: Enable write delegation support").
> 

Sounds great to me.

> 
> > ---
> >  fs/nfsd/nfs4state.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ef7118ebee00..9f1c90afed72 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5462,6 +5462,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  		return ERR_PTR(-EAGAIN);
> >  
> >  	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> > +		if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ))
> > +			return ERR_PTR(-EBADF);
> 
> 			return ERR_PTR(-EAGAIN);
> 
> might be more consistent with the other failure returns in this
> function.
> 

Shrug, it doesn't matter much. A distinctive error is nice for debugging
purposes though.

> 
> >  		nf = find_writeable_file(fp);
> >  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> >  	} else {
> > 
> > ---
> > base-commit: ec89391563792edd11d138a853901bce76d11f44
> > change-id: 20230731-wdeleg-bbdb6b25a3c6
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
>
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ef7118ebee00..9f1c90afed72 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5462,6 +5462,8 @@  nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		return ERR_PTR(-EAGAIN);
 
 	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+		if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ))
+			return ERR_PTR(-EBADF);
 		nf = find_writeable_file(fp);
 		dl_type = NFS4_OPEN_DELEGATE_WRITE;
 	} else {