netdev - Re: [PATCH net-next v2 3/5] netconsole: add support for sysdata and CPU population

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] [day] [month] [year] [list]

Message-ID: <20250124-red-crab-of-mastery-23bc19@leitao>
Date: Fri, 24 Jan 2025 07:28:28 -0800
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
	Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kselftest@...r.kernel.org, kernel-team@...a.com,
	max@...sevol.com, thepacketgeek@...il.com
Subject: Re: [PATCH net-next v2 3/5] netconsole: add support for sysdata and
 CPU population

Hello Jakub,

On Mon, Jan 20, 2025 at 11:06:53AM -0800, Jakub Kicinski wrote:
> >
> > The only way to do it properly is having a extra buffer where we
> > have `cpu=42` and copy 5 bytes from there, and then copy the last one in
> > the next iteration. I am not sure we can do it in one shot.
> 
> FWIW to simplify reasoning about the length I thought we could take the
> worst case, assume we'll need len(cpu=) + log10(nr_cpu_ids) of space.

We can do that, but, we are going to come back to this discussion again
as soon we expand sysdata. For instance, I have plans to expand it to
have task_struct->comm, release, etc.

For that, we need to know the length of the struct ahead of time

> > 1) Average messages size will become bigger. Thus, memcpy will be needed
> > one way or another.
> > 
> > 2) Unless we can come up with a smart solution, this solution will be
> > harder to reason about.
> > 
> > If you want to invest more time in this direction, I am more than happy
> > to create a PoC, so we can discuss more concretely. 
> 
> I don't feel super strongly about this. But hacking around is always
> good to get a sense of how hairy the implementation ends up being.
> 
> 
> To rephrase my concern is that we have some data as static on the
> stack, some dynamically appended at the send_*() stage, now we're
> adding a third way of handling things. Perhaps the simplest way to
> make me happy would be to move the bufs which are currently static 
> into nt.

I've hacked it, and I think I addressed most of these concerns. This is
how the new RFC is:

1) moved the buffer to netconsole_target. no more static buffer.
2) created a function called prepare_extradata(), which will handle
   sysdata and userdata.
	2.1) to be fair, userdata is already in the temporary buffer
	  (extradata_complete) since it doesn't change frequently, only
	  when configfs helpers are called. We can parse configfs nodes
	  to generate it in runtime, but, this will be unnecessary.

3) prepare_extradata() is called once at the send path.

I've just sent an RFC (v3) with the full changes, let's see if it
improves your concerns.

https://lore.kernel.org/all/20250124-netcon_cpu-v3-0-12a0d286ba1d@debian.org/

Again, thanks for reviewing this change,
--breno