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: <20250117-terrestrial-clam-of-satiation-cf312f@leitao>
Date: Fri, 17 Jan 2025 03:02:40 -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 Thu, Jan 16, 2025 at 05:44:05PM -0800, Jakub Kicinski wrote:
> On Wed, 15 Jan 2025 05:35:20 -0800 Breno Leitao wrote:
> > +	WARN_ON_ONCE(userdata_len + sysdata_len >
> > +		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);
> > +
> > +	/* nt->sysdata_length will be used later to decide if the message
> > +	 * needs to be fragmented.
> > +	 * userdata_len cannot be used for it, once next sysdata append should
> > +	 * start from the same userdata_len location, and only overwrite old
> > +	 * sysdata.
> > +	 */
> > +	nt->sysdata_length = sysdata_len;
> 
> Updating nt-> fields at runtime is something we haven't done before,
> right?

Correct. nt-> fields were only updated by configfs helpers.

> What's the locking? We depend on target_list_lock ?

Correct. "Runtime updates" (aka nt->sysdata_length) always occur within
the target_list_lock().

At the same time, userdata updates (aka nt->userdata_length) happen
inside dynamic_netconsole_mutex().

The weirdness here is that:

	1) Writers of nt->sysdata_length hold dynamic_netconsole_mutex()

	2) Readers of nt->sysdata_length hold target_list_lock()

	3) There is no dependency between target_list_lock() and
	dynamic_netconsole_mutex()

	4) Creating a dependency on target_list_lock() in configfs could lead to
	potential DoS attacks by userspace holding target_list_lock() for
	extended periods, starving netconsole.

	5) A possible solution might involve using read-write or RCU locks.


> Looks like previously all the data was on the stack, now we have a mix.

Not sure I followed. The data ({userdata,extradata}_complete) was always
inside nt field, which belongs to target_list.

{userdata,extradata}_complete was always in the stack. It is allocated
in the following way:


	struct netconsole_target {
		...
		char extradata_complete[MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS];
	}

	static struct netconsole_target *alloc_and_init(void) {
		struct netconsole_target *nt;
		nt = kzalloc(sizeof(*nt), GFP_KERNEL);
		...
		return nt
	}

	static struct netconsole_target *alloc_param_target(char *target_config,
							    int cmdline_count) {
		nt = alloc_and_init();
		....
		return nt;
	}

	static int __init init_netconsole(void) {
		nt = alloc_param_target(target_config, count);
		...
		list_add(&nt->list, &target_list);
	}

> Maybe we can pack all the bits of state into a struct for easier
> passing around, but still put it on the stack?

It depends on what state you need here. We can certainly pass runtime
(aka sysdata in this patchset) data in the stack, but doing the same for
userdata would require extra computation in runtime. In other words, the
userdata_complete and length are calculated at configfs update time
today, and only read during runtime, and there is no connection between
configfs and runtime (write_ext_msg()) except through the stack.


On the other side, if we want to have extradata_complete in the stack, I
still think that userdata will need to be in the stack, and create a
buffer in runtime's frame and copy userdata + sysdata at run time, doing
an extra copy. 

Trying to put this in code, this is what I thought:

	/* Copy to the stack (buf) the userdata string + sysdata */
	static void append_runtime_sysdata(struct netconsole_target *nt, char *buf) {
		if (!(nt->sysdata_fields & CPU_NR))
			return;

		return scnprintf(buf,  MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS,
				  "%s cpu=%u\n", nt->userdata_complete, raw_smp_processor_id());
	}

	/* Move complete string in the stack and send from there */
	static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
				     int msg_len) {
		...
	#ifdef CONFIG_NETCONSOLE_DYNAMIC
		struct char buf[MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS];
		extradata_len = append_runtime_sysdata(nt, buf);
	#endif

		send_msg_{no}_fragmentation(nt, msg, buf, extradata_len, release_len)
		...
	}


Thank for the review and the help with the design,
--breno