netdev - Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

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: <20240826143546.77669b47@kernel.org>
Date: Mon, 26 Aug 2024 14:35:46 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Maksym Kutsevol <max@...sevol.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Breno Leitao
 <leitao@...ian.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

On Sat, 24 Aug 2024 14:50:24 -0700 Maksym Kutsevol wrote:
> Enhance observability of netconsole. UDP sends can fail. Start tracking at

nit: "UDP sends" sounds a bit too much like it's using sockets
maybe "packet sends"?

> least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> Stats are exposed via an additional attribute in CONFIGFS.

Please provide a reference to another configfs user in the kernel which
exposes stats. To help reviewers validate it's a legit use case.

> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +struct netconsole_target_stats  {
> +	size_t xmit_drop_count;
> +	size_t enomem_count;
> +};
> +#endif

Don't hide types under ifdefs
In fact I'm not sure if hiding stats if DYNAMIC isn't enabled makes
sense. They don't take up much space.

> +static ssize_t stats_show(struct config_item *item, char *buf)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +
> +	return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
> +		nt->stats.xmit_drop_count, nt->stats.enomem_count);

does configfs require value per file like sysfs or this is okay?

>  /**
>   * send_ext_msg_udp - send extended log message to target
>   * @nt: target to send message to
> @@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
>  					     "%s", userdata);
>  
>  		msg_ready = buf;
> -		netpoll_send_udp(&nt->np, msg_ready, msg_len);
> +		count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
> +							  msg_ready,
> +							  msg_len));

Please add a wrapper which calls netpoll_send_udp() and counts the
stats. This sort of nested function calls are unlikely to meet kernel
coding requirements.
-- 
pw-bot: cr