netdev - Re: [PATCH iwl-next v3 3/6] devlink: add devlink_fmsg_dump

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: <20240822104007.GL2164@kernel.org>
Date: Thu, 22 Aug 2024 11:40:07 +0100
From: Simon Horman <horms@...nel.org>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, Jiri Pirko <jiri@...nulli.us>,
	Tony Nguyen <anthony.l.nguyen@...el.com>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Jakub Kicinski <kuba@...nel.org>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com, apw@...onical.com,
	joe@...ches.com, dwaipayanray1@...il.com, lukas.bulwahn@...il.com,
	akpm@...ux-foundation.org, willemb@...gle.com,
	Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Subject: Re: [PATCH iwl-next v3 3/6] devlink: add devlink_fmsg_dump_skb()
 function

On Wed, Aug 21, 2024 at 03:37:11PM +0200, Przemek Kitszel wrote:
> From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> 
> Add devlink_fmsg_dump_skb() function that adds some diagnostic
> information about skb (like length, pkt type, MAC, etc) to devlink
> fmsg mechanism using bunch of devlink_fmsg_put() function calls.
> 
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

...

> diff --git a/net/devlink/health.c b/net/devlink/health.c
> index acb8c0e174bb..b98ca650284c 100644
> --- a/net/devlink/health.c
> +++ b/net/devlink/health.c
> @@ -1241,3 +1241,70 @@ int devlink_nl_health_reporter_test_doit(struct sk_buff *skb,
>  
>  	return reporter->ops->test(reporter, info->extack);
>  }
> +
> +/**
> + * devlink_fmsg_dump_skb - Dump sk_buffer structure
> + * @fmsg: devlink formatted message pointer
> + * @skb: pointer to skb
> + *
> + * Dump diagnostic information about sk_buff structure, like headroom, length,
> + * tailroom, MAC, etc.
> + */
> +void devlink_fmsg_dump_skb(struct devlink_fmsg *fmsg, const struct sk_buff *skb)
> +{
> +	struct skb_shared_info *sh = skb_shinfo(skb);
> +	struct sock *sk = skb->sk;
> +	bool has_mac, has_trans;
> +
> +	has_mac = skb_mac_header_was_set(skb);
> +	has_trans = skb_transport_header_was_set(skb);
> +
> +	devlink_fmsg_pair_nest_start(fmsg, "skb");
> +	devlink_fmsg_obj_nest_start(fmsg);
> +	devlink_fmsg_put(fmsg, "actual len", skb->len);
> +	devlink_fmsg_put(fmsg, "head len", skb_headlen(skb));
> +	devlink_fmsg_put(fmsg, "data len", skb->data_len);
> +	devlink_fmsg_put(fmsg, "tail len", skb_tailroom(skb));
> +	devlink_fmsg_put(fmsg, "MAC", has_mac ? skb->mac_header : -1);
> +	devlink_fmsg_put(fmsg, "MAC len",
> +			 has_mac ? skb_mac_header_len(skb) : -1);
> +	devlink_fmsg_put(fmsg, "network hdr", skb->network_header);
> +	devlink_fmsg_put(fmsg, "network hdr len",
> +			 has_trans ? skb_network_header_len(skb) : -1);
> +	devlink_fmsg_put(fmsg, "transport hdr",
> +			 has_trans ? skb->transport_header : -1);
> +	devlink_fmsg_put(fmsg, "csum", skb->csum);

Hi,

One minor nit here, which I don't think needs to stop progress of this
patchset. Sparse warns that:

error: no generic selection for 'restricted __wsum const [usertype] csum'

I believe this can be addressed by casting: (__force __u32) skb->csum,
perhaps incorporated into devlink_fmsg_put(). Which seems fine enough for
this case.

However, my observation is that there are a lot of sparse warnings
present in the tree due to similar issues around the use of __wsum.
And IMHO naked casts are error prone and not obviously correct to the
reader (me). So I wonder if there is some value in introducing some
helpers. E.g.

	wsum_to_cpu()
	cpu_to_wsum()

To my mind, that would clearly be out of scope for this patchset.
But It seems appropriate to raise this as it's been on my mind for a while.

...