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]

Message-ID: <20230817183751.194d547a@kernel.org>
Date: Thu, 17 Aug 2023 18:37:51 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Donald Hunter <donald.hunter@...hat.com>
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 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.

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.