Hi, On 10/23/2024 11:10 AM, Yafang Shao wrote: > On Mon, Oct 21, 2024 at 9:28 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: >> From: Hou Tao <houtao1@xxxxxxxxxx> >> >> On 32-bit hosts (e.g., arm32), when a bpf program passes a u64 to >> bpf_iter_bits_new(), bpf_iter_bits_new() will use bits_copy to store the >> content of the u64. However, bits_copy is only 4 bytes, leading to stack >> corruption. >> >> The straightforward solution would be to replace u64 with unsigned long >> in bpf_iter_bits_new(). However, this introduces confusion and problems >> for 32-bit hosts because the size of ulong in bpf program is 8 bytes, >> but it is treated as 4-bytes after passed to bpf_iter_bits_new(). >> >> Fix it by changing the type of both bits and bit_count from unsigned >> long to u64. > Thank you for the fix. This change is necessary. > >> However, the change is not enough. The main reason is that >> bpf_iter_bits_next() uses find_next_bit() to find the next bit and the >> pointer passed to find_next_bit() is an unsigned long pointer instead >> of a u64 pointer. For 32-bit little-endian host, it is fine but it is >> not the case for 32-bit big-endian host. Because under 32-bit big-endian >> host, the first iterated unsigned long will be the bits 32-63 of the u64 >> instead of the expected bits 0-31. Therefore, in addition to changing >> the type, swap the two unsigned longs within the u64 for 32-bit >> big-endian host. > The API uses a u64 data type, and the nr_words parameter represents > the number of 8-byte units. On a 32-bit system, if you want to call > this API, you would define an array like `u32 data[2]` and invoke the > function as `bpf_for_each(bits, bit, &data[0], 1)`. However, since the > API expects a u64, you'll need to merge the two u32 values into a > single u64 value. It is a bit weird to pass a u32 pointer to bpf_for_each, because it expects a u64 pointer. I think the bpf program should pass a u64 pointer instead. > > Given this, it might be more appropriate to ask users to handle the > u32 to u64 merge on their side when preparing the data, rather than > performing the swap within the kernel itself. However, the swap implemented in the patch has nothing to do whether the user passes pointer to u32 array or not. It is necessary due to the mismatched between the pointer type used by find_next_bit and the pointer used by bpf_iter_bits_new(). The latter uses u64 pointer, but find_next_bit() uses unsigned long pointer and iterates a long each time. So just like the comment in the patch said: under 32-bit big-endian host, the first iterated unsigned long will be the bits 32-63 of the u64 instead of the expected bits 0-31. The swap in the patch will swap the two long values in the u64 and make the first iterated unsigned long will be the bits 0-31 of the u64 value. > > -- > Regards > > Yafang
