netdev - Re: [PATCH net-next 2/4] netconsole: Add option to auto-populate CPU number in userdata

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: <20241118183336.34e42b01@kernel.org>
Date: Mon, 18 Nov 2024 18:33:36 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Breno Leitao <leitao@...ian.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, max@...sevol.com, thepacketgeek@...il.com,
 vlad.wing@...il.com, davej@...emonkey.org.uk
Subject: Re: [PATCH net-next 2/4] netconsole: Add option to auto-populate
 CPU number in userdata

Sorry for the late review, I think this will miss v6.13 :(

On Wed, 13 Nov 2024 07:10:53 -0800 Breno Leitao wrote:
>  /**
>   * struct netconsole_target - Represents a configured netconsole target.
>   * @list:	Links this target into the target_list.
> @@ -97,6 +105,7 @@ static struct console netconsole_ext;
>   * @userdata_group:	Links to the userdata configfs hierarchy
>   * @userdata_complete:	Cached, formatted string of append
>   * @userdata_length:	String length of userdata_complete
> + * @userdata_auto:	Kernel auto-populated bitwise fields in userdata.
>   * @enabled:	On / off knob to enable / disable target.
>   *		Visible from userspace (read-write).
>   *		We maintain a strict 1:1 correspondence between this and
> @@ -123,6 +132,7 @@ struct netconsole_target {
>  	struct config_group	userdata_group;
>  	char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
>  	size_t			userdata_length;
> +	enum userdata_auto	userdata_auto;

If you want to set multiple bits here type should probably be unsigned
long. Otherwise the enum will contain combination of its values, which
are in themselves not valid enum values ... if that makes sense.

>  #endif
>  	bool			enabled;
>  	bool			extended;

> +	/* Check if CPU NR should be populated, and append it to the user
> +	 * dictionary.
> +	 */
> +	if (child_count < MAX_USERDATA_ITEMS && nt->userdata_auto & AUTO_CPU_NR)
> +		scnprintf(&nt->userdata_complete[complete_idx],
> +			  MAX_USERDATA_ENTRY_LENGTH, " cpu=%u\n",
> +			  raw_smp_processor_id());

I guess it may be tricky for backward compat, but shouldn't we return
an error rather than silently skip?

>  	nt->userdata_length = strnlen(nt->userdata_complete,
>  				      sizeof(nt->userdata_complete));
>  }
> @@ -757,7 +788,36 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
>  	return ret;
>  }
>  
> +static ssize_t populate_cpu_nr_store(struct config_item *item, const char *buf,
> +				     size_t count)
> +{
> +	struct netconsole_target *nt = to_target(item->ci_parent);
> +	bool cpu_nr_enabled;
> +	ssize_t ret;
> +
> +	if (!nt)
> +		return -EINVAL;

Can this happen? Only if function gets called with a NULL @item
which would be pretty nutty.

> +	mutex_lock(&dynamic_netconsole_mutex);
> +	ret = kstrtobool(buf, &cpu_nr_enabled);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (cpu_nr_enabled)
> +		nt->userdata_auto |= AUTO_CPU_NR;
> +	else
> +		nt->userdata_auto &= ~AUTO_CPU_NR;
> +
> +	update_userdata(nt);
> +
> +	ret = strnlen(buf, count);
> +out_unlock:
> +	mutex_unlock(&dynamic_netconsole_mutex);
> +	return ret;
> +}