linux-kernel - Re: [PATCH v2] tracing/synthetic: use union instead of casts

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: <20230814204723.0fd326c0@gandalf.local.home>
Date:   Mon, 14 Aug 2023 20:47:23 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     Sven Schnelle <svens@...ux.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tracing/synthetic: use union instead of casts

On Mon, 14 Aug 2023 11:34:20 +0000
David Laight <David.Laight@...LAB.COM> wrote:

> > No need to create a structure around a single element union. Also, I would
> > like to name it for what it is for.
> > 
> > union trace_synth_field {
> > 	u8				as_u8;
> > 	u16				as_u16;
> > 	u32				as_u32;
> > 	u64				as_u64;
> > 	struct trace_dynamic_info	as_dynamic;
> > };
> > 
> > Other than that, the patch looks good. Although I still need to test it.  
> 
> I was wondering if you need the u8 and u16 members at all?
> Can't the values just be treated as 32bit?
> Both char and short aren't really 'proper' arithmetic types.

Not sure what you mean by "arithmatic types".

If you do something like:

 ~# echo 's:skb u16 protocol; u64 delta;' >> /sys/kernel/tracing/dynamic_events
 ~# echo 'hist:keys=skbaddr:ts=common_timestamp.usecs' >> /sys/kernel/tracing/events/net/netif_receive_skb/trigger
 ~# echo 'hist:keys=skbaddr:delta=common_timestamp.usecs-$ts:onmatch(net.netif_receive_skb).trace(skb,protocol,$delta)' >> /sys/kernel/tracing/events/skb/kfree_skb/trigger

Where it records the protocol as u16, I'm guessing that if we didn't use
this union, it might break on BIG_ENDIAN machines.

For reference, the kfree_skb event looks like:

system: skb
name: kfree_skb
ID: 1735
format:
        field:unsigned short common_type;       offset:0;       size:2; signed:0;
        field:unsigned char common_flags;       offset:2;       size:1; signed:0;
        field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
        field:int common_pid;   offset:4;       size:4; signed:1;

        field:void * skbaddr;   offset:8;       size:8; signed:0;
        field:void * location;  offset:16;      size:8; signed:0;
        field:unsigned short protocol;  offset:24;      size:2; signed:0;
        field:enum skb_drop_reason reason;      offset:28;      size:4; signed:0;

-- Steve