[DTrace-devel] [PATCH] cg: fix offset for > 8 bit bitfields in dt_cg_ctf_offsetof()

[Alan Maguire]

On 26/08/2025 18:40, Nick Alcock wrote:
> On 26 Aug 2025, Alan Maguire verbalised:
> 
>> The tcp provider uses dt_cg_tramp_get_member() to retrieve the
>> offset of the sk_protocol field in struct sock.  However it
>> returns the wrong value on UEK6 since it is an 8-bit bitfield.
>> From pahole we see:
>>
>> 	unsigned int               __sk_flags_offset[0]; /*   560     0 */
>> 	unsigned int               sk_padding:1;         /*   560: 0  4 */
>> 	unsigned int               sk_kern_sock:1;       /*   560: 1  4 */
>> 	unsigned int               sk_no_check_tx:1;     /*   560: 2  4 */
>> 	unsigned int               sk_no_check_rx:1;     /*   560: 3  4 */
>> 	unsigned int               sk_userlocks:4;       /*   560: 4  4 */
>> 	unsigned int               sk_protocol:8;        /*   560: 8  4 */
>>
>> In other words it is really at offset 561 but because we just
>> lookup the member offset and not the member type offset we get the
>> wrong value for the sk_protoocol.
> 
> Excellent description! It's annoying as hell that BTF still has this
> redundant representation :( DTrace literally always got this wrong,
> right back to the year dot.
> 
>> The fix is to look up the member _type_ offset and add it to the
>> bit offset we get for the member itself.  With this in place the
>> state-change probes fire, but the local tcp tests still fail due
>> to separate issues with the tcp:::accept-established probe.
> 
> This is certainly necessary (and your code looks good), but we also need
> to ensure that the load is suitably shifted and masked, particularly for
> bitfields that cross machine words. DTrace never used to do this
> either... e.g. the size returned by dt_cg_ldsize() assumes that the
> entire bitfield can be read in one load op, which depending on its
> offset may not be true.
> 
> It looks like dt_cg_field_get might get this *mostly* right (once you
> delete the #if 0'ed stuff -- but I'd be suspicious until I'd tested it
> on a big-endian platform), but even that assumes it only needs to do one
> load. As soon as a bitfield crosses machine words, I suspect we'll only
> get the first half of it :( we might need to do a second load to fetch
> the second half of the bitfield, then a shift and | to combine the two,
> and DTrace has never had any code for that.


thanks for taking a look! To be honest I punted on bitfield handling as
currently we don't have any cases where we are looking up bitfield
values that require shifting. The problem is we have the machinery for
offsetof calculation in dt_cg_ctf_offsetof() and it just returns byte
offset and load size, then after we do the probe_read_kernel(). We'd
need to repeat some of the operations done for the offsetof cacluation
to determine if/how we bitshift, or alternatively have
dt_cg_ctf_offsetof() return more info to consumers.

Perhaps the better option would be to rework dt_cg_ctf_offsetof() to be
a wrapper to a function that gives us this additional info; that would
require less changes overall to consumers of that function that don't
need to handle bitfields. I'm thinking something like this:

int
dt_cg_ctf_bit_offsetof(const char *structname, const char *membername,
		       size_t *ldsizep, uint_t *ldopp, size_t *bitszp,
		       int relaxed)


where the returned offset, *bitsizep are in bits (*ldsizep would still
be in bytes). This would allow us to figure out what shifting is
required in dt_cg_tramp_get_member().

Either that or I can just add a comment that we don't need bitshifts yet!

Thanks!

Alan