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 <[email protected]>
---
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);
nf = find_writeable_file(fp);
dl_type = NFS4_OPEN_DELEGATE_WRITE;
} else {
---
base-commit: ec89391563792edd11d138a853901bce76d11f44
change-id: 20230731-wdeleg-bbdb6b25a3c6
Best regards,
--
Jeff Layton <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>
>
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 <[email protected]>
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 <[email protected]>
>
--
Chuck Lever
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 <[email protected]>
>
> 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 <[email protected]>
> >
>
--
Jeff Layton <[email protected]>