netdev - Re: [PATCH net-next v2 05/10] tools/net/ynl: Refactor decode_fixed

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]

Date: Fri, 18 Aug 2023 11:21:40 +0100
From: Donald Hunter <donald.hunter@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Donald Hunter <donald.hunter@...il.com>, netdev@...r.kernel.org, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org, 
	Stanislav Fomichev <sdf@...gle.com>, Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Subject: Re: [PATCH net-next v2 05/10] tools/net/ynl: Refactor
 decode_fixed_header into NlMsg

On Fri, 18 Aug 2023 at 02:37, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 17 Aug 2023 16:14:35 +0100 Donald Hunter wrote:
> > > It's a bit of a layering violation that we are futzing with the raw
> > > member of NlMsg inside GenlMsg, no?
> > >
> > > Should we add "fixed hdrs len" argument to NlMsg? Either directly or
> > > pass ynl and let get the expected len from ynl? That way NlMsg can
> > > split itself into hdr, userhdrs and attrs without GenlMsg "fixing it
> > > up"?
> >
> > I agree, it breaks the layering. The issue is that GenlMsg gets created at
> > some point after NlMsg, only when we know the nl_msg is suitable for
> > decoding. The fixed header bit is quite well encapsulated in NlMsg,
> > it's the genl header that needs pulled out and NlMsg shouldn't know
> > anything about it. How about I add a take_bytes(length) method or a
> > generic decode_subheader(format, length) method to NlMsg?
>
> Why do we need to fix up the .raw of NlMsg underlying the GenlMsg
> in the first place? GenlMsg by itself didn't need to do that until now.

Fair point. I will refactor to leave nl_msg.raw untouched.

> Another option to consider which would make things more symmetric
> between raw and genetlink would be to add a wrapper class for old
> families, too. ClassicMsg? CnlMsg? That way we could retain the
> separation of NlMsg is just a raw message which could be a NLM_DONE or
> some other control thing, and higher level class being used to pull
> fixed headers and separate out attrs. Just a thought, not sure it helps.

I _think_ I can avoid doing this. There's an asymmetry to the way
the NlAttrs get created that I need to fix. When I do that, the rest
should be a bit cleaner.