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: <CAAf2yc=C4N=QT6hpxmLi-uywc+MhuLdAsbfPRORv5ms9k1JAfQ@mail.gmail.com>
Date: Thu, 17 Aug 2023 16:14:35 +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 Wed, 16 Aug 2023 at 16:20, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 15 Aug 2023 20:42:49 +0100 Donald Hunter wrote:
> > +    def __init__(self, nl_msg, ynl=None):
> > +        self.genl_cmd, self.genl_version, _ = struct.unpack_from("BBH", nl_msg.raw, 0)
> > +        nl_msg.raw = nl_msg.raw[4:]
>
> 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?