linux-kernel - Re: [PATCH v3 03/13] nfsd: drop the ncf_cb

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Zth6oPq1GV2iiypL@tissot.1015granger.net>
Date: Wed, 4 Sep 2024 11:20:00 -0400
From: Chuck Lever <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>,
        Trond Myklebust <trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>,
        Olga Kornievskaia <okorniev@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Jonathan Corbet <corbet@....net>, Tom Haynes <loghyr@...il.com>,
        linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> This is always the same value, and in a later patch we're going to need
> to set bits in WORD2. We can simplify this code and save a little space
> in the delegation too. Just hardcode the bitmap in the callback encode
> function.
> 
> Signed-off-by: Jeff Layton <jlayton@...nel.org>

OK, lurching forward on this ;-)

 - The first patch in v3 was applied to v6.11-rc.
 - The second patch is now in nfsd-next.
 - I've squashed the sixth and eighth patches into nfsd-next.

I'm replying to this patch because this one seems like a step
backwards to me: the bitmap values are implementation-dependent,
and this code will eventually be automatically generated based only
on the protocol, not the local implementation. IMO, architecturally,
bitmap values should be set at the proc layer, not in the encoders.

I've gone back and forth on whether to just go with it for now, but
I thought I'd mention it here to see if this change is truly
necessary for what follows. If it can't be replaced, I will suck it
up and fix it up later when this encoder is converted to an xdrgen-
manufactured one.

I've tried to apply a few of the following patches in this series,
but by 9/13, things stop compiling. So I will let you rebase your
work on the current nfsd-next and redrive it, and we can go from
there.


> ---
>  fs/nfsd/nfs4callback.c | 5 ++++-
>  fs/nfsd/nfs4state.c    | 1 -
>  fs/nfsd/state.h        | 1 -
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index b5b3ab9d719a..0c49e31d4350 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -364,10 +364,13 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
>  	struct nfs4_delegation *dp =
>  		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
>  	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
> +	u32 bmap[1];
> +
> +	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
>  
>  	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
>  	encode_nfs_fh4(xdr, fh);
> -	encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> +	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
>  	hdr->nops++;
>  }
>  
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8835930ecee6..6844ae9ea350 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1183,7 +1183,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>  	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
>  			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
>  	dp->dl_cb_fattr.ncf_file_modified = false;
> -	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
>  	get_nfs4_file(fp);
>  	dp->dl_stid.sc_file = fp;
>  	return dp;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 79c743c01a47..ac3a29224806 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -138,7 +138,6 @@ struct nfs4_cpntf_state {
>  struct nfs4_cb_fattr {
>  	struct nfsd4_callback ncf_getattr;
>  	u32 ncf_cb_status;
> -	u32 ncf_cb_bmap[1];
>  
>  	/* from CB_GETATTR reply */
>  	u64 ncf_cb_change;
> 
> -- 
> 2.46.0
> 



-- 
Chuck Lever