linux-kernel - Re: [PATCH] nfsd: remove unsafe BUG_ON from set_change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B067A0F-93E3-435A-A32B-B17BC07D4606@oracle.com>
Date:   Thu, 20 Jul 2023 15:15:41 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
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 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 ?


> 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.


> 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