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: <B222389A-F6FA-4090-B420-A9DDCD454139@oracle.com>
Date: Mon, 19 Aug 2024 23:16:21 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>
CC: Neil Brown <neilb@...e.de>, Dai Ngo <dai.ngo@...cle.com>,
        Olga
 Kornievskaia <okorniev@...hat.com>, Tom Talpey <tom@...pey.com>,
        Trond
 Myklebust <trondmy@...nel.org>,
        Anna Schumaker <anna@...nel.org>, Tom Haynes
	<loghyr@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>
Subject: Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding



> On Aug 19, 2024, at 4:04 PM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> On Mon, 2024-08-19 at 19:50 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@...nel.org>
>>> wrote:
>>> 
>>> I'm playing with the new version now and it seems to be much
>>> improved.
>>> Only two real bugs I've hit at this point:
>>> 
>>> 1/ Some of the struct specifications need to be typedefs as well.
>>> For
>>> instance, the delstid draft refers to "nfstime4", but the
>>> autogenerated
>>> struct definition doesn't have the typedef for it. It may be best
>>> to
>>> just add typedefs for all of these sorts of structs.
>> 
>> What's the specific symptom? I've been able to catenate nfs4_1.x
>> and delstid.x, xdrgen builds the header and source without tossing
>> any exceptions, and gcc compiles it without complaint.
>> 
> 
> 
> Basically, I was getting this when I'd convert nfs4_1.x to a header:
> 
> struct nfstime4 {
>        int64_t seconds;
>        uint32_t nseconds;
> };
> 
> ...but the delstid header has these:
> 
> typedef nfstime4 fattr4_time_deleg_access;
> 
> typedef nfstime4 fattr4_time_deleg_modify;
> 
> 
> ...nothing defined nfstime4 in this case.
> 
>> AFAICT, xdrgen will add "struct" where it's necessary.
>> 
>> I've been squirrelly about using "typedef" too often because
>> the Linux kernel's coding style is to avoid C typedefs for
>> shorthand structure names.
>> 
> 
> Oh, ok. I didn't concatenate the files like you did and just generated
> the delstid files separately from the nfs4_1 ones. I guess that throws
> off the dependency tracking that you're doing here for typedefs.

cat'ing the two files together is the spec-recommended approach,
but it assumes you're generating the whole protocol at once.
Here it was just a quick and dirty way for me to build a
reproducer.

For an initial fs/nfsd/nfs4_1.x file, I recommend starting with
delstid.x, and then add the pieces of the NFSv4_1 XDR until
xdrgen and gcc can make proper sense of it.

I can take a stab at that if you like, and send you something
tomorrow?


Sidebar: We could go with all typedefs for structs, unions, and
enums. That would make C code generation easier. Something like:

typedef struct {
	int64_t seconds;
	uint32_t nseconds;
} nfstime4;

But like I said, I expect that approach might be frowned upon.


>>> 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
>>> autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
>>> xdrgen_encode_fattr4_time_deleg_modify try to pass it by value
>>> instead.
>> 
>> Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:
>> 
>> /* typedef fattr4_time_deleg_access */
>> static bool
>> __maybe_unused                                                     
>> xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const
>> fattr4_time_deleg_access value)
>> {
>> /* (basic) */
>> return xdrgen_encode_nfstime4(xdr, &value);
>> };
>> 
>> Looks like it does the right thing...?
> 
> Probably another side-effect of it not knowing what to do with nfstime4
> when I convert the delstid draft. Concatenating them seems unwieldy but
> I guess that would work. I do like being able to keep generated code
> from different files separate though.

I don't think cat'ing the .x files is /required/, but it was a
quick way to get started.

Having a working nfs4_1.x that can generate the small piece of
XDR code that we need, in a separate file that can be augmented
over time, I think, is a win. I don't see that anything so far
is preventing that.

--
Chuck Lever