Christian Brauner <brauner@kernel.org> writes: > On Thu, Feb 15, 2024 at 09:16:36PM -0800, Vinicius Costa Gomes wrote: >> Fix the following warning when defining a cleanup guard for a "const" >> pointer type: >> >> ./include/linux/cleanup.h:211:18: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] >> 211 | return _T->lock; \ >> | ~~^~~~~~ >> ./include/linux/cleanup.h:233:1: note: in expansion of macro ‘__DEFINE_UNLOCK_GUARD’ >> 233 | __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \ >> | ^~~~~~~~~~~~~~~~~~~~~ >> ./include/linux/cred.h:193:1: note: in expansion of macro ‘DEFINE_LOCK_GUARD_1’ >> 193 | DEFINE_LOCK_GUARD_1(cred, const struct cred, _T->lock = override_creds_light(_T->lock), >> | ^~~~~~~~~~~~~~~~~~~ >> >> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> --- >> include/linux/cleanup.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h >> index c2d09bc4f976..085482ef46c8 100644 >> --- a/include/linux/cleanup.h >> +++ b/include/linux/cleanup.h >> @@ -208,7 +208,7 @@ static inline void class_##_name##_destructor(class_##_name##_t *_T) \ >> \ >> static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \ >> { \ >> - return _T->lock; \ >> + return (void *)_T->lock; \ >> } > > I think both of these patches are a bit ugly as we burden the generic > cleanup code with casting to void which could cause actual issues. Fair point. > > Casting from const to non-const is rather specific to the cred code so I > would rather like to put the burden on the cred code instead of the > generic code if possible. For what it's worth, I liked your changes, will remove these two "fixes" from the series and use your suggestions in the next version. Thank you, -- Vinicius
The pull request you sent on Mon, 18 Mar 2024 19:06:38 +0200: > git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git ovl-fixes-6.9-rc1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/0d7ca657df77a3d97698d6ec392894362d2ad334 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Hi Linus, Please pull overlayfs fixes for 6.9-rc1. This branch contains only minor fixes. It has been sitting in linux-next overnight and it has gone through the usual overlayfs test routines. The branch merges cleanly with master branch of the moment. Thanks, Amir. ---------------------------------------------------------------- The following changes since commit 277100b3d5fefacba4f5ff18e2e52a9553eb6e3f: Merge tag 'block-6.9-20240315' of git://git.kernel.dk/linux (2024-03-15 14:55:50 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git ovl-fixes-6.9-rc1 for you to fetch changes up to 77a28aa476873048024ad56daf8f4f17d58ee48e: ovl: relax WARN_ON in ovl_verify_area() (2024-03-17 15:59:41 +0200) ---------------------------------------------------------------- overlayfs fixes for 6.9-rc1 - Fix uncalled for WARN_ON from v6.8-rc1 - Fix the overlayfs MAINTAINERS entry ---------------------------------------------------------------- Amir Goldstein (2): MAINTAINERS: update overlayfs git tree ovl: relax WARN_ON in ovl_verify_area() MAINTAINERS | 2 +- fs/overlayfs/copy_up.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
On Mon, Mar 18, 2024 at 12:12:06PM -0400, Jeff Layton wrote: > On Sun, 2024-03-17 at 11:34 -0400, Chuck Lever wrote: > > On Fri, Mar 15, 2024 at 12:53:03PM -0400, Jeff Layton wrote: > > > This adds basic infrastructure for handing GET_DIR_DELEGATION calls from > > > clients, including the decoders and encoders. For now, the server side > > > always just returns that the delegation is GDDR_UNAVAIL (and that we > > > won't call back). > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++ > > > fs/nfsd/nfs4xdr.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > fs/nfsd/xdr4.h | 8 ++++++ > > > include/linux/nfs4.h | 5 ++++ > > > 4 files changed, 113 insertions(+), 2 deletions(-) > > > > Just a handful of style preferences below. > > > > Those comments all make sense. I'll respin along those lines. > > Also, I may go ahead and send this patch separately from the rest of the > series. I think it would be best to have trivial support for > GET_DIR_DELEGATION in the kernel server as soon as possible. > > Eventually, clients may start sending these calls, and it's better if we > can just return a non-fatal error instead of sending back NFSERR_NOTSUPP > when they do. I have no objection to the XDR pieces going into NFSD before the rest of the series. Thanks for this implementation! > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > index 2927b1263f08..7973fe17bf3c 100644 > > > --- a/fs/nfsd/nfs4proc.c > > > +++ b/fs/nfsd/nfs4proc.c > > > @@ -2173,6 +2173,18 @@ nfsd4_layout_verify(struct svc_export *exp, unsigned int layout_type) > > > return nfsd4_layout_ops[layout_type]; > > > } > > > > > > +static __be32 > > > +nfsd4_get_dir_delegation(struct svc_rqst *rqstp, > > > + struct nfsd4_compound_state *cstate, > > > + union nfsd4_op_u *u) > > > +{ > > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > > + > > > + /* FIXME: actually return a delegation */ > > > + gdd->nf_status = GDD4_UNAVAIL; > > > + return nfs_ok; > > > +} > > > + > > > static __be32 > > > nfsd4_getdeviceinfo(struct svc_rqst *rqstp, > > > struct nfsd4_compound_state *cstate, union nfsd4_op_u *u) > > > @@ -3082,6 +3094,18 @@ static u32 nfsd4_copy_notify_rsize(const struct svc_rqst *rqstp, > > > * sizeof(__be32); > > > } > > > > > > +static u32 nfsd4_get_dir_delegation_rsize(const struct svc_rqst *rqstp, > > > + const struct nfsd4_op *op) > > > +{ > > > + return (op_encode_hdr_size + > > > + 1 /* gddr_status */ + > > > + op_encode_verifier_maxsz + > > > + op_encode_stateid_maxsz + > > > + 2 /* gddr_notification */ + > > > + 2 /* gddr_child_attributes */ + > > > + 2 /* gddr_dir_attributes */); > > > +} > > > + > > > #ifdef CONFIG_NFSD_PNFS > > > static u32 nfsd4_getdeviceinfo_rsize(const struct svc_rqst *rqstp, > > > const struct nfsd4_op *op) > > > @@ -3470,6 +3494,12 @@ static const struct nfsd4_operation nfsd4_ops[] = { > > > .op_get_currentstateid = nfsd4_get_freestateid, > > > .op_rsize_bop = nfsd4_only_status_rsize, > > > }, > > > + [OP_GET_DIR_DELEGATION] = { > > > + .op_func = nfsd4_get_dir_delegation, > > > + .op_flags = OP_MODIFIES_SOMETHING, > > > + .op_name = "OP_GET_DIR_DELEGATION", > > > + .op_rsize_bop = nfsd4_get_dir_delegation_rsize, > > > + }, > > > #ifdef CONFIG_NFSD_PNFS > > > [OP_GETDEVICEINFO] = { > > > .op_func = nfsd4_getdeviceinfo, > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > index fac938f563ad..3718bef74e9f 100644 > > > --- a/fs/nfsd/nfs4xdr.c > > > +++ b/fs/nfsd/nfs4xdr.c > > > @@ -1732,6 +1732,40 @@ nfsd4_decode_free_stateid(struct nfsd4_compoundargs *argp, > > > return nfsd4_decode_stateid4(argp, &free_stateid->fr_stateid); > > > } > > > > > > +static __be32 > > > +nfsd4_decode_get_dir_delegation(struct nfsd4_compoundargs *argp, > > > + union nfsd4_op_u *u) > > > +{ > > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > > + struct timespec64 ts; > > > + u32 signal_deleg_avail; > > > + u32 attrs[1]; > > > > I know this isn't how we've done XDR in the past, but I'd rather > > see these dummy args as fields in struct nfsd4_get_dir_delegation, > > and also move the comments about whether each argument is supported > > to the putative nfsd4_proc_get_dir_delegation(). > > > > The actual implementation of GET_DIR_DELEGATION is in nfs4proc.c, > > after all, not here. This is simply a translation function. > > > > > > > + __be32 status; > > > + > > > + memset(gdd, 0, sizeof(*gdd)); > > > + > > > + /* No signal_avail support for now (and maybe never) */ > > > + if (xdr_stream_decode_bool(argp->xdr, &signal_deleg_avail) < 0) > > > + return nfserr_bad_xdr; > > > + status = nfsd4_decode_bitmap4(argp, gdd->notification_types, > > > + ARRAY_SIZE(gdd->notification_types)); > > > + if (status) > > > + return status; > > > + > > > + /* For now, we don't support child or dir attr change notification */ > > > + status = nfsd4_decode_nfstime4(argp, &ts); > > > + if (status) > > > + return status; > > > + /* No dir attr notification support yet either */ > > > + status = nfsd4_decode_nfstime4(argp, &ts); > > > + if (status) > > > + return status; > > > + status = nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > > + if (status) > > > + return status; > > > + return nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > > +} > > > + > > > #ifdef CONFIG_NFSD_PNFS > > > static __be32 > > > nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp, > > > @@ -2370,7 +2404,7 @@ static const nfsd4_dec nfsd4_dec_ops[] = { > > > [OP_CREATE_SESSION] = nfsd4_decode_create_session, > > > [OP_DESTROY_SESSION] = nfsd4_decode_destroy_session, > > > [OP_FREE_STATEID] = nfsd4_decode_free_stateid, > > > - [OP_GET_DIR_DELEGATION] = nfsd4_decode_notsupp, > > > + [OP_GET_DIR_DELEGATION] = nfsd4_decode_get_dir_delegation, > > > #ifdef CONFIG_NFSD_PNFS > > > [OP_GETDEVICEINFO] = nfsd4_decode_getdeviceinfo, > > > [OP_GETDEVICELIST] = nfsd4_decode_notsupp, > > > @@ -5002,6 +5036,40 @@ nfsd4_encode_device_addr4(struct xdr_stream *xdr, > > > return nfserr_toosmall; > > > } > > > > > > +static __be32 > > > +nfsd4_encode_get_dir_delegation(struct nfsd4_compoundres *resp, __be32 nfserr, > > > + union nfsd4_op_u *u) > > > +{ > > > + struct xdr_stream *xdr = resp->xdr; > > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > > + > > > + /* Encode the GDDR_* status code */ > > > > In other encoders, I've used simply the name of the field as it is > > in the RFC as a documenting comment. That's more clear, and is > > easily grep-able. So: > > > > /* gddrnf_status */ > > > > > > > + if (xdr_stream_encode_u32(xdr, gdd->nf_status) != XDR_UNIT) > > > + return nfserr_resource; > > > + > > > + /* if it's GDD4_UNAVAIL then we're (almost) done */ > > > + if (gdd->nf_status == GDD4_UNAVAIL) { > > > > I prefer using a switch for XDR unions. That makes our > > implementation look more like the XDR definition; easier for humans > > to audit and modify. > > > > > > > + /* We never call back */ > > > + return nfsd4_encode_bool(xdr, false); > > > > Again, let's move this boolean to struct nfsd4_get_dir_delegation to > > enable nfsd4_proc_get_dir_delegation to decide in the future. > > > > > > > + } > > > + > > > + /* GDD4_OK case */ > > > > If a switch is used, then this comment becomes a real piece of > > self-verifying code: > > > > case GDD4_OK: > > > > > > > + nfserr = nfsd4_encode_verifier4(xdr, &gdd->cookieverf); > > > + if (nfserr) > > > + return nfserr; > > > + nfserr = nfsd4_encode_stateid4(xdr, &gdd->stateid); > > > + if (nfserr) > > > + return nfserr; > > > + /* No notifications (yet) */ > > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > > + if (nfserr) > > > + return nfserr; > > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > > + if (nfserr) > > > + return nfserr; > > > + return nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > > > All these as well can go in struct nfsd4_get_dir_delegation. > > > > > > > +} > > > + > > > static __be32 > > > nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr, > > > union nfsd4_op_u *u) > > > @@ -5580,7 +5648,7 @@ static const nfsd4_enc nfsd4_enc_ops[] = { > > > [OP_CREATE_SESSION] = nfsd4_encode_create_session, > > > [OP_DESTROY_SESSION] = nfsd4_encode_noop, > > > [OP_FREE_STATEID] = nfsd4_encode_noop, > > > - [OP_GET_DIR_DELEGATION] = nfsd4_encode_noop, > > > + [OP_GET_DIR_DELEGATION] = nfsd4_encode_get_dir_delegation, > > > #ifdef CONFIG_NFSD_PNFS > > > [OP_GETDEVICEINFO] = nfsd4_encode_getdeviceinfo, > > > [OP_GETDEVICELIST] = nfsd4_encode_noop, > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > index 415516c1b27e..27de75f32dea 100644 > > > --- a/fs/nfsd/xdr4.h > > > +++ b/fs/nfsd/xdr4.h > > > @@ -518,6 +518,13 @@ struct nfsd4_free_stateid { > > > stateid_t fr_stateid; /* request */ > > > }; > > > > > > +struct nfsd4_get_dir_delegation { > > > + u32 notification_types[1]; /* request */ > > > + u32 nf_status; /* response */ > > > + nfs4_verifier cookieverf; /* response */ > > > + stateid_t stateid; /* response */ > > > +}; > > > + > > > /* also used for NVERIFY */ > > > struct nfsd4_verify { > > > u32 ve_bmval[3]; /* request */ > > > @@ -797,6 +804,7 @@ struct nfsd4_op { > > > struct nfsd4_reclaim_complete reclaim_complete; > > > struct nfsd4_test_stateid test_stateid; > > > struct nfsd4_free_stateid free_stateid; > > > + struct nfsd4_get_dir_delegation get_dir_delegation; > > > struct nfsd4_getdeviceinfo getdeviceinfo; > > > struct nfsd4_layoutget layoutget; > > > struct nfsd4_layoutcommit layoutcommit; > > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > > > index ef8d2d618d5b..11ad088b411d 100644 > > > --- a/include/linux/nfs4.h > > > +++ b/include/linux/nfs4.h > > > @@ -701,6 +701,11 @@ enum state_protect_how4 { > > > SP4_SSV = 2 > > > }; > > > > > > +enum get_dir_delegation_non_fatal_res { > > > + GDD4_OK = 0, > > > + GDD4_UNAVAIL = 1 > > > +}; > > > + > > > enum pnfs_layouttype { > > > LAYOUT_NFSV4_1_FILES = 1, > > > LAYOUT_OSD2_OBJECTS = 2, > > > > > > -- > > > 2.44.0 > > > > > > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Sun, 2024-03-17 at 11:34 -0400, Chuck Lever wrote: > On Fri, Mar 15, 2024 at 12:53:03PM -0400, Jeff Layton wrote: > > This adds basic infrastructure for handing GET_DIR_DELEGATION calls from > > clients, including the decoders and encoders. For now, the server side > > always just returns that the delegation is GDDR_UNAVAIL (and that we > > won't call back). > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++ > > fs/nfsd/nfs4xdr.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > fs/nfsd/xdr4.h | 8 ++++++ > > include/linux/nfs4.h | 5 ++++ > > 4 files changed, 113 insertions(+), 2 deletions(-) > > Just a handful of style preferences below. > Those comments all make sense. I'll respin along those lines. Also, I may go ahead and send this patch separately from the rest of the series. I think it would be best to have trivial support for GET_DIR_DELEGATION in the kernel server as soon as possible. Eventually, clients may start sending these calls, and it's better if we can just return a non-fatal error instead of sending back NFSERR_NOTSUPP when they do. > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index 2927b1263f08..7973fe17bf3c 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -2173,6 +2173,18 @@ nfsd4_layout_verify(struct svc_export *exp, unsigned int layout_type) > > return nfsd4_layout_ops[layout_type]; > > } > > > > +static __be32 > > +nfsd4_get_dir_delegation(struct svc_rqst *rqstp, > > + struct nfsd4_compound_state *cstate, > > + union nfsd4_op_u *u) > > +{ > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + > > + /* FIXME: actually return a delegation */ > > + gdd->nf_status = GDD4_UNAVAIL; > > + return nfs_ok; > > +} > > + > > static __be32 > > nfsd4_getdeviceinfo(struct svc_rqst *rqstp, > > struct nfsd4_compound_state *cstate, union nfsd4_op_u *u) > > @@ -3082,6 +3094,18 @@ static u32 nfsd4_copy_notify_rsize(const struct svc_rqst *rqstp, > > * sizeof(__be32); > > } > > > > +static u32 nfsd4_get_dir_delegation_rsize(const struct svc_rqst *rqstp, > > + const struct nfsd4_op *op) > > +{ > > + return (op_encode_hdr_size + > > + 1 /* gddr_status */ + > > + op_encode_verifier_maxsz + > > + op_encode_stateid_maxsz + > > + 2 /* gddr_notification */ + > > + 2 /* gddr_child_attributes */ + > > + 2 /* gddr_dir_attributes */); > > +} > > + > > #ifdef CONFIG_NFSD_PNFS > > static u32 nfsd4_getdeviceinfo_rsize(const struct svc_rqst *rqstp, > > const struct nfsd4_op *op) > > @@ -3470,6 +3494,12 @@ static const struct nfsd4_operation nfsd4_ops[] = { > > .op_get_currentstateid = nfsd4_get_freestateid, > > .op_rsize_bop = nfsd4_only_status_rsize, > > }, > > + [OP_GET_DIR_DELEGATION] = { > > + .op_func = nfsd4_get_dir_delegation, > > + .op_flags = OP_MODIFIES_SOMETHING, > > + .op_name = "OP_GET_DIR_DELEGATION", > > + .op_rsize_bop = nfsd4_get_dir_delegation_rsize, > > + }, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = { > > .op_func = nfsd4_getdeviceinfo, > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index fac938f563ad..3718bef74e9f 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -1732,6 +1732,40 @@ nfsd4_decode_free_stateid(struct nfsd4_compoundargs *argp, > > return nfsd4_decode_stateid4(argp, &free_stateid->fr_stateid); > > } > > > > +static __be32 > > +nfsd4_decode_get_dir_delegation(struct nfsd4_compoundargs *argp, > > + union nfsd4_op_u *u) > > +{ > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + struct timespec64 ts; > > + u32 signal_deleg_avail; > > + u32 attrs[1]; > > I know this isn't how we've done XDR in the past, but I'd rather > see these dummy args as fields in struct nfsd4_get_dir_delegation, > and also move the comments about whether each argument is supported > to the putative nfsd4_proc_get_dir_delegation(). > > The actual implementation of GET_DIR_DELEGATION is in nfs4proc.c, > after all, not here. This is simply a translation function. > > > > + __be32 status; > > + > > + memset(gdd, 0, sizeof(*gdd)); > > + > > + /* No signal_avail support for now (and maybe never) */ > > + if (xdr_stream_decode_bool(argp->xdr, &signal_deleg_avail) < 0) > > + return nfserr_bad_xdr; > > + status = nfsd4_decode_bitmap4(argp, gdd->notification_types, > > + ARRAY_SIZE(gdd->notification_types)); > > + if (status) > > + return status; > > + > > + /* For now, we don't support child or dir attr change notification */ > > + status = nfsd4_decode_nfstime4(argp, &ts); > > + if (status) > > + return status; > > + /* No dir attr notification support yet either */ > > + status = nfsd4_decode_nfstime4(argp, &ts); > > + if (status) > > + return status; > > + status = nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > + if (status) > > + return status; > > + return nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > +} > > + > > #ifdef CONFIG_NFSD_PNFS > > static __be32 > > nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp, > > @@ -2370,7 +2404,7 @@ static const nfsd4_dec nfsd4_dec_ops[] = { > > [OP_CREATE_SESSION] = nfsd4_decode_create_session, > > [OP_DESTROY_SESSION] = nfsd4_decode_destroy_session, > > [OP_FREE_STATEID] = nfsd4_decode_free_stateid, > > - [OP_GET_DIR_DELEGATION] = nfsd4_decode_notsupp, > > + [OP_GET_DIR_DELEGATION] = nfsd4_decode_get_dir_delegation, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = nfsd4_decode_getdeviceinfo, > > [OP_GETDEVICELIST] = nfsd4_decode_notsupp, > > @@ -5002,6 +5036,40 @@ nfsd4_encode_device_addr4(struct xdr_stream *xdr, > > return nfserr_toosmall; > > } > > > > +static __be32 > > +nfsd4_encode_get_dir_delegation(struct nfsd4_compoundres *resp, __be32 nfserr, > > + union nfsd4_op_u *u) > > +{ > > + struct xdr_stream *xdr = resp->xdr; > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + > > + /* Encode the GDDR_* status code */ > > In other encoders, I've used simply the name of the field as it is > in the RFC as a documenting comment. That's more clear, and is > easily grep-able. So: > > /* gddrnf_status */ > > > > + if (xdr_stream_encode_u32(xdr, gdd->nf_status) != XDR_UNIT) > > + return nfserr_resource; > > + > > + /* if it's GDD4_UNAVAIL then we're (almost) done */ > > + if (gdd->nf_status == GDD4_UNAVAIL) { > > I prefer using a switch for XDR unions. That makes our > implementation look more like the XDR definition; easier for humans > to audit and modify. > > > > + /* We never call back */ > > + return nfsd4_encode_bool(xdr, false); > > Again, let's move this boolean to struct nfsd4_get_dir_delegation to > enable nfsd4_proc_get_dir_delegation to decide in the future. > > > > + } > > + > > + /* GDD4_OK case */ > > If a switch is used, then this comment becomes a real piece of > self-verifying code: > > case GDD4_OK: > > > > + nfserr = nfsd4_encode_verifier4(xdr, &gdd->cookieverf); > > + if (nfserr) > > + return nfserr; > > + nfserr = nfsd4_encode_stateid4(xdr, &gdd->stateid); > > + if (nfserr) > > + return nfserr; > > + /* No notifications (yet) */ > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > + if (nfserr) > > + return nfserr; > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > + if (nfserr) > > + return nfserr; > > + return nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > All these as well can go in struct nfsd4_get_dir_delegation. > > > > +} > > + > > static __be32 > > nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr, > > union nfsd4_op_u *u) > > @@ -5580,7 +5648,7 @@ static const nfsd4_enc nfsd4_enc_ops[] = { > > [OP_CREATE_SESSION] = nfsd4_encode_create_session, > > [OP_DESTROY_SESSION] = nfsd4_encode_noop, > > [OP_FREE_STATEID] = nfsd4_encode_noop, > > - [OP_GET_DIR_DELEGATION] = nfsd4_encode_noop, > > + [OP_GET_DIR_DELEGATION] = nfsd4_encode_get_dir_delegation, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = nfsd4_encode_getdeviceinfo, > > [OP_GETDEVICELIST] = nfsd4_encode_noop, > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index 415516c1b27e..27de75f32dea 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -518,6 +518,13 @@ struct nfsd4_free_stateid { > > stateid_t fr_stateid; /* request */ > > }; > > > > +struct nfsd4_get_dir_delegation { > > + u32 notification_types[1]; /* request */ > > + u32 nf_status; /* response */ > > + nfs4_verifier cookieverf; /* response */ > > + stateid_t stateid; /* response */ > > +}; > > + > > /* also used for NVERIFY */ > > struct nfsd4_verify { > > u32 ve_bmval[3]; /* request */ > > @@ -797,6 +804,7 @@ struct nfsd4_op { > > struct nfsd4_reclaim_complete reclaim_complete; > > struct nfsd4_test_stateid test_stateid; > > struct nfsd4_free_stateid free_stateid; > > + struct nfsd4_get_dir_delegation get_dir_delegation; > > struct nfsd4_getdeviceinfo getdeviceinfo; > > struct nfsd4_layoutget layoutget; > > struct nfsd4_layoutcommit layoutcommit; > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > > index ef8d2d618d5b..11ad088b411d 100644 > > --- a/include/linux/nfs4.h > > +++ b/include/linux/nfs4.h > > @@ -701,6 +701,11 @@ enum state_protect_how4 { > > SP4_SSV = 2 > > }; > > > > +enum get_dir_delegation_non_fatal_res { > > + GDD4_OK = 0, > > + GDD4_UNAVAIL = 1 > > +}; > > + > > enum pnfs_layouttype { > > LAYOUT_NFSV4_1_FILES = 1, > > LAYOUT_OSD2_OBJECTS = 2, > > > > -- > > 2.44.0 > > > -- Jeff Layton <jlayton@kernel.org>
On Mon, Mar 18, 2024 at 04:13:12PM +0100, Christian Brauner wrote: > On Thu, Feb 15, 2024 at 09:16:36PM -0800, Vinicius Costa Gomes wrote: > > Fix the following warning when defining a cleanup guard for a "const" > > pointer type: > > > > ./include/linux/cleanup.h:211:18: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > > 211 | return _T->lock; \ > > | ~~^~~~~~ > > ./include/linux/cleanup.h:233:1: note: in expansion of macro ‘__DEFINE_UNLOCK_GUARD’ > > 233 | __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \ > > | ^~~~~~~~~~~~~~~~~~~~~ > > ./include/linux/cred.h:193:1: note: in expansion of macro ‘DEFINE_LOCK_GUARD_1’ > > 193 | DEFINE_LOCK_GUARD_1(cred, const struct cred, _T->lock = override_creds_light(_T->lock), > > | ^~~~~~~~~~~~~~~~~~~ > > > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > > --- > > include/linux/cleanup.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h > > index c2d09bc4f976..085482ef46c8 100644 > > --- a/include/linux/cleanup.h > > +++ b/include/linux/cleanup.h > > @@ -208,7 +208,7 @@ static inline void class_##_name##_destructor(class_##_name##_t *_T) \ > > \ > > static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \ > > { \ > > - return _T->lock; \ > > + return (void *)_T->lock; \ > > } > > I think both of these patches are a bit ugly as we burden the generic > cleanup code with casting to void which could cause actual issues. > > Casting from const to non-const is rather specific to the cred code so I > would rather like to put the burden on the cred code instead of the > generic code if possible. So something like this? (Amir?) diff --git a/fs/backing-file.c b/fs/backing-file.c index ed00ce93cad2..9610d5166736 100644 --- a/fs/backing-file.c +++ b/fs/backing-file.c @@ -152,7 +152,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter, !(file->f_mode & FMODE_CAN_ODIRECT)) return -EINVAL; - guard(cred)(ctx->cred); + cred_guard(ctx->cred); if (is_sync_kiocb(iocb)) { rwf_t rwf = iocb_to_rw_flags(flags); @@ -206,7 +206,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter, */ flags &= ~IOCB_DIO_CALLER_COMP; - guard(cred)(ctx->cred); + cred_guard(ctx->cred); if (is_sync_kiocb(iocb)) { rwf_t rwf = iocb_to_rw_flags(flags); @@ -250,7 +250,7 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos, if (WARN_ON_ONCE(!(in->f_mode & FMODE_BACKING))) return -EIO; - guard(cred)(ctx->cred); + cred_guard(ctx->cred); ret = vfs_splice_read(in, ppos, pipe, len, flags); if (ctx->accessed) @@ -274,7 +274,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe, if (ret) return ret; - guard(cred)(ctx->cred); + cred_guard(ctx->cred); file_start_write(out); ret = iter_file_splice_write(pipe, out, ppos, len, flags); file_end_write(out); @@ -300,7 +300,7 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma, vma_set_file(vma, file); - guard(cred)(ctx->cred); + cred_guard(ctx->cred); ret = call_mmap(vma->vm_file, vma); if (ctx->accessed) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 192997837a56..156d0262ddad 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -1199,7 +1199,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags) if (err) return err; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); while (!err) { struct dentry *next; struct dentry *parent = NULL; diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 3ea5ba7b980d..d9497e34b4c8 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -686,7 +686,7 @@ static int ovl_set_link_redirect(struct dentry *dentry) { int err; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_set_redirect(dentry, false); return err; @@ -891,7 +891,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir) if (err) goto out; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); if (!lower_positive) err = ovl_remove_upper(dentry, is_dir, &list); else diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 44744196bf21..f97d4b106d52 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -41,7 +41,7 @@ static struct file *ovl_open_realfile(const struct file *file, if (flags & O_APPEND) acc_mode |= MAY_APPEND; - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); real_idmap = mnt_idmap(realpath->mnt); err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode); if (err) { @@ -211,7 +211,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) ovl_inode_lock(inode); real.file->f_pos = file->f_pos; - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); ret = vfs_llseek(real.file, offset, whence); file->f_pos = real.file->f_pos; @@ -396,7 +396,7 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) /* Don't sync lower file for fear of receiving EROFS error */ if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) { - guard(cred)(ovl_creds(file_inode(file)->i_sb)); + cred_guard(ovl_creds(file_inode(file)->i_sb)); ret = vfs_fsync_range(real.file, start, end, datasync); } @@ -434,7 +434,7 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len if (ret) goto out_unlock; - guard(cred)(ovl_creds(file_inode(file)->i_sb)); + cred_guard(ovl_creds(file_inode(file)->i_sb)); ret = vfs_fallocate(real.file, mode, offset, len); /* Update size */ @@ -457,7 +457,7 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice) if (ret) return ret; - guard(cred)(ovl_creds(file_inode(file)->i_sb)); + cred_guard(ovl_creds(file_inode(file)->i_sb)); ret = vfs_fadvise(real.file, offset, len, advice); fdput(real); @@ -498,7 +498,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in, goto out_unlock; } - guard(cred)(ovl_creds(file_inode(file_out)->i_sb)); + cred_guard(ovl_creds(file_inode(file_out)->i_sb)); switch (op) { case OVL_COPY: ret = vfs_copy_file_range(real_in.file, pos_in, @@ -574,7 +574,7 @@ static int ovl_flush(struct file *file, fl_owner_t id) return err; if (real.file->f_op->flush) { - guard(cred)(ovl_creds(file_inode(file)->i_sb)); + cred_guard(ovl_creds(file_inode(file)->i_sb)); err = real.file->f_op->flush(real.file, id); } fdput(real); diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 8e399b10ebba..2f3024d2a119 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -78,7 +78,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry, goto out_put_write; inode_lock(upperdentry->d_inode); - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_do_notify_change(ofs, upperdentry, attr); @@ -169,7 +169,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path, metacopy_blocks = ovl_is_metacopy_dentry(dentry); type = ovl_path_real(dentry, &realpath); - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_do_getattr(&realpath, stat, request_mask, flags); if (err) goto out; @@ -306,7 +306,7 @@ int ovl_permission(struct mnt_idmap *idmap, if (err) return err; - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); if (!upperinode && !special_file(realinode->i_mode) && mask & MAY_WRITE) { @@ -328,7 +328,7 @@ static const char *ovl_get_link(struct dentry *dentry, if (!dentry) return ERR_PTR(-ECHILD); - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); p = vfs_get_link(ovl_dentry_real(dentry), done); return p; @@ -461,7 +461,7 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap, acl = get_cached_acl_rcu(realinode, type); } else { - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); acl = ovl_get_acl_path(&realpath, posix_acl_xattr_name(type), noperm); } @@ -487,7 +487,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode, struct posix_acl *real_acl; ovl_path_lower(dentry, &realpath); - scoped_guard(cred, ovl_creds(dentry->d_sb)) + cred_scoped_guard(ovl_creds(dentry->d_sb)) real_acl = vfs_get_acl(mnt_idmap(realpath.mnt), realdentry, acl_name); @@ -510,7 +510,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode, if (err) goto out; - scoped_guard(cred, ovl_creds(dentry->d_sb)) { + cred_scoped_guard(ovl_creds(dentry->d_sb)) { if (acl) err = ovl_do_set_acl(ofs, realdentry, acl_name, acl); else @@ -589,7 +589,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, if (!realinode->i_op->fiemap) return -EOPNOTSUPP; - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); err = realinode->i_op->fiemap(realinode, fieinfo, start, len); return err; @@ -649,7 +649,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap, if (err) goto out; - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); /* * Store immutable/append-only flags in xattr and clear them * in upper fileattr (in case they were set by older kernel) @@ -717,7 +717,7 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa) ovl_path_real(dentry, &realpath); - guard(cred)(ovl_creds(inode->i_sb)); + cred_guard(ovl_creds(inode->i_sb)); err = ovl_real_fileattr_get(&realpath, fa); ovl_fileattr_prot_flags(inode, fa); diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 4b9137572cdc..193f7015c8f5 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -953,7 +953,7 @@ static int ovl_maybe_validate_verity(struct dentry *dentry) return err; if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) { - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_validate_verity(ofs, &metapath, &datapath); if (err == 0) @@ -988,7 +988,7 @@ static int ovl_maybe_lookup_lowerdata(struct dentry *dentry) if (ovl_dentry_lowerdata(dentry)) goto out; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_lookup_data_layers(dentry, redirect, &datapath); if (err) @@ -1055,7 +1055,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, if (dentry->d_name.len > ofs->namelen) return ERR_PTR(-ENAMETOOLONG); - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); upperdir = ovl_dentry_upper(dentry->d_parent); if (upperdir) { d.layer = &ofs->layers[0]; @@ -1381,7 +1381,7 @@ bool ovl_lower_positive(struct dentry *dentry) if (!ovl_dentry_upper(dentry)) return true; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); /* Positive upper -> have to look up lower to see whether it exists */ for (i = 0; !done && !positive && i < ovl_numlower(poe); i++) { struct dentry *this; diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c index d17f8a5aae6f..41e01fe3ae4a 100644 --- a/fs/overlayfs/readdir.c +++ b/fs/overlayfs/readdir.c @@ -274,7 +274,7 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data struct ovl_cache_entry *p; struct dentry *dentry, *dir = path->dentry; - guard(cred)(ovl_creds(rdd->dentry->d_sb)); + cred_guard(ovl_creds(rdd->dentry->d_sb)); err = down_write_killable(&dir->d_inode->i_rwsem); if (!err) { @@ -753,7 +753,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx) struct ovl_cache_entry *p; int err; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); if (!ctx->pos) ovl_dir_reset(file); @@ -853,7 +853,7 @@ static struct file *ovl_dir_open_realfile(const struct file *file, { struct file *res; - guard(cred)(ovl_creds(file_inode(file)->i_sb)); + cred_guard(ovl_creds(file_inode(file)->i_sb)); res = ovl_path_open(realpath, O_RDONLY | (file->f_flags & O_LARGEFILE)); return res; @@ -978,7 +978,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list) struct ovl_cache_entry *p, *n; struct rb_root root = RB_ROOT; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); err = ovl_dir_read_merged(dentry, list, &root); if (err) return err; diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 4f237de0836c..fdce1e453d40 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -1169,7 +1169,7 @@ int ovl_nlink_start(struct dentry *dentry) if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode)) return 0; - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); /* * The overlay inode nlink should be incremented/decremented IFF the * upper operation succeeds, along with nlink change of upper inode. @@ -1198,7 +1198,7 @@ void ovl_nlink_end(struct dentry *dentry) ovl_drop_write(dentry); if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) { - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); ovl_cleanup_index(dentry); } diff --git a/fs/overlayfs/xattrs.c b/fs/overlayfs/xattrs.c index cc4ab2d81162..ad1a656e2798 100644 --- a/fs/overlayfs/xattrs.c +++ b/fs/overlayfs/xattrs.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only +#include <linux/cred.h> #include <linux/fs.h> #include <linux/xattr.h> #include "overlayfs.h" @@ -44,7 +45,7 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char if (!value && !upperdentry) { ovl_path_lower(dentry, &realpath); - scoped_guard(cred, ovl_creds(dentry->d_sb)) + cred_scoped_guard(ovl_creds(dentry->d_sb)) err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0); if (err < 0) goto out; @@ -62,7 +63,7 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char if (err) goto out; - scoped_guard(cred, ovl_creds(dentry->d_sb)) { + cred_scoped_guard(ovl_creds(dentry->d_sb)) { if (value) { err = ovl_do_setxattr(ofs, realdentry, name, value, size, flags); @@ -86,7 +87,7 @@ static int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char struct path realpath; ovl_i_path_real(inode, &realpath); - guard(cred)(ovl_creds(dentry->d_sb)); + cred_guard(ovl_creds(dentry->d_sb)); res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size); return res; } @@ -114,7 +115,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size) char *s; size_t prefix_len, name_len; - scoped_guard(cred, ovl_creds(dentry->d_sb)) + cred_scoped_guard(ovl_creds(dentry->d_sb)) res = vfs_listxattr(realdentry, list, size); if (res <= 0 || size == 0) return res; diff --git a/include/linux/cred.h b/include/linux/cred.h index be1e211d82e0..5a532fce1713 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -171,7 +171,6 @@ static inline bool cap_ambient_invariant_ok(const struct cred *cred) cap_intersect(cred->cap_permitted, cred->cap_inheritable)); } - /* * Override creds without bumping reference count. Caller must ensure * reference remains valid or has taken reference. Almost always not the @@ -190,8 +189,12 @@ static inline void revert_creds_light(const struct cred *revert_cred) rcu_assign_pointer(current->cred, revert_cred); } -DEFINE_LOCK_GUARD_1(cred, const struct cred, _T->lock = override_creds_light(_T->lock), - revert_creds_light(_T->lock)); +DEFINE_LOCK_GUARD_1(cred, struct cred, + _T->lock = (struct cred *)override_creds_light(_T->lock), + revert_creds_light(_T->lock)); + +#define cred_guard(_cred) guard(cred)(((struct cred *)_cred)) +#define cred_scoped_guard(_cred) scoped_guard(cred, ((struct cred *)_cred)) /** * get_new_cred_many - Get references on a new set of credentials
On Thu, Feb 15, 2024 at 09:16:36PM -0800, Vinicius Costa Gomes wrote:
> Fix the following warning when defining a cleanup guard for a "const"
> pointer type:
>
> ./include/linux/cleanup.h:211:18: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 211 | return _T->lock; \
> | ~~^~~~~~
> ./include/linux/cleanup.h:233:1: note: in expansion of macro ‘__DEFINE_UNLOCK_GUARD’
> 233 | __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \
> | ^~~~~~~~~~~~~~~~~~~~~
> ./include/linux/cred.h:193:1: note: in expansion of macro ‘DEFINE_LOCK_GUARD_1’
> 193 | DEFINE_LOCK_GUARD_1(cred, const struct cred, _T->lock = override_creds_light(_T->lock),
> | ^~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> include/linux/cleanup.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..085482ef46c8 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -208,7 +208,7 @@ static inline void class_##_name##_destructor(class_##_name##_t *_T) \
> \
> static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \
> { \
> - return _T->lock; \
> + return (void *)_T->lock; \
> }
I think both of these patches are a bit ugly as we burden the generic
cleanup code with casting to void which could cause actual issues.
Casting from const to non-const is rather specific to the cred code so I
would rather like to put the burden on the cred code instead of the
generic code if possible.
On Sun, 2024-03-17 at 16:03 +0000, Trond Myklebust wrote:
> On Sun, 2024-03-17 at 11:09 -0400, Chuck Lever wrote:
> > On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote:
> > > fh_verify only allows you to filter on a single type of inode, so
> > > have
> > > nfsd4_delegreturn not filter by type. Once fh_verify returns, do
> > > the
> > > appropriate check of the type and return an error if it's not a
> > > regular
> > > file or directory.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > fs/nfsd/nfs4state.c | 14 +++++++++++++-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 17d09d72632b..c52e807f9672 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp,
> > > struct nfsd4_compound_state *cstate,
> > > struct nfs4_delegation *dp;
> > > stateid_t *stateid = &dr->dr_stateid;
> > > struct nfs4_stid *s;
> > > + umode_t mode;
> > > __be32 status;
> > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp),
> > > nfsd_net_id);
> > >
> > > - if ((status = fh_verify(rqstp, &cstate->current_fh,
> > > S_IFREG, 0)))
> > > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0,
> > > 0)))
> > > return status;
> > >
> > > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode &
> > > S_IFMT;
> > > + switch(mode) {
> > > + case S_IFREG:
> > > + case S_IFDIR:
> > > + break;
> > > + case S_IFLNK:
> > > + return nfserr_symlink;
> > > + default:
> > > + return nfserr_inval;
> > > + }
> > > +
> >
> > RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the
> > valid status codes for the DELEGRETURN operation. Maybe the naked
> > fh_verify() call has gotten it wrong all these years...?
>
> The WANT_DELEGATION operation allows the server to hand out delegations
> for aggressive caching of symlinks. It is not an error to return that
> delegation using DELEGRETURN.
>
> Furthermore, provided that the presented stateid is actually valid, it
> is also sufficient to uniquely identify the file to which it is
> associated (see RFC8881 Section 8.2.4), so the filehandle should be
> considered mostly irrelevant for operations like DELEGRETURN.
>
Ok. I think we can probably just drop the switch altogether. We already
don't validate that the stateid is associated with current_fh, AFAICT.
It looks possible to send a valid stateid alongside an FH that refers to
a completely different file, and we'll just accept it.
--
Jeff Layton <jlayton@kernel.org>
On Sun, 2024-03-17 at 10:56 -0400, Chuck Lever wrote: > On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > > The NFSv4.1 protocol adds support for directory delegations, but it > > specifies that if you already have a delegation and try to request a new > > one on the same filehandle, the server must reply that the delegation is > > unavailable. > > > > Add a new lease_manager callback to allow the lease manager (nfsd in > > this case) to impose extra checks when performing a setlease. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/locks.c | 5 +++++ > > include/linux/filelock.h | 10 ++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index cb4b35d26162..415cca8e9565 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > > continue; > > } > > > > + /* Allow the lease manager to veto the setlease */ > > + if (lease->fl_lmops->lm_set_conflict && > > + lease->fl_lmops->lm_set_conflict(lease, fl)) > > + goto out; > > + > > /* > > * No exclusive leases if someone else has a lease on > > * this file: > > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > > index daee999d05f3..c5fc768087df 100644 > > --- a/include/linux/filelock.h > > +++ b/include/linux/filelock.h > > @@ -49,6 +49,16 @@ struct lease_manager_operations { > > int (*lm_change)(struct file_lease *, int, struct list_head *); > > void (*lm_setup)(struct file_lease *, void **); > > bool (*lm_breaker_owns_lease)(struct file_lease *); > > + > > + /** > > + * lm_set_conflict - extra conditions for setlease > > + * @new: new file_lease being set > > + * @old: old (extant) file_lease > > + * > > + * This allows the lease manager to add extra conditions when > > + * setting a lease. > > To make it clear which return value causes add_lease() to abort, I'd > rather see API contract-style descriptions of the meaning of the > return values instead of this design note. Something like: > > * Return values: > * %true: @new and @old conflict > * %false: No conflict detected > > Thanks. I added this to the patch in my tree. > > + */ > > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); > > }; > > > > struct lock_manager { > > > > -- > > 2.44.0 > > > -- Jeff Layton <jlayton@kernel.org>
On Mon, 2024-03-18 at 09:25 +0100, Stefan Metzmacher wrote:
> Hi Jeff,
>
> > In order to add directory delegation support, we need to break
> > delegations on the parent whenever there is going to be a change in the
> > directory.
> >
> > Add a delegated_inode parameter to lookup_open and have it break the
> > delegation. Then, open_last_lookups can wait for the delegation break
> > and retry the call to lookup_open once it's done.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/namei.c | 22 ++++++++++++++++++----
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index f00d8d708001..88598a62ec64 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3404,7 +3404,7 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> > */
> > static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > const struct open_flags *op,
> > - bool got_write)
> > + bool got_write, struct inode **delegated_inode)
>
> Does NFS has a concept of lease keys and parent lease keys?
>
> In SMB it's possible that the client passes a lease key (16 client chosen bytes) to a directory open,
> when asking for a directory lease.
>
> Then operations on files within that directory, take that lease key from the directory as
> 'parent lease keys' in addition to a unique lease key for the file.
>
> That way a client can avoid breaking its own directory leases when creating/move/delete... files
> in the directory.
>
No, it's a bit different with NFSv4 directory delegations. A delegation
is given vs a filehandle (which is analogous to an inode), and it gets a
stateid, which just uniquely identifies it. There is no real association
with the parent.
When you request the dir delegation, you can request to be notified when
something changes instead of the server recalling it. The server may or
may not grant that request. Notifications are not implemented in this
patchset as of yet. I'm focusing on getting the recall handling right
first, and then I'll plan to add that in a later phase.
--
Jeff Layton <jlayton@kernel.org>
Hi Jeff,
> In order to add directory delegation support, we need to break
> delegations on the parent whenever there is going to be a change in the
> directory.
>
> Add a delegated_inode parameter to lookup_open and have it break the
> delegation. Then, open_last_lookups can wait for the delegation break
> and retry the call to lookup_open once it's done.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/namei.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index f00d8d708001..88598a62ec64 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> */
> static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> const struct open_flags *op,
> - bool got_write)
> + bool got_write, struct inode **delegated_inode)
Does NFS has a concept of lease keys and parent lease keys?
In SMB it's possible that the client passes a lease key (16 client chosen bytes) to a directory open,
when asking for a directory lease.
Then operations on files within that directory, take that lease key from the directory as
'parent lease keys' in addition to a unique lease key for the file.
That way a client can avoid breaking its own directory leases when creating/move/delete... files
in the directory.
metze
On Sun, 2024-03-17 at 11:09 -0400, Chuck Lever wrote:
> On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote:
> > fh_verify only allows you to filter on a single type of inode, so
> > have
> > nfsd4_delegreturn not filter by type. Once fh_verify returns, do
> > the
> > appropriate check of the type and return an error if it's not a
> > regular
> > file or directory.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/nfsd/nfs4state.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 17d09d72632b..c52e807f9672 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp,
> > struct nfsd4_compound_state *cstate,
> > struct nfs4_delegation *dp;
> > stateid_t *stateid = &dr->dr_stateid;
> > struct nfs4_stid *s;
> > + umode_t mode;
> > __be32 status;
> > struct nfsd_net *nn = net_generic(SVC_NET(rqstp),
> > nfsd_net_id);
> >
> > - if ((status = fh_verify(rqstp, &cstate->current_fh,
> > S_IFREG, 0)))
> > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0,
> > 0)))
> > return status;
> >
> > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode &
> > S_IFMT;
> > + switch(mode) {
> > + case S_IFREG:
> > + case S_IFDIR:
> > + break;
> > + case S_IFLNK:
> > + return nfserr_symlink;
> > + default:
> > + return nfserr_inval;
> > + }
> > +
>
> RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the
> valid status codes for the DELEGRETURN operation. Maybe the naked
> fh_verify() call has gotten it wrong all these years...?
The WANT_DELEGATION operation allows the server to hand out delegations
for aggressive caching of symlinks. It is not an error to return that
delegation using DELEGRETURN.
Furthermore, provided that the presented stateid is actually valid, it
is also sufficient to uniquely identify the file to which it is
associated (see RFC8881 Section 8.2.4), so the filehandle should be
considered mostly irrelevant for operations like DELEGRETURN.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
On Fri, Mar 15, 2024 at 12:53:05PM -0400, Jeff Layton wrote: > Add a new routine for acquiring a read delegation on a directory. Since > the same CB_RECALL/DELEGRETURN infrastrure is used for regular and > directory delegations, we can just use a normal nfs4_delegation to > represent it. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4proc.c | 23 ++++++++++++++++-- > fs/nfsd/nfs4state.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > fs/nfsd/state.h | 5 ++++ > 3 files changed, 94 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 7973fe17bf3c..1a2c90f4ea53 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -2179,9 +2179,28 @@ nfsd4_get_dir_delegation(struct svc_rqst *rqstp, > union nfsd4_op_u *u) > { > struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > + struct nfs4_delegation *dd; > + struct nfsd_file *nf; > + __be32 status; > + > + status = nfsd_file_acquire_dir(rqstp, &cstate->current_fh, &nf); > + if (status != nfs_ok) > + return status; > + > + dd = nfsd_get_dir_deleg(cstate, gdd, nf); > + if (IS_ERR(dd)) { > + int err = PTR_ERR(dd); > + > + if (err != -EAGAIN) > + return nfserrno(err); > + gdd->nf_status = GDD4_UNAVAIL; > + return nfs_ok; > + } > > - /* FIXME: actually return a delegation */ > - gdd->nf_status = GDD4_UNAVAIL; > + gdd->nf_status = GDD4_OK; > + memcpy(&gdd->stateid, &dd->dl_stid.sc_stateid, sizeof(gdd->stateid)); > + memset(&gdd->cookieverf, '\0', sizeof(gdd->cookieverf)); > + nfs4_put_stid(&dd->dl_stid); > return nfs_ok; > } > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 778c1c6e3b54..36574aedc211 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -8874,7 +8874,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > } > break_lease: > nfsd_stats_wdeleg_getattr_inc(nn); > - dp = fl->fl_owner; > + dp = fl->c.flc_owner; > ncf = &dp->dl_cb_fattr; > nfs4_cb_getattr(&dp->dl_cb_fattr); > spin_unlock(&ctx->flc_lock); > @@ -8912,3 +8912,70 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > spin_unlock(&ctx->flc_lock); > return 0; > } > + I'll admit to not having examined the below function closely, but since it is not a static function, can you add a kdoc comment and a few salient points like, if there is something NFSD doesn't implement (yet), or short cuts that would be good for readers to know about? > +struct nfs4_delegation * > +nfsd_get_dir_deleg(struct nfsd4_compound_state *cstate, > + struct nfsd4_get_dir_delegation *gdd, > + struct nfsd_file *nf) > +{ > + struct nfs4_client *clp = cstate->clp; > + struct nfs4_delegation *dp; > + struct file_lease *fl; > + struct nfs4_file *fp; > + int status = 0; > + > + fp = nfsd4_alloc_file(); > + if (!fp) > + return ERR_PTR(-ENOMEM); > + > + nfsd4_file_init(&cstate->current_fh, fp); > + fp->fi_deleg_file = nf; > + fp->fi_delegees = 1; > + > + spin_lock(&state_lock); > + spin_lock(&fp->fi_lock); > + if (nfs4_delegation_exists(clp, fp)) > + status = -EAGAIN; > + spin_unlock(&fp->fi_lock); > + spin_unlock(&state_lock); > + > + if (status) > + goto out_delegees; > + > + status = -ENOMEM; > + dp = alloc_init_deleg(clp, fp, NULL, NFS4_OPEN_DELEGATE_READ); > + if (!dp) > + goto out_delegees; > + > + fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); > + if (!fl) > + goto out_put_stid; > + > + status = kernel_setlease(nf->nf_file, > + fl->c.flc_type, &fl, NULL); > + if (fl) > + locks_free_lease(fl); > + if (status) > + goto out_put_stid; > + > + spin_lock(&state_lock); > + spin_lock(&clp->cl_lock); > + spin_lock(&fp->fi_lock); > + status = hash_delegation_locked(dp, fp); > + spin_unlock(&fp->fi_lock); > + spin_unlock(&clp->cl_lock); > + spin_unlock(&state_lock); > + > + if (status) > + goto out_unlock; > + > + return dp; > +out_unlock: > + kernel_setlease(fp->fi_deleg_file->nf_file, F_UNLCK, NULL, (void **)&dp); > +out_put_stid: > + nfs4_put_stid(&dp->dl_stid); > +out_delegees: > + put_deleg_file(fp); > + return ERR_PTR(status); > +} > + > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index 01c6f3445646..20551483cc7b 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -782,4 +782,9 @@ static inline bool try_to_expire_client(struct nfs4_client *clp) > > extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, > struct inode *inode, bool *file_modified, u64 *size); > + > +struct nfsd4_get_dir_delegation; > +struct nfs4_delegation *nfsd_get_dir_deleg(struct nfsd4_compound_state *cstate, > + struct nfsd4_get_dir_delegation *gdd, > + struct nfsd_file *nf); > #endif /* NFSD4_STATE_H */ > > -- > 2.44.0 > -- Chuck Lever
On Fri, Mar 15, 2024 at 12:53:03PM -0400, Jeff Layton wrote: > This adds basic infrastructure for handing GET_DIR_DELEGATION calls from > clients, including the decoders and encoders. For now, the server side > always just returns that the delegation is GDDR_UNAVAIL (and that we > won't call back). > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++ > fs/nfsd/nfs4xdr.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > fs/nfsd/xdr4.h | 8 ++++++ > include/linux/nfs4.h | 5 ++++ > 4 files changed, 113 insertions(+), 2 deletions(-) Just a handful of style preferences below. > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 2927b1263f08..7973fe17bf3c 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -2173,6 +2173,18 @@ nfsd4_layout_verify(struct svc_export *exp, unsigned int layout_type) > return nfsd4_layout_ops[layout_type]; > } > > +static __be32 > +nfsd4_get_dir_delegation(struct svc_rqst *rqstp, > + struct nfsd4_compound_state *cstate, > + union nfsd4_op_u *u) > +{ > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > + > + /* FIXME: actually return a delegation */ > + gdd->nf_status = GDD4_UNAVAIL; > + return nfs_ok; > +} > + > static __be32 > nfsd4_getdeviceinfo(struct svc_rqst *rqstp, > struct nfsd4_compound_state *cstate, union nfsd4_op_u *u) > @@ -3082,6 +3094,18 @@ static u32 nfsd4_copy_notify_rsize(const struct svc_rqst *rqstp, > * sizeof(__be32); > } > > +static u32 nfsd4_get_dir_delegation_rsize(const struct svc_rqst *rqstp, > + const struct nfsd4_op *op) > +{ > + return (op_encode_hdr_size + > + 1 /* gddr_status */ + > + op_encode_verifier_maxsz + > + op_encode_stateid_maxsz + > + 2 /* gddr_notification */ + > + 2 /* gddr_child_attributes */ + > + 2 /* gddr_dir_attributes */); > +} > + > #ifdef CONFIG_NFSD_PNFS > static u32 nfsd4_getdeviceinfo_rsize(const struct svc_rqst *rqstp, > const struct nfsd4_op *op) > @@ -3470,6 +3494,12 @@ static const struct nfsd4_operation nfsd4_ops[] = { > .op_get_currentstateid = nfsd4_get_freestateid, > .op_rsize_bop = nfsd4_only_status_rsize, > }, > + [OP_GET_DIR_DELEGATION] = { > + .op_func = nfsd4_get_dir_delegation, > + .op_flags = OP_MODIFIES_SOMETHING, > + .op_name = "OP_GET_DIR_DELEGATION", > + .op_rsize_bop = nfsd4_get_dir_delegation_rsize, > + }, > #ifdef CONFIG_NFSD_PNFS > [OP_GETDEVICEINFO] = { > .op_func = nfsd4_getdeviceinfo, > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index fac938f563ad..3718bef74e9f 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -1732,6 +1732,40 @@ nfsd4_decode_free_stateid(struct nfsd4_compoundargs *argp, > return nfsd4_decode_stateid4(argp, &free_stateid->fr_stateid); > } > > +static __be32 > +nfsd4_decode_get_dir_delegation(struct nfsd4_compoundargs *argp, > + union nfsd4_op_u *u) > +{ > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > + struct timespec64 ts; > + u32 signal_deleg_avail; > + u32 attrs[1]; I know this isn't how we've done XDR in the past, but I'd rather see these dummy args as fields in struct nfsd4_get_dir_delegation, and also move the comments about whether each argument is supported to the putative nfsd4_proc_get_dir_delegation(). The actual implementation of GET_DIR_DELEGATION is in nfs4proc.c, after all, not here. This is simply a translation function. > + __be32 status; > + > + memset(gdd, 0, sizeof(*gdd)); > + > + /* No signal_avail support for now (and maybe never) */ > + if (xdr_stream_decode_bool(argp->xdr, &signal_deleg_avail) < 0) > + return nfserr_bad_xdr; > + status = nfsd4_decode_bitmap4(argp, gdd->notification_types, > + ARRAY_SIZE(gdd->notification_types)); > + if (status) > + return status; > + > + /* For now, we don't support child or dir attr change notification */ > + status = nfsd4_decode_nfstime4(argp, &ts); > + if (status) > + return status; > + /* No dir attr notification support yet either */ > + status = nfsd4_decode_nfstime4(argp, &ts); > + if (status) > + return status; > + status = nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > + if (status) > + return status; > + return nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > +} > + > #ifdef CONFIG_NFSD_PNFS > static __be32 > nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp, > @@ -2370,7 +2404,7 @@ static const nfsd4_dec nfsd4_dec_ops[] = { > [OP_CREATE_SESSION] = nfsd4_decode_create_session, > [OP_DESTROY_SESSION] = nfsd4_decode_destroy_session, > [OP_FREE_STATEID] = nfsd4_decode_free_stateid, > - [OP_GET_DIR_DELEGATION] = nfsd4_decode_notsupp, > + [OP_GET_DIR_DELEGATION] = nfsd4_decode_get_dir_delegation, > #ifdef CONFIG_NFSD_PNFS > [OP_GETDEVICEINFO] = nfsd4_decode_getdeviceinfo, > [OP_GETDEVICELIST] = nfsd4_decode_notsupp, > @@ -5002,6 +5036,40 @@ nfsd4_encode_device_addr4(struct xdr_stream *xdr, > return nfserr_toosmall; > } > > +static __be32 > +nfsd4_encode_get_dir_delegation(struct nfsd4_compoundres *resp, __be32 nfserr, > + union nfsd4_op_u *u) > +{ > + struct xdr_stream *xdr = resp->xdr; > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > + > + /* Encode the GDDR_* status code */ In other encoders, I've used simply the name of the field as it is in the RFC as a documenting comment. That's more clear, and is easily grep-able. So: /* gddrnf_status */ > + if (xdr_stream_encode_u32(xdr, gdd->nf_status) != XDR_UNIT) > + return nfserr_resource; > + > + /* if it's GDD4_UNAVAIL then we're (almost) done */ > + if (gdd->nf_status == GDD4_UNAVAIL) { I prefer using a switch for XDR unions. That makes our implementation look more like the XDR definition; easier for humans to audit and modify. > + /* We never call back */ > + return nfsd4_encode_bool(xdr, false); Again, let's move this boolean to struct nfsd4_get_dir_delegation to enable nfsd4_proc_get_dir_delegation to decide in the future. > + } > + > + /* GDD4_OK case */ If a switch is used, then this comment becomes a real piece of self-verifying code: case GDD4_OK: > + nfserr = nfsd4_encode_verifier4(xdr, &gdd->cookieverf); > + if (nfserr) > + return nfserr; > + nfserr = nfsd4_encode_stateid4(xdr, &gdd->stateid); > + if (nfserr) > + return nfserr; > + /* No notifications (yet) */ > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > + if (nfserr) > + return nfserr; > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > + if (nfserr) > + return nfserr; > + return nfsd4_encode_bitmap4(xdr, 0, 0, 0); All these as well can go in struct nfsd4_get_dir_delegation. > +} > + > static __be32 > nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr, > union nfsd4_op_u *u) > @@ -5580,7 +5648,7 @@ static const nfsd4_enc nfsd4_enc_ops[] = { > [OP_CREATE_SESSION] = nfsd4_encode_create_session, > [OP_DESTROY_SESSION] = nfsd4_encode_noop, > [OP_FREE_STATEID] = nfsd4_encode_noop, > - [OP_GET_DIR_DELEGATION] = nfsd4_encode_noop, > + [OP_GET_DIR_DELEGATION] = nfsd4_encode_get_dir_delegation, > #ifdef CONFIG_NFSD_PNFS > [OP_GETDEVICEINFO] = nfsd4_encode_getdeviceinfo, > [OP_GETDEVICELIST] = nfsd4_encode_noop, > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 415516c1b27e..27de75f32dea 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -518,6 +518,13 @@ struct nfsd4_free_stateid { > stateid_t fr_stateid; /* request */ > }; > > +struct nfsd4_get_dir_delegation { > + u32 notification_types[1]; /* request */ > + u32 nf_status; /* response */ > + nfs4_verifier cookieverf; /* response */ > + stateid_t stateid; /* response */ > +}; > + > /* also used for NVERIFY */ > struct nfsd4_verify { > u32 ve_bmval[3]; /* request */ > @@ -797,6 +804,7 @@ struct nfsd4_op { > struct nfsd4_reclaim_complete reclaim_complete; > struct nfsd4_test_stateid test_stateid; > struct nfsd4_free_stateid free_stateid; > + struct nfsd4_get_dir_delegation get_dir_delegation; > struct nfsd4_getdeviceinfo getdeviceinfo; > struct nfsd4_layoutget layoutget; > struct nfsd4_layoutcommit layoutcommit; > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > index ef8d2d618d5b..11ad088b411d 100644 > --- a/include/linux/nfs4.h > +++ b/include/linux/nfs4.h > @@ -701,6 +701,11 @@ enum state_protect_how4 { > SP4_SSV = 2 > }; > > +enum get_dir_delegation_non_fatal_res { > + GDD4_OK = 0, > + GDD4_UNAVAIL = 1 > +}; > + > enum pnfs_layouttype { > LAYOUT_NFSV4_1_FILES = 1, > LAYOUT_OSD2_OBJECTS = 2, > > -- > 2.44.0 > -- Chuck Lever
On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote: > fh_verify only allows you to filter on a single type of inode, so have > nfsd4_delegreturn not filter by type. Once fh_verify returns, do the > appropriate check of the type and return an error if it's not a regular > file or directory. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 17d09d72632b..c52e807f9672 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > struct nfs4_delegation *dp; > stateid_t *stateid = &dr->dr_stateid; > struct nfs4_stid *s; > + umode_t mode; > __be32 status; > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); > > - if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0))) > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0, 0))) > return status; > > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode & S_IFMT; > + switch(mode) { > + case S_IFREG: > + case S_IFDIR: > + break; > + case S_IFLNK: > + return nfserr_symlink; > + default: > + return nfserr_inval; > + } > + RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the valid status codes for the DELEGRETURN operation. Maybe the naked fh_verify() call has gotten it wrong all these years...? > status = nfsd4_lookup_stateid(cstate, stateid, SC_TYPE_DELEG, 0, &s, nn); > if (status) > goto out; > > -- > 2.44.0 > -- Chuck Lever
On Fri, Mar 15, 2024 at 12:52:53PM -0400, Jeff Layton wrote: > The NFSv4.1 protocol adds support for directory delegations, but it > specifies that if you already have a delegation and try to request a new > one on the same filehandle, the server must reply that the delegation is > unavailable. > > Add a new lease_manager callback to allow the lease manager (nfsd in > this case) to impose extra checks when performing a setlease. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/locks.c | 5 +++++ > include/linux/filelock.h | 10 ++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/fs/locks.c b/fs/locks.c > index cb4b35d26162..415cca8e9565 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1822,6 +1822,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr > continue; > } > > + /* Allow the lease manager to veto the setlease */ > + if (lease->fl_lmops->lm_set_conflict && > + lease->fl_lmops->lm_set_conflict(lease, fl)) > + goto out; > + > /* > * No exclusive leases if someone else has a lease on > * this file: > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > index daee999d05f3..c5fc768087df 100644 > --- a/include/linux/filelock.h > +++ b/include/linux/filelock.h > @@ -49,6 +49,16 @@ struct lease_manager_operations { > int (*lm_change)(struct file_lease *, int, struct list_head *); > void (*lm_setup)(struct file_lease *, void **); > bool (*lm_breaker_owns_lease)(struct file_lease *); > + > + /** > + * lm_set_conflict - extra conditions for setlease > + * @new: new file_lease being set > + * @old: old (extant) file_lease > + * > + * This allows the lease manager to add extra conditions when > + * setting a lease. To make it clear which return value causes add_lease() to abort, I'd rather see API contract-style descriptions of the meaning of the return values instead of this design note. Something like: * Return values: * %true: @new and @old conflict * %false: No conflict detected > + */ > + bool (*lm_set_conflict)(struct file_lease *new, struct file_lease *old); > }; > > struct lock_manager { > > -- > 2.44.0 > -- Chuck Lever
On Sat, 2024-03-16 at 23:57 +0000, Al Viro wrote:
> On Fri, Mar 15, 2024 at 12:52:54PM -0400, Jeff Layton wrote:
> > @@ -4603,9 +4606,12 @@ int vfs_link(struct dentry *old_dentry, struct mnt_idmap *idmap,
> > else if (max_links && inode->i_nlink >= max_links)
> > error = -EMLINK;
> > else {
> > - error = try_break_deleg(inode, delegated_inode);
> > - if (!error)
> > - error = dir->i_op->link(old_dentry, dir, new_dentry);
> > + error = try_break_deleg(dir, delegated_inode);
> > + if (!error) {
> > + error = try_break_deleg(inode, delegated_inode);
> > + if (!error)
> > + error = dir->i_op->link(old_dentry, dir, new_dentry);
> > + }
>
> A minor nit: that might be easier to follow as
> error = try_break_deleg(dir, delegated_inode);
> if (!error)
> error = try_break_deleg(inode, delegated_inode);
> if (!error)
> error = dir->i_op->link(old_dentry, dir, new_dentry);
>
> and let the compiler deal with optimizing it - any C compiler is going to be
> able to figure out that one out. vfs_link() is a mix of those styles anyway -
> we have
> if (!error && (inode->i_state & I_LINKABLE)) {
> spin_lock(&inode->i_lock);
> inode->i_state &= ~I_LINKABLE;
> spin_unlock(&inode->i_lock);
> }
> immediately afterwards; might as well make that consistent, especially since
> you are getting more shallow nesting that way.
Sounds good. Fixed in my tree.
Thanks for the review so far!
--
Jeff Layton <jlayton@kernel.org>
Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+3abd99031b42acf367ef@syzkaller.appspotmail.com Tested on: commit: a8d73a85 ovl: relax WARN_ON in ovl_verify_area() git tree: https://github.com/amir73il/linux ovl-fixes console output: https://syzkaller.appspot.com/x/log.txt?x=14d627b6180000 kernel config: https://syzkaller.appspot.com/x/.config?x=2271d60b0617d474 dashboard link: https://syzkaller.appspot.com/bug?extid=3abd99031b42acf367ef compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 userspace arch: arm64 Note: no patches were applied. Note: testing is done by a robot and is best-effort only.
On Wed, Mar 13, 2024 at 12:23 PM syzbot <syzbot+3abd99031b42acf367ef@syzkaller.appspotmail.com> wrote: > > Hello, > > syzbot found the following issue on: > > HEAD commit: 707081b61156 Merge branch 'for-next/core', remote-tracking.. > git tree: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci > console output: https://syzkaller.appspot.com/x/log.txt?x=1785a859180000 > kernel config: https://syzkaller.appspot.com/x/.config?x=caeac3f3565b057a > dashboard link: https://syzkaller.appspot.com/bug?extid=3abd99031b42acf367ef > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 > userspace arch: arm64 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1115ada6180000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1626870a180000 > > Downloadable assets: > disk image: https://storage.googleapis.com/syzbot-assets/6cad68bf7532/disk-707081b6.raw.xz > vmlinux: https://storage.googleapis.com/syzbot-assets/1a27e5400778/vmlinux-707081b6.xz > kernel image: https://storage.googleapis.com/syzbot-assets/67dfc53755d0/Image-707081b6.gz.xz > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+3abd99031b42acf367ef@syzkaller.appspotmail.com > #syz test: https://github.com/amir73il/linux ovl-fixes Thanks, Amir. > evm: overlay not supported > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 6187 at fs/overlayfs/copy_up.c:239 ovl_copy_up_file+0x624/0x674 fs/overlayfs/copy_up.c:330 > Modules linked in: > CPU: 0 PID: 6187 Comm: syz-executor136 Not tainted 6.8.0-rc7-syzkaller-g707081b61156 #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024 > pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : ovl_copy_up_file+0x624/0x674 fs/overlayfs/copy_up.c:330 > lr : ovl_verify_area fs/overlayfs/copy_up.c:239 [inline] > lr : ovl_copy_up_file+0x620/0x674 fs/overlayfs/copy_up.c:330 > sp : ffff800097997180 > x29: ffff800097997280 x28: 00000000fffffffb x27: ffff700012f32e3c > x26: 0000000000800000 x25: 0000000000800000 x24: ffff800097997240 > x23: ffff800097997220 x22: ffffffffffa64000 x21: ffffffffffa64000 > x20: ffff0000d9fc1900 x19: dfff800000000000 x18: 1ffff00012f32dee > x17: ffff80008ec9d000 x16: ffff80008ad6b1c0 x15: 0000000000000001 > x14: 1fffe0001b9177f2 x13: 0000000000000000 x12: 0000000000000000 > x11: 0000000000000001 x10: 0000000000ff0100 x9 : 0000000000000000 > x8 : ffff0000d7568000 x7 : ffff80008108d924 x6 : 0000000000000000 > x5 : 0000000000000000 x4 : 0000000000000001 x3 : ffff80008031a200 > x2 : 00000ffffffff000 x1 : ffffffffffa64000 x0 : ffffffffffffffff > Call trace: > ovl_copy_up_file+0x624/0x674 fs/overlayfs/copy_up.c:330 > ovl_copy_up_tmpfile fs/overlayfs/copy_up.c:863 [inline] > ovl_do_copy_up fs/overlayfs/copy_up.c:976 [inline] > ovl_copy_up_one fs/overlayfs/copy_up.c:1168 [inline] > ovl_copy_up_flags+0x16d0/0x3694 fs/overlayfs/copy_up.c:1223 > ovl_copy_up+0x24/0x34 fs/overlayfs/copy_up.c:1263 > ovl_setattr+0xfc/0x4e4 fs/overlayfs/inode.c:41 > notify_change+0x9d4/0xc8c fs/attr.c:499 > chmod_common+0x23c/0x418 fs/open.c:648 > do_fchmodat fs/open.c:696 [inline] > __do_sys_fchmodat fs/open.c:715 [inline] > __se_sys_fchmodat fs/open.c:712 [inline] > __arm64_sys_fchmodat+0x118/0x1dc fs/open.c:712 > __invoke_syscall arch/arm64/kernel/syscall.c:34 [inline] > invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:48 > el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:133 > do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:152 > el0_svc+0x54/0x168 arch/arm64/kernel/entry-common.c:712 > el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:730 > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598 > irq event stamp: 862 > hardirqs last enabled at (861): [<ffff8000831abcb4>] percpu_counter_add_batch+0x210/0x30c lib/percpu_counter.c:102 > hardirqs last disabled at (862): [<ffff80008ad66988>] el1_dbg+0x24/0x80 arch/arm64/kernel/entry-common.c:470 > softirqs last enabled at (62): [<ffff80008002189c>] softirq_handle_end kernel/softirq.c:399 [inline] > softirqs last enabled at (62): [<ffff80008002189c>] __do_softirq+0xac8/0xce4 kernel/softirq.c:582 > softirqs last disabled at (53): [<ffff80008002ab48>] ____do_softirq+0x14/0x20 arch/arm64/kernel/irq.c:81 > ---[ end trace 0000000000000000 ]--- > > > --- > This report is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller@googlegroups.com. > > syzbot will keep track of this issue. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > > If the report is already addressed, let syzbot know by replying with: > #syz fix: exact-commit-title > > If you want syzbot to run the reproducer, reply with: > #syz test: git://repo/address.git branch-or-commit-hash > If you attach or paste a git patch, syzbot will apply it before testing. > > If you want to overwrite report's subsystems, reply with: > #syz set subsystems: new-subsystem > (See the list of subsystem names on the web dashboard) > > If the report is a duplicate of another one, reply with: > #syz dup: exact-subject-of-another-report > > If you want to undo deduplication, reply with: > #syz undup
On Sun, 2024-03-17 at 00:19 +0000, Al Viro wrote: > On Fri, Mar 15, 2024 at 12:52:57PM -0400, Jeff Layton wrote: > > In order to add directory delegation support, we need to break > > delegations on the parent whenever there is going to be a change in the > > directory. > > > > Add a delegated_inode parameter to lookup_open and have it break the > > delegation. Then, open_last_lookups can wait for the delegation break > > and retry the call to lookup_open once it's done. > > > @@ -3490,6 +3490,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, > > Wait a sec - are you going to do anything to the atomic_open side of things? > > Hmm good point. I was thinking that all of the filesystems that had atomic_open didn't support leases. I'm wrong though -- there are some that currently do: 9p: It's a network filesystem, and I don't think it has any sort of asynchronous notification or delegation-like object, does it? It might be best though to just make it call simple_nosetlease. fuse: fuse allows leases today. I doubt we can get away with turning that off now. There probably ought to be a way for the userland driver to opt-in or out of allowing built-in lease support maybe a flag or something? ntfs3: IDGI. Why does ntfs3 (which is a local filesystem, unless I'm mistaken) have an atomic_open? Shouldn't lookup+open be fine, like with most local filesystems? vboxsf: Probably the same situation as 9p. Can we just disable leases? I'll spin up a patchset soon to add proper setlease handlers to all of the above. Then we can then guard against allowing generic_setlease on filesystems by default on filesystems with an atomic_open handler. Another (maybe better) idea might be to require filesystems to specify a setlease handler if they want them enabled. We could just set the existing local filesystems to generic_setlease. That would make lease support a strictly opt-in thing, which is probably the best idea for avoiding surprises with them. > > > /* Negative dentry, just create the file */ > > if (!dentry->d_inode && (open_flag & O_CREAT)) { > > + /* but break the directory lease first! */ > > + error = try_break_deleg(dir_inode, delegated_inode); > > + if (error) > > + goto out_dput; -- Jeff Layton <jlayton@kernel.org>
On Fri, Mar 15, 2024 at 12:52:57PM -0400, Jeff Layton wrote: > In order to add directory delegation support, we need to break > delegations on the parent whenever there is going to be a change in the > directory. > > Add a delegated_inode parameter to lookup_open and have it break the > delegation. Then, open_last_lookups can wait for the delegation break > and retry the call to lookup_open once it's done. > @@ -3490,6 +3490,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, Wait a sec - are you going to do anything to the atomic_open side of things? > /* Negative dentry, just create the file */ > if (!dentry->d_inode && (open_flag & O_CREAT)) { > + /* but break the directory lease first! */ > + error = try_break_deleg(dir_inode, delegated_inode); > + if (error) > + goto out_dput;
On Fri, Mar 15, 2024 at 12:52:54PM -0400, Jeff Layton wrote:
> @@ -4603,9 +4606,12 @@ int vfs_link(struct dentry *old_dentry, struct mnt_idmap *idmap,
> else if (max_links && inode->i_nlink >= max_links)
> error = -EMLINK;
> else {
> - error = try_break_deleg(inode, delegated_inode);
> - if (!error)
> - error = dir->i_op->link(old_dentry, dir, new_dentry);
> + error = try_break_deleg(dir, delegated_inode);
> + if (!error) {
> + error = try_break_deleg(inode, delegated_inode);
> + if (!error)
> + error = dir->i_op->link(old_dentry, dir, new_dentry);
> + }
A minor nit: that might be easier to follow as
error = try_break_deleg(dir, delegated_inode);
if (!error)
error = try_break_deleg(inode, delegated_inode);
if (!error)
error = dir->i_op->link(old_dentry, dir, new_dentry);
and let the compiler deal with optimizing it - any C compiler is going to be
able to figure out that one out. vfs_link() is a mix of those styles anyway -
we have
if (!error && (inode->i_state & I_LINKABLE)) {
spin_lock(&inode->i_lock);
inode->i_state &= ~I_LINKABLE;
spin_unlock(&inode->i_lock);
}
immediately afterwards; might as well make that consistent, especially since
you are getting more shallow nesting that way.
On Fri, 2024-03-15 at 16:50 -0400, Anna Schumaker wrote: > Hi Jeff, > > On Fri, Mar 15, 2024 at 12:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > Add a new compound that does a GET_DIR_DELEGATION just before doing a > > GETATTR on an inode. Add a delegation stateid and a nf_status code to > > struct nfs4_getattr_res to store the result. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfs/nfs4xdr.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/nfs4.h | 1 + > > include/linux/nfs_xdr.h | 2 + > > 3 files changed, 139 insertions(+) > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > index 1416099dfcd1..c28025018bda 100644 > > --- a/fs/nfs/nfs4xdr.c > > +++ b/fs/nfs/nfs4xdr.c > > @@ -391,6 +391,22 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req, > > XDR_QUADLEN(NFS4_MAX_SESSIONID_LEN) + 5) > > #define encode_reclaim_complete_maxsz (op_encode_hdr_maxsz + 4) > > #define decode_reclaim_complete_maxsz (op_decode_hdr_maxsz + 4) > > +#define encode_get_dir_delegation_maxsz (op_encode_hdr_maxsz + \ > > + 4 /* gdda_signal_deleg_avail */ + \ > > + 8 /* gdda_notification_types */ + \ > > + nfstime4_maxsz /* gdda_child_attr_delay */ + \ > > + nfstime4_maxsz /* gdda_dir_attr_delay */ + \ > > + nfs4_fattr_bitmap_maxsz /* gdda_child_attributes */ + \ > > + nfs4_fattr_bitmap_maxsz /* gdda_dir_attributes */) > > + > > +#define decode_get_dir_delegation_maxsz (op_encode_hdr_maxsz + \ > > + 4 /* gddrnf_status */ + \ > > + encode_verifier_maxsz /* gddr_cookieverf */ + \ > > + encode_stateid_maxsz /* gddr_stateid */ + \ > > + 8 /* gddr_notification */ + \ > > + nfs4_fattr_bitmap_maxsz /* gddr_child_attributes */ + \ > > + nfs4_fattr_bitmap_maxsz /* gddr_dir_attributes */) > > + > > #define encode_getdeviceinfo_maxsz (op_encode_hdr_maxsz + \ > > XDR_QUADLEN(NFS4_DEVICEID4_SIZE) + \ > > 1 /* layout type */ + \ > > @@ -636,6 +652,18 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req, > > decode_putfh_maxsz + \ > > decode_getattr_maxsz + \ > > decode_renew_maxsz) > > +#define NFS4_enc_gdd_getattr_sz (compound_encode_hdr_maxsz + \ > > + encode_sequence_maxsz + \ > > + encode_putfh_maxsz + \ > > + encode_get_dir_delegation_maxsz + \ > > + encode_getattr_maxsz + \ > > + encode_renew_maxsz) > > +#define NFS4_dec_gdd_getattr_sz (compound_decode_hdr_maxsz + \ > > + decode_sequence_maxsz + \ > > + decode_putfh_maxsz + \ > > + decode_get_dir_delegation_maxsz + \ > > + decode_getattr_maxsz + \ > > + decode_renew_maxsz) > > #define NFS4_enc_lookup_sz (compound_encode_hdr_maxsz + \ > > encode_sequence_maxsz + \ > > encode_putfh_maxsz + \ > > @@ -1981,6 +2009,30 @@ static void encode_sequence(struct xdr_stream *xdr, > > } > > > > #ifdef CONFIG_NFS_V4_1 > > +static void > > +encode_get_dir_delegation(struct xdr_stream *xdr, struct compound_hdr *hdr) > > +{ > > + __be32 *p; > > + struct timespec64 ts = {}; > > + u32 zerobm[1] = {}; > > + > > + encode_op_hdr(xdr, OP_GET_DIR_DELEGATION, decode_get_dir_delegation_maxsz, hdr); > > + > > + /* We can't handle CB_RECALLABLE_OBJ_AVAIL yet */ > > + xdr_stream_encode_bool(xdr, false); > > + > > + /* for now, we request no notification types */ > > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > > + > > + /* Request no attribute updates */ > > + p = reserve_space(xdr, 12 + 12); > > + p = xdr_encode_nfstime4(p, &ts); > > + xdr_encode_nfstime4(p, &ts); > > + > > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > > +} > > + > > static void > > encode_getdeviceinfo(struct xdr_stream *xdr, > > const struct nfs4_getdeviceinfo_args *args, > > @@ -2334,6 +2386,25 @@ static void nfs4_xdr_enc_getattr(struct rpc_rqst *req, struct xdr_stream *xdr, > > encode_nops(&hdr); > > } > > > > +/* > > + * Encode GDD_GETATTR request > > + */ > > +static void nfs4_xdr_enc_gdd_getattr(struct rpc_rqst *req, struct xdr_stream *xdr, > > + const void *data) > > +{ > > + const struct nfs4_getattr_arg *args = data; > > + struct compound_hdr hdr = { > > + .minorversion = nfs4_xdr_minorversion(&args->seq_args), > > + }; > > + > > + encode_compound_hdr(xdr, req, &hdr); > > + encode_sequence(xdr, &args->seq_args, &hdr); > > + encode_putfh(xdr, args->fh, &hdr); > > + encode_get_dir_delegation(xdr, &hdr); > > + encode_getfattr(xdr, args->bitmask, &hdr); > > + encode_nops(&hdr); > > +} > > + > > This function should be under a "#ifdef CONFIG_NFS_V4_1" to avoid the > following compiler error: > > fs/nfs/nfs4xdr.c:2403:2: error: call to undeclared function > 'encode_get_dir_delegation'; ISO C99 and later do not support implicit > function declarations [-Wimplicit-function-declaration] > 2403 | encode_get_dir_delegation(xdr, &hdr); > | ^ > Thanks Anna! I'll fix up both problems before the next iteration. > > > /* > > * Encode a CLOSE request > > */ > > @@ -5919,6 +5990,43 @@ static int decode_layout_stateid(struct xdr_stream *xdr, nfs4_stateid *stateid) > > return decode_stateid(xdr, stateid); > > } > > > > +static int decode_get_dir_delegation(struct xdr_stream *xdr, > > + struct nfs4_getattr_res *res) > > +{ > > + nfs4_verifier cookieverf; > > + int status; > > + u32 bm[1]; > > + > > + status = decode_op_hdr(xdr, OP_GET_DIR_DELEGATION); > > + if (status) > > + return status; > > + > > + if (xdr_stream_decode_u32(xdr, &res->nf_status)) > > + return -EIO; > > + > > + if (res->nf_status == GDD4_UNAVAIL) > > + return xdr_inline_decode(xdr, 4) ? 0 : -EIO; > > + > > + status = decode_verifier(xdr, &cookieverf); > > + if (status) > > + return status; > > + > > + status = decode_delegation_stateid(xdr, &res->deleg); > > + if (status) > > + return status; > > + > > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > > + if (status < 0) > > + return status; > > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > > + if (status < 0) > > + return status; > > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > > + if (status < 0) > > + return status; > > + return 0; > > +} > > + > > static int decode_getdeviceinfo(struct xdr_stream *xdr, > > struct nfs4_getdeviceinfo_res *res) > > { > > @@ -6455,6 +6563,33 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, struct xdr_stream *xdr, > > return status; > > } > > > > +/* > > + * Decode GDD_GETATTR response > > + */ > > +static int nfs4_xdr_dec_gdd_getattr(struct rpc_rqst *rqstp, struct xdr_stream *xdr, > > + void *data) > > +{ > > + struct nfs4_getattr_res *res = data; > > + struct compound_hdr hdr; > > + int status; > > + > > + status = decode_compound_hdr(xdr, &hdr); > > + if (status) > > + goto out; > > + status = decode_sequence(xdr, &res->seq_res, rqstp); > > + if (status) > > + goto out; > > + status = decode_putfh(xdr); > > + if (status) > > + goto out; > > + status = decode_get_dir_delegation(xdr, res); > > + if (status) > > + goto out; > > + status = decode_getfattr(xdr, res->fattr, res->server); > > +out: > > + return status; > > +} > > + > > This needs to be under the same #ifdef, too. > > Thanks, > Anna > > > /* > > * Encode an SETACL request > > */ > > @@ -7704,6 +7839,7 @@ const struct rpc_procinfo nfs4_procedures[] = { > > PROC41(BIND_CONN_TO_SESSION, > > enc_bind_conn_to_session, dec_bind_conn_to_session), > > PROC41(DESTROY_CLIENTID,enc_destroy_clientid, dec_destroy_clientid), > > + PROC41(GDD_GETATTR, enc_gdd_getattr, dec_gdd_getattr), > > PROC42(SEEK, enc_seek, dec_seek), > > PROC42(ALLOCATE, enc_allocate, dec_allocate), > > PROC42(DEALLOCATE, enc_deallocate, dec_deallocate), > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > > index 11ad088b411d..86cbfd50ecd1 100644 > > --- a/include/linux/nfs4.h > > +++ b/include/linux/nfs4.h > > @@ -681,6 +681,7 @@ enum { > > NFSPROC4_CLNT_LISTXATTRS, > > NFSPROC4_CLNT_REMOVEXATTR, > > NFSPROC4_CLNT_READ_PLUS, > > + NFSPROC4_CLNT_GDD_GETATTR, > > }; > > > > /* nfs41 types */ > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > > index d09b9773b20c..85ee37ccc25e 100644 > > --- a/include/linux/nfs_xdr.h > > +++ b/include/linux/nfs_xdr.h > > @@ -1072,6 +1072,8 @@ struct nfs4_getattr_res { > > struct nfs4_sequence_res seq_res; > > const struct nfs_server * server; > > struct nfs_fattr * fattr; > > + nfs4_stateid deleg; > > + u32 nf_status; > > }; > > > > struct nfs4_link_arg { > > > > -- > > 2.44.0 > > -- Jeff Layton <jlayton@kernel.org>
Hi Jeff, On Fri, Mar 15, 2024 at 12:54 PM Jeff Layton <jlayton@kernel.org> wrote: > > Add a new compound that does a GET_DIR_DELEGATION just before doing a > GETATTR on an inode. Add a delegation stateid and a nf_status code to > struct nfs4_getattr_res to store the result. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfs/nfs4xdr.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/nfs4.h | 1 + > include/linux/nfs_xdr.h | 2 + > 3 files changed, 139 insertions(+) > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index 1416099dfcd1..c28025018bda 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -391,6 +391,22 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req, > XDR_QUADLEN(NFS4_MAX_SESSIONID_LEN) + 5) > #define encode_reclaim_complete_maxsz (op_encode_hdr_maxsz + 4) > #define decode_reclaim_complete_maxsz (op_decode_hdr_maxsz + 4) > +#define encode_get_dir_delegation_maxsz (op_encode_hdr_maxsz + \ > + 4 /* gdda_signal_deleg_avail */ + \ > + 8 /* gdda_notification_types */ + \ > + nfstime4_maxsz /* gdda_child_attr_delay */ + \ > + nfstime4_maxsz /* gdda_dir_attr_delay */ + \ > + nfs4_fattr_bitmap_maxsz /* gdda_child_attributes */ + \ > + nfs4_fattr_bitmap_maxsz /* gdda_dir_attributes */) > + > +#define decode_get_dir_delegation_maxsz (op_encode_hdr_maxsz + \ > + 4 /* gddrnf_status */ + \ > + encode_verifier_maxsz /* gddr_cookieverf */ + \ > + encode_stateid_maxsz /* gddr_stateid */ + \ > + 8 /* gddr_notification */ + \ > + nfs4_fattr_bitmap_maxsz /* gddr_child_attributes */ + \ > + nfs4_fattr_bitmap_maxsz /* gddr_dir_attributes */) > + > #define encode_getdeviceinfo_maxsz (op_encode_hdr_maxsz + \ > XDR_QUADLEN(NFS4_DEVICEID4_SIZE) + \ > 1 /* layout type */ + \ > @@ -636,6 +652,18 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req, > decode_putfh_maxsz + \ > decode_getattr_maxsz + \ > decode_renew_maxsz) > +#define NFS4_enc_gdd_getattr_sz (compound_encode_hdr_maxsz + \ > + encode_sequence_maxsz + \ > + encode_putfh_maxsz + \ > + encode_get_dir_delegation_maxsz + \ > + encode_getattr_maxsz + \ > + encode_renew_maxsz) > +#define NFS4_dec_gdd_getattr_sz (compound_decode_hdr_maxsz + \ > + decode_sequence_maxsz + \ > + decode_putfh_maxsz + \ > + decode_get_dir_delegation_maxsz + \ > + decode_getattr_maxsz + \ > + decode_renew_maxsz) > #define NFS4_enc_lookup_sz (compound_encode_hdr_maxsz + \ > encode_sequence_maxsz + \ > encode_putfh_maxsz + \ > @@ -1981,6 +2009,30 @@ static void encode_sequence(struct xdr_stream *xdr, > } > > #ifdef CONFIG_NFS_V4_1 > +static void > +encode_get_dir_delegation(struct xdr_stream *xdr, struct compound_hdr *hdr) > +{ > + __be32 *p; > + struct timespec64 ts = {}; > + u32 zerobm[1] = {}; > + > + encode_op_hdr(xdr, OP_GET_DIR_DELEGATION, decode_get_dir_delegation_maxsz, hdr); > + > + /* We can't handle CB_RECALLABLE_OBJ_AVAIL yet */ > + xdr_stream_encode_bool(xdr, false); > + > + /* for now, we request no notification types */ > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > + > + /* Request no attribute updates */ > + p = reserve_space(xdr, 12 + 12); > + p = xdr_encode_nfstime4(p, &ts); > + xdr_encode_nfstime4(p, &ts); > + > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > + xdr_encode_bitmap4(xdr, zerobm, ARRAY_SIZE(zerobm)); > +} > + > static void > encode_getdeviceinfo(struct xdr_stream *xdr, > const struct nfs4_getdeviceinfo_args *args, > @@ -2334,6 +2386,25 @@ static void nfs4_xdr_enc_getattr(struct rpc_rqst *req, struct xdr_stream *xdr, > encode_nops(&hdr); > } > > +/* > + * Encode GDD_GETATTR request > + */ > +static void nfs4_xdr_enc_gdd_getattr(struct rpc_rqst *req, struct xdr_stream *xdr, > + const void *data) > +{ > + const struct nfs4_getattr_arg *args = data; > + struct compound_hdr hdr = { > + .minorversion = nfs4_xdr_minorversion(&args->seq_args), > + }; > + > + encode_compound_hdr(xdr, req, &hdr); > + encode_sequence(xdr, &args->seq_args, &hdr); > + encode_putfh(xdr, args->fh, &hdr); > + encode_get_dir_delegation(xdr, &hdr); > + encode_getfattr(xdr, args->bitmask, &hdr); > + encode_nops(&hdr); > +} > + This function should be under a "#ifdef CONFIG_NFS_V4_1" to avoid the following compiler error: fs/nfs/nfs4xdr.c:2403:2: error: call to undeclared function 'encode_get_dir_delegation'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 2403 | encode_get_dir_delegation(xdr, &hdr); | ^ > /* > * Encode a CLOSE request > */ > @@ -5919,6 +5990,43 @@ static int decode_layout_stateid(struct xdr_stream *xdr, nfs4_stateid *stateid) > return decode_stateid(xdr, stateid); > } > > +static int decode_get_dir_delegation(struct xdr_stream *xdr, > + struct nfs4_getattr_res *res) > +{ > + nfs4_verifier cookieverf; > + int status; > + u32 bm[1]; > + > + status = decode_op_hdr(xdr, OP_GET_DIR_DELEGATION); > + if (status) > + return status; > + > + if (xdr_stream_decode_u32(xdr, &res->nf_status)) > + return -EIO; > + > + if (res->nf_status == GDD4_UNAVAIL) > + return xdr_inline_decode(xdr, 4) ? 0 : -EIO; > + > + status = decode_verifier(xdr, &cookieverf); > + if (status) > + return status; > + > + status = decode_delegation_stateid(xdr, &res->deleg); > + if (status) > + return status; > + > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > + if (status < 0) > + return status; > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > + if (status < 0) > + return status; > + status = decode_bitmap4(xdr, bm, ARRAY_SIZE(bm)); > + if (status < 0) > + return status; > + return 0; > +} > + > static int decode_getdeviceinfo(struct xdr_stream *xdr, > struct nfs4_getdeviceinfo_res *res) > { > @@ -6455,6 +6563,33 @@ static int nfs4_xdr_dec_getattr(struct rpc_rqst *rqstp, struct xdr_stream *xdr, > return status; > } > > +/* > + * Decode GDD_GETATTR response > + */ > +static int nfs4_xdr_dec_gdd_getattr(struct rpc_rqst *rqstp, struct xdr_stream *xdr, > + void *data) > +{ > + struct nfs4_getattr_res *res = data; > + struct compound_hdr hdr; > + int status; > + > + status = decode_compound_hdr(xdr, &hdr); > + if (status) > + goto out; > + status = decode_sequence(xdr, &res->seq_res, rqstp); > + if (status) > + goto out; > + status = decode_putfh(xdr); > + if (status) > + goto out; > + status = decode_get_dir_delegation(xdr, res); > + if (status) > + goto out; > + status = decode_getfattr(xdr, res->fattr, res->server); > +out: > + return status; > +} > + This needs to be under the same #ifdef, too. Thanks, Anna > /* > * Encode an SETACL request > */ > @@ -7704,6 +7839,7 @@ const struct rpc_procinfo nfs4_procedures[] = { > PROC41(BIND_CONN_TO_SESSION, > enc_bind_conn_to_session, dec_bind_conn_to_session), > PROC41(DESTROY_CLIENTID,enc_destroy_clientid, dec_destroy_clientid), > + PROC41(GDD_GETATTR, enc_gdd_getattr, dec_gdd_getattr), > PROC42(SEEK, enc_seek, dec_seek), > PROC42(ALLOCATE, enc_allocate, dec_allocate), > PROC42(DEALLOCATE, enc_deallocate, dec_deallocate), > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > index 11ad088b411d..86cbfd50ecd1 100644 > --- a/include/linux/nfs4.h > +++ b/include/linux/nfs4.h > @@ -681,6 +681,7 @@ enum { > NFSPROC4_CLNT_LISTXATTRS, > NFSPROC4_CLNT_REMOVEXATTR, > NFSPROC4_CLNT_READ_PLUS, > + NFSPROC4_CLNT_GDD_GETATTR, > }; > > /* nfs41 types */ > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index d09b9773b20c..85ee37ccc25e 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -1072,6 +1072,8 @@ struct nfs4_getattr_res { > struct nfs4_sequence_res seq_res; > const struct nfs_server * server; > struct nfs_fattr * fattr; > + nfs4_stateid deleg; > + u32 nf_status; > }; > > struct nfs4_link_arg { > > -- > 2.44.0 >
For testing purposes, add a module parameter that will prevent the client from requesting further directory delegations. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfs/nfs4proc.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 3dbe9a18c9be..a85a594cad88 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -4318,8 +4318,14 @@ static int nfs4_get_referral(struct rpc_clnt *client, struct inode *dir, return status; } +static bool nfs_dir_delegation_enabled = true; +module_param(nfs_dir_delegation_enabled, bool, 0644); +MODULE_PARM_DESC(nfs_dir_delegation_enabled, "Enable directory delegations?"); + static bool should_request_dir_deleg(struct inode *inode) { + if (!nfs_dir_delegation_enabled) + return false; if (!inode) return false; if (!S_ISDIR(inode->i_mode)) -- 2.44.0