Message-ID: <d666a76bd243d4299551381523698a47229df71b.camel@kernel.org>
Date:   Thu, 20 Jul 2023 11:43:39 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Boyang Xue <bxue@...hat.com>
Subject: Re: [PATCH] nfsd: remove unsafe BUG_ON from set_change_info

On Thu, 2023-07-20 at 15:37 +0000, Chuck Lever III wrote:
> 
> > On Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@...nel.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@...nel.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@...hat.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@...nel.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@...nel.org>
> > > > 
> > > 
> > > --
> > > Chuck Lever
> > > 
> > > 
> > 
> > -- 
> > Jeff Layton <jlayton@...nel.org>
> 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists