linux-kernel - Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

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: <Zr97bLdCvjD69yW7@tissot.1015granger.net>
Date: Fri, 16 Aug 2024 12:16:44 -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>,
        Tom Haynes <loghyr@...il.com>, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org
Subject: Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

On Fri, Aug 16, 2024 at 11:45:32AM -0400, Jeff Layton wrote:
> On Fri, 2024-08-16 at 11:17 -0400, Chuck Lever wrote:
> > On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> > > This adds support for the "delstid" draft:
> > > 
> > >     https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> > > 
> > > Most of this was autogenerated using Chuck's lkxdrgen tool with
> > > some
> > > by-hand tweaks to work around some symbol conflicts, and to add
> > > some
> > > missing pieces that were needed from nfs4_1.x.
> > 
> > I haven't read delstid closely enough to comment on the approach
> > you've taken in 3/3, but I do have some thoughts about code
> > organization here. I will try to study that draft soon.
> > 
> > And, I'm assuming you are continuing to evolve support for the draft
> > and will be growing this series. So I will hold off on careful
> > inspection of 3/3 for the moment.
> > 
> 
> Yes. The client pieces are already in place, and I read I get is that
> the draft is all but done at this point. 3/3 is probably pretty close
> to what should go in. There are really 3 parts to the delstid draft:
> 
> 1/ the OPEN_XOR_DELEGATION part, which allows the server to avoid
> giving out an open stateid when giving out a deleg.
> 
> 2/ delegated timestamp support (which is the part I'm still looking at)
> 
> 3/ FATTR4_OPEN_ARGUMENTS (which enables 1 and 2)
> 
> This patchset encompasses 1 & 3. Part 2 shouldn't have much bearing on
> this set. It's really a separate feature entirely that just happens to
> also depend on FATTR4_OPEN_ARGUMENTS support.
> 
> > First, I'm pleased that you found xdrgen useful for rapid
> > prototyping. That's not something I had envisioned when I created
> > the tool, but it's a good match, looks like.
> > 
> 
> Yeah. It has some bugs that still need fixing, but it was certainly
> better than having to roll all of that by hand.

I'm very interested to hear bug reports.


> > Here you add a separate set of source files for delstid XDR...  That
> > approach might not be scalable for adding subsequent new features in
> > general, it occurs to me.
> >
> > We might end up with a bunch of these little code blurbs with no
> > clear understanding of how they inter-relate.  Thoughts about how to
> > manage these are welcome: xdrgen could generate only header files
> > and then we would #include them where needed, for example.
> >
> > For now, I suggest folding the new generated XDR code into the
> > existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you have
> > some time for cleaning up the patches. An alternative would be to
> > leave it and I can fold these together before committing.
> > 
> > (The long term, of course, will hopefully be generating all XDR code
> > automatically, but we're a ways out from that, IMO).
> > 
> 
> This is where I disagree.
> 
> The nice thing about lkxdrgen is that most of what it generates is
> fairly self-contained. I think we ought to try to keep the new (mostly
> autogenerated) and old code (hand-rolled) separate for now.
> 
> I'm not sure what makes the most sense for a new naming scheme, but I
> really don't think we should paste all of this into nfs4xdr.c, which is
> just a huge jumble of hand-rolled XDR code. 

nfs4xdr.c is a mix of stuff that I constructed by rote, which is
pretty clean, and stuff that mixes the "just serialize" logic with
"do the proc part as well" logic, which is more ugly. I had thought
that OPEN's XDR was in the former category, but I get it. 

So I still think there's a scalability problem with adding each new
feature in its own XDR .c and .h, but I don't mind keeping the
generated code separate from the legacy code. How about creating one
new file to collect the mostly- or all-generated XDR code?

I've been using fs/nfsd/nfs[34]gen_xdr.[ch] in my testing.

-- 
Chuck Lever