| Message ID | 20230720-bz2223560-v1-1-edb4900043b8@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: remove unsafe BUG_ON from set_change_info | expand |
> On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > At one time, nfsd would scrape inode information directly out of struct > inode in order to populate the change_info4. At that time, the BUG_ON in > set_change_info made some sense, since having it unset meant a coding > error. > > More recently, it calls vfs_getattr to get this information, which can > fail. If that fails, fh_pre_saved can end up not being set. While this > situation is unfortunate, we don't need to crash the box. I'm always happy to get rid of a BUG_ON(). But I'm not sure even a warning is necessary in this case. It's not likely that it's a software bug or something that the server administrator can do something about. Can you elaborate on why the vfs_getattr() might fail? Eg, how was it failing in 2223560 ? > Move set_change_info to nfs4proc.c since all of the callers are there. > Revise the condition for setting "atomic" to also check for > fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it > zero out both change_attr4s when this occurs. > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 > Reported-by: Boyang Xue <bxue@redhat.com> checkpatch now wants Reported-by: Closes: in that order. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ > fs/nfsd/xdr4.h | 11 ----------- > 2 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index d8e7a533f9d2..e6f406f27821 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > return status; > } > > +/** > + * set_change_info - set up the change_info4 for a reply > + * @cinfo: pointer to nfsd4_change_info to be populated > + * @fhp: pointer to svc_fh to use as source > + * > + * Many operations in NFSv4 require change_info4 in the reply. This function > + * populates that from the info that we (should!) have already collected. In > + * the event that we didn't get any pre-attrs, throw a warning and just > + * zero out both change_attr4 fields. > + */ > +static void > +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > +{ > + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > + > + /* > + * In the event that we couldn't fetch attributes from the > + * server for some reason, just zero out the before and after "From the server"? Is it only likely to fail if the exported filesystem is an NFS mount? Or did you mean "from the filesystem" ? > + * change values, after throwing a warning. > + */ > + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { Maybe you should clear ->atomic as well in this case. > + cinfo->before_change = 0; > + cinfo->after_change = 0; > + return; > + } > + > + cinfo->before_change = fhp->fh_pre_change; > + cinfo->after_change = fhp->fh_post_change; > +} > + > static __be32 > do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) > { > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index b2931fdf53be..9e67f63c5f4d 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); > > #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) > > -static inline void > -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > -{ > - BUG_ON(!fhp->fh_pre_saved); > - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > - > - cinfo->before_change = fhp->fh_pre_change; > - cinfo->after_change = fhp->fh_post_change; > -} > - > - > bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); > bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); > bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > --- > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e > change-id: 20230720-bz2223560-9c4690a8217b > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> > -- Chuck Lever
On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: > > > On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > At one time, nfsd would scrape inode information directly out of struct > > inode in order to populate the change_info4. At that time, the BUG_ON in > > set_change_info made some sense, since having it unset meant a coding > > error. > > > > More recently, it calls vfs_getattr to get this information, which can > > fail. If that fails, fh_pre_saved can end up not being set. While this > > situation is unfortunate, we don't need to crash the box. > > I'm always happy to get rid of a BUG_ON(). But I'm not sure even > a warning is necessary in this case. It's not likely that it's > a software bug or something that the server administrator can > do something about. > > Can you elaborate on why the vfs_getattr() might fail? Eg, how > was it failing in 2223560 ? > I'm fine with dropping the WARN_ON. You are correct that there is probably little the admin can do about it. vfs_getattr can fail for all sorts of reasons. It really depends on the underlying filesystem. In 2223560, I don't know for sure, but just prior to the oops, there were these messages in the log: [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 My assumption was that the fs being shut down caused some VFS operations to start returning errors (including getattr) and that is why fh_pre_saved ultimately didn't get set. > > > Move set_change_info to nfs4proc.c since all of the callers are there. > > Revise the condition for setting "atomic" to also check for > > fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it > > zero out both change_attr4s when this occurs. > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 > > Reported-by: Boyang Xue <bxue@redhat.com> > > checkpatch now wants > > Reported-by: > Closes: > > in that order. > Mmmmkay. So I assume the URL should go in the Closes: field then? I'll take out the WARN_ON_ONCE and resend, once others have had a chance to comment. Thanks! > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ > > fs/nfsd/xdr4.h | 11 ----------- > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index d8e7a533f9d2..e6f406f27821 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > return status; > > } > > > > +/** > > + * set_change_info - set up the change_info4 for a reply > > + * @cinfo: pointer to nfsd4_change_info to be populated > > + * @fhp: pointer to svc_fh to use as source > > + * > > + * Many operations in NFSv4 require change_info4 in the reply. This function > > + * populates that from the info that we (should!) have already collected. In > > + * the event that we didn't get any pre-attrs, throw a warning and just > > + * zero out both change_attr4 fields. > > + */ > > +static void > > +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > +{ > > + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > + > > + /* > > + * In the event that we couldn't fetch attributes from the > > + * server for some reason, just zero out the before and after > > "From the server"? Is it only likely to fail if the exported > filesystem is an NFS mount? Or did you mean "from the filesystem" ? > > > > + * change values, after throwing a warning. > > + */ > > + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { > > Maybe you should clear ->atomic as well in this case. > > > > + cinfo->before_change = 0; > > + cinfo->after_change = 0; > > + return; > > + } > > + > > + cinfo->before_change = fhp->fh_pre_change; > > + cinfo->after_change = fhp->fh_post_change; > > +} > > + > > static __be32 > > do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) > > { > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index b2931fdf53be..9e67f63c5f4d 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); > > > > #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) > > > > -static inline void > > -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > -{ > > - BUG_ON(!fhp->fh_pre_saved); > > - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > - > > - cinfo->before_change = fhp->fh_pre_change; > > - cinfo->after_change = fhp->fh_post_change; > > -} > > - > > - > > bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); > > bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > --- > > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e > > change-id: 20230720-bz2223560-9c4690a8217b > > > > Best regards, > > -- > > Jeff Layton <jlayton@kernel.org> > > > > -- > Chuck Lever > >
> On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: >> >>> On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> At one time, nfsd would scrape inode information directly out of struct >>> inode in order to populate the change_info4. At that time, the BUG_ON in >>> set_change_info made some sense, since having it unset meant a coding >>> error. >>> >>> More recently, it calls vfs_getattr to get this information, which can >>> fail. If that fails, fh_pre_saved can end up not being set. While this >>> situation is unfortunate, we don't need to crash the box. >> >> I'm always happy to get rid of a BUG_ON(). But I'm not sure even >> a warning is necessary in this case. It's not likely that it's >> a software bug or something that the server administrator can >> do something about. >> >> Can you elaborate on why the vfs_getattr() might fail? Eg, how >> was it failing in 2223560 ? >> > > I'm fine with dropping the WARN_ON. You are correct that there is > probably little the admin can do about it. > > vfs_getattr can fail for all sorts of reasons. It really depends on the > underlying filesystem. In 2223560, I don't know for sure, but just prior > to the oops, there were these messages in the log: > > [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). > [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). > [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 > > My assumption was that the fs being shut down caused some VFS operations > to start returning errors (including getattr) and that is why > fh_pre_saved ultimately didn't get set. I'm wondering if the operation should just fail in this case rather than return a cobbled-up changeinfo4. Maybe for another day. >>> Move set_change_info to nfs4proc.c since all of the callers are there. >>> Revise the condition for setting "atomic" to also check for >>> fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it >>> zero out both change_attr4s when this occurs. >>> >>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 >>> Reported-by: Boyang Xue <bxue@redhat.com> >> >> checkpatch now wants >> >> Reported-by: >> Closes: >> >> in that order. >> > > > Mmmmkay. So I assume the URL should go in the Closes: field then? Yes, a bug link goes in the Closes: field. > I'll take out the WARN_ON_ONCE and resend, once others have had a chance > to comment. Don't miss the other comments below. > Thanks! > >> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ >>> fs/nfsd/xdr4.h | 11 ----------- >>> 2 files changed, 30 insertions(+), 11 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c >>> index d8e7a533f9d2..e6f406f27821 100644 >>> --- a/fs/nfsd/nfs4proc.c >>> +++ b/fs/nfsd/nfs4proc.c >>> @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> return status; >>> } >>> >>> +/** >>> + * set_change_info - set up the change_info4 for a reply >>> + * @cinfo: pointer to nfsd4_change_info to be populated >>> + * @fhp: pointer to svc_fh to use as source >>> + * >>> + * Many operations in NFSv4 require change_info4 in the reply. This function >>> + * populates that from the info that we (should!) have already collected. In >>> + * the event that we didn't get any pre-attrs, throw a warning and just >>> + * zero out both change_attr4 fields. >>> + */ >>> +static void >>> +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) >>> +{ >>> + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); >>> + >>> + /* >>> + * In the event that we couldn't fetch attributes from the >>> + * server for some reason, just zero out the before and after >> >> "From the server"? Is it only likely to fail if the exported >> filesystem is an NFS mount? Or did you mean "from the filesystem" ? >> >> >>> + * change values, after throwing a warning. >>> + */ >>> + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { >> >> Maybe you should clear ->atomic as well in this case. >> >> >>> + cinfo->before_change = 0; >>> + cinfo->after_change = 0; >>> + return; >>> + } >>> + >>> + cinfo->before_change = fhp->fh_pre_change; >>> + cinfo->after_change = fhp->fh_post_change; >>> +} >>> + >>> static __be32 >>> do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) >>> { >>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h >>> index b2931fdf53be..9e67f63c5f4d 100644 >>> --- a/fs/nfsd/xdr4.h >>> +++ b/fs/nfsd/xdr4.h >>> @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); >>> >>> #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) >>> >>> -static inline void >>> -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) >>> -{ >>> - BUG_ON(!fhp->fh_pre_saved); >>> - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); >>> - >>> - cinfo->before_change = fhp->fh_pre_change; >>> - cinfo->after_change = fhp->fh_post_change; >>> -} >>> - >>> - >>> bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); >>> bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); >>> bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); >>> >>> --- >>> base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e >>> change-id: 20230720-bz2223560-9c4690a8217b >>> >>> Best regards, >>> -- >>> Jeff Layton <jlayton@kernel.org> >>> >> >> -- >> Chuck Lever >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Thu, 2023-07-20 at 15:37 +0000, Chuck Lever III wrote: > > > On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: > > > > > > > On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > At one time, nfsd would scrape inode information directly out of struct > > > > inode in order to populate the change_info4. At that time, the BUG_ON in > > > > set_change_info made some sense, since having it unset meant a coding > > > > error. > > > > > > > > More recently, it calls vfs_getattr to get this information, which can > > > > fail. If that fails, fh_pre_saved can end up not being set. While this > > > > situation is unfortunate, we don't need to crash the box. > > > > > > I'm always happy to get rid of a BUG_ON(). But I'm not sure even > > > a warning is necessary in this case. It's not likely that it's > > > a software bug or something that the server administrator can > > > do something about. > > > > > > Can you elaborate on why the vfs_getattr() might fail? Eg, how > > > was it failing in 2223560 ? > > > > > > > I'm fine with dropping the WARN_ON. You are correct that there is > > probably little the admin can do about it. > > > > vfs_getattr can fail for all sorts of reasons. It really depends on the > > underlying filesystem. In 2223560, I don't know for sure, but just prior > > to the oops, there were these messages in the log: > > > > [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). > > [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). > > [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 > > > > My assumption was that the fs being shut down caused some VFS operations > > to start returning errors (including getattr) and that is why > > fh_pre_saved ultimately didn't get set. > > I'm wondering if the operation should just fail in this case > rather than return a cobbled-up changeinfo4. Maybe for another > day. > > > > > > Move set_change_info to nfs4proc.c since all of the callers are there. > > > > Revise the condition for setting "atomic" to also check for > > > > fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it > > > > zero out both change_attr4s when this occurs. > > > > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 > > > > Reported-by: Boyang Xue <bxue@redhat.com> > > > > > > checkpatch now wants > > > > > > Reported-by: > > > Closes: > > > > > > in that order. > > > > > > > > > Mmmmkay. So I assume the URL should go in the Closes: field then? > > Yes, a bug link goes in the Closes: field. > > > > I'll take out the WARN_ON_ONCE and resend, once others have had a chance > > to comment. > > Don't miss the other comments below. > > > > Thanks! > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ > > > > fs/nfsd/xdr4.h | 11 ----------- > > > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > > index d8e7a533f9d2..e6f406f27821 100644 > > > > --- a/fs/nfsd/nfs4proc.c > > > > +++ b/fs/nfsd/nfs4proc.c > > > > @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > return status; > > > > } > > > > > > > > +/** > > > > + * set_change_info - set up the change_info4 for a reply > > > > + * @cinfo: pointer to nfsd4_change_info to be populated > > > > + * @fhp: pointer to svc_fh to use as source > > > > + * > > > > + * Many operations in NFSv4 require change_info4 in the reply. This function > > > > + * populates that from the info that we (should!) have already collected. In > > > > + * the event that we didn't get any pre-attrs, throw a warning and just > > > > + * zero out both change_attr4 fields. > > > > + */ > > > > +static void > > > > +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > +{ > > > > + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > + > > > > + /* > > > > + * In the event that we couldn't fetch attributes from the > > > > + * server for some reason, just zero out the before and after > > > > > > "From the server"? Is it only likely to fail if the exported > > > filesystem is an NFS mount? Or did you mean "from the filesystem" ? > > > Yep, will fix. > > > > > > > + * change values, after throwing a warning. > > > > + */ > > > > + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { > > > > > > Maybe you should clear ->atomic as well in this case. > > > > > > It should already be clear since fh_pre_saved isn't set. > > > > + cinfo->before_change = 0; > > > > + cinfo->after_change = 0; > > > > + return; > > > > + } > > > > + > > > > + cinfo->before_change = fhp->fh_pre_change; > > > > + cinfo->after_change = fhp->fh_post_change; > > > > +} > > > > + > > > > static __be32 > > > > do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) > > > > { > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > > index b2931fdf53be..9e67f63c5f4d 100644 > > > > --- a/fs/nfsd/xdr4.h > > > > +++ b/fs/nfsd/xdr4.h > > > > @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); > > > > > > > > #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) > > > > > > > > -static inline void > > > > -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > -{ > > > > - BUG_ON(!fhp->fh_pre_saved); > > > > - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > - > > > > - cinfo->before_change = fhp->fh_pre_change; > > > > - cinfo->after_change = fhp->fh_post_change; > > > > -} > > > > - > > > > - > > > > bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); > > > > bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > > > > > --- > > > > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e > > > > change-id: 20230720-bz2223560-9c4690a8217b > > > > > > > > Best regards, > > > > -- > > > > Jeff Layton <jlayton@kernel.org> > > > > > > > > > > -- > > > Chuck Lever > > > > > > > > > > -- > > Jeff Layton <jlayton@kernel.org> > > > -- > Chuck Lever > >
On Thu, 2023-07-20 at 15:37 +0000, Chuck Lever III wrote: > > > On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: > > > > > > > On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > At one time, nfsd would scrape inode information directly out of struct > > > > inode in order to populate the change_info4. At that time, the BUG_ON in > > > > set_change_info made some sense, since having it unset meant a coding > > > > error. > > > > > > > > More recently, it calls vfs_getattr to get this information, which can > > > > fail. If that fails, fh_pre_saved can end up not being set. While this > > > > situation is unfortunate, we don't need to crash the box. > > > > > > I'm always happy to get rid of a BUG_ON(). But I'm not sure even > > > a warning is necessary in this case. It's not likely that it's > > > a software bug or something that the server administrator can > > > do something about. > > > > > > Can you elaborate on why the vfs_getattr() might fail? Eg, how > > > was it failing in 2223560 ? > > > > > > > I'm fine with dropping the WARN_ON. You are correct that there is > > probably little the admin can do about it. > > > > vfs_getattr can fail for all sorts of reasons. It really depends on the > > underlying filesystem. In 2223560, I don't know for sure, but just prior > > to the oops, there were these messages in the log: > > > > [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). > > [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). > > [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 > > > > My assumption was that the fs being shut down caused some VFS operations > > to start returning errors (including getattr) and that is why > > fh_pre_saved ultimately didn't get set. > > I'm wondering if the operation should just fail in this case > rather than return a cobbled-up changeinfo4. Maybe for another > day. > If we issue a create or rename or something and it did happen, then we don't want to return an error just because we couldn't collect the info on it. We probably could abort the operation if collecting the pre-change info fails though. > > > > > Move set_change_info to nfs4proc.c since all of the callers are there. > > > > Revise the condition for setting "atomic" to also check for > > > > fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it > > > > zero out both change_attr4s when this occurs. > > > > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 > > > > Reported-by: Boyang Xue <bxue@redhat.com> > > > > > > checkpatch now wants > > > > > > Reported-by: > > > Closes: > > > > > > in that order. > > > > > > > > > Mmmmkay. So I assume the URL should go in the Closes: field then? > > Yes, a bug link goes in the Closes: field. > > > > I'll take out the WARN_ON_ONCE and resend, once others have had a chance > > to comment. > > Don't miss the other comments below. > > > > Thanks! > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ > > > > fs/nfsd/xdr4.h | 11 ----------- > > > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > > index d8e7a533f9d2..e6f406f27821 100644 > > > > --- a/fs/nfsd/nfs4proc.c > > > > +++ b/fs/nfsd/nfs4proc.c > > > > @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > return status; > > > > } > > > > > > > > +/** > > > > + * set_change_info - set up the change_info4 for a reply > > > > + * @cinfo: pointer to nfsd4_change_info to be populated > > > > + * @fhp: pointer to svc_fh to use as source > > > > + * > > > > + * Many operations in NFSv4 require change_info4 in the reply. This function > > > > + * populates that from the info that we (should!) have already collected. In > > > > + * the event that we didn't get any pre-attrs, throw a warning and just > > > > + * zero out both change_attr4 fields. > > > > + */ > > > > +static void > > > > +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > +{ > > > > + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > + > > > > + /* > > > > + * In the event that we couldn't fetch attributes from the > > > > + * server for some reason, just zero out the before and after > > > > > > "From the server"? Is it only likely to fail if the exported > > > filesystem is an NFS mount? Or did you mean "from the filesystem" ? > > > > > > > > > > + * change values, after throwing a warning. > > > > + */ > > > > + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { > > > > > > Maybe you should clear ->atomic as well in this case. > > > > > > > > > > + cinfo->before_change = 0; > > > > + cinfo->after_change = 0; > > > > + return; > > > > + } > > > > + > > > > + cinfo->before_change = fhp->fh_pre_change; > > > > + cinfo->after_change = fhp->fh_post_change; > > > > +} > > > > + > > > > static __be32 > > > > do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) > > > > { > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > > index b2931fdf53be..9e67f63c5f4d 100644 > > > > --- a/fs/nfsd/xdr4.h > > > > +++ b/fs/nfsd/xdr4.h > > > > @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); > > > > > > > > #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) > > > > > > > > -static inline void > > > > -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > > > > -{ > > > > - BUG_ON(!fhp->fh_pre_saved); > > > > - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > > > - > > > > - cinfo->before_change = fhp->fh_pre_change; > > > > - cinfo->after_change = fhp->fh_post_change; > > > > -} > > > > - > > > > - > > > > bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); > > > > bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr); > > > > > > > > --- > > > > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e > > > > change-id: 20230720-bz2223560-9c4690a8217b > > > > > > > > Best regards, > > > > -- > > > > Jeff Layton <jlayton@kernel.org> > > > > > > > > > > -- > > > Chuck Lever > > > > > > > > > > -- > > Jeff Layton <jlayton@kernel.org> > > > -- > Chuck Lever > >
On Thu, 2023-07-20 at 15:37 +0000, Chuck Lever III wrote: > > > On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote: > > > > > > > On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > At one time, nfsd would scrape inode information directly out of struct > > > > inode in order to populate the change_info4. At that time, the BUG_ON in > > > > set_change_info made some sense, since having it unset meant a coding > > > > error. > > > > > > > > More recently, it calls vfs_getattr to get this information, which can > > > > fail. If that fails, fh_pre_saved can end up not being set. While this > > > > situation is unfortunate, we don't need to crash the box. > > > > > > I'm always happy to get rid of a BUG_ON(). But I'm not sure even > > > a warning is necessary in this case. It's not likely that it's > > > a software bug or something that the server administrator can > > > do something about. > > > > > > Can you elaborate on why the vfs_getattr() might fail? Eg, how > > > was it failing in 2223560 ? > > > > > > > I'm fine with dropping the WARN_ON. You are correct that there is > > probably little the admin can do about it. > > > > vfs_getattr can fail for all sorts of reasons. It really depends on the > > underlying filesystem. In 2223560, I don't know for sure, but just prior > > to the oops, there were these messages in the log: > > > > [51935.482019] XFS (vda3): Filesystem has been shut down due to log error (0x2). > > [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the problem(s). > > [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, sector 58684120 > > > > My assumption was that the fs being shut down caused some VFS operations > > to start returning errors (including getattr) and that is why > > fh_pre_saved ultimately didn't get set. > > I'm wondering if the operation should just fail in this case > rather than return a cobbled-up changeinfo4. Maybe for another > day. > Actually, this doesn't look too hard to do. We should be able to just unwind and return an error in all cases if collecting pre_op_attrs fails. The trickier bit is what to do if collecting post_op_attrs fails after collecting pre-op attrs and the operation itself succeeded. What should go into the after_change value? 0? Should we just copy the before_change value?
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index d8e7a533f9d2..e6f406f27821 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, return status; } +/** + * set_change_info - set up the change_info4 for a reply + * @cinfo: pointer to nfsd4_change_info to be populated + * @fhp: pointer to svc_fh to use as source + * + * Many operations in NFSv4 require change_info4 in the reply. This function + * populates that from the info that we (should!) have already collected. In + * the event that we didn't get any pre-attrs, throw a warning and just + * zero out both change_attr4 fields. + */ +static void +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) +{ + cinfo->atomic = (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !fhp->fh_no_atomic_attr); + + /* + * In the event that we couldn't fetch attributes from the + * server for some reason, just zero out the before and after + * change values, after throwing a warning. + */ + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) { + cinfo->before_change = 0; + cinfo->after_change = 0; + return; + } + + cinfo->before_change = fhp->fh_pre_change; + cinfo->after_change = fhp->fh_post_change; +} + static __be32 do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) { diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index b2931fdf53be..9e67f63c5f4d 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op); #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs) -static inline void -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) -{ - BUG_ON(!fhp->fh_pre_saved); - cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); - - cinfo->before_change = fhp->fh_pre_change; - cinfo->after_change = fhp->fh_post_change; -} - - bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp); bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_stream *xdr); bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr);
At one time, nfsd would scrape inode information directly out of struct inode in order to populate the change_info4. At that time, the BUG_ON in set_change_info made some sense, since having it unset meant a coding error. More recently, it calls vfs_getattr to get this information, which can fail. If that fails, fh_pre_saved can end up not being set. While this situation is unfortunate, we don't need to crash the box. Move set_change_info to nfs4proc.c since all of the callers are there. Revise the condition for setting "atomic" to also check for fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it zero out both change_attr4s when this occurs. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2223560 Reported-by: Boyang Xue <bxue@redhat.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++ fs/nfsd/xdr4.h | 11 ----------- 2 files changed, 30 insertions(+), 11 deletions(-) --- base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e change-id: 20230720-bz2223560-9c4690a8217b Best regards,