Re: [PATCH] nfsd: remove unsafe BUG_ON from set_change_info

[Chuck Lever III]

> On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> 
>> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>


--
Chuck Lever