On Tue, Dec 29, 2020 at 5:50 AM Andy Shevchenko <[email protected]> wrote: > > On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler > <[email protected]> wrote: > > > > On 12/28/20 8:10 PM, Yury Norov wrote: > > > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko > > > <[email protected]> wrote: > > >> On Mon, Dec 28, 2020 at 11:43:43AM -0800, Yury Norov wrote: > > >>> The commit be3e477effba636ad25 ("drm/komeda: Fix bit > > >>> check to import to value of proper type") fixes possible > > >>> out-of-bound issue related to find_first_bit() usage, but > > >>> does not address the endianness problem. > > >> Hmm... Can you elaborate? > > >> > > >> ... > > >> > > >>> u32 comp_mask) > > >>> - unsigned long comp_mask_local = (unsigned long)comp_mask; > > >> Here we convert u32 to unsigned long (LSB is kept LSB since it happens in > > >> native endianess). > > >> > > >>> - id = find_first_bit(&comp_mask_local, 32); > > >> Here it takes an address to unsigned long and tries only lower 32 bits. > > >> > > >> Are you telling that find_first_bit() has an issue? > > > It seems you're right, there's no issue with endianness in existing code. > > > In fact, the line > > > > Indeed Andy covered this. Take LSB32 with the cast to "local on-stack > > long" and possible extend upper 32bits with 0's if needed (64bit longs). > > > > >>> - unsigned long comp_mask_local = (unsigned long)comp_mask; > > > is an opencoded version of bitmap_from_arr32(dst, src, 32). > > > > > > Maybe it would be better to use the bitmap API here, but existing code is > > > correct. Sorry for the noise. > > While your code is seemingly also valid (I can check to be sure with > > KASAN if you want still), it does seem a little less "nice to read" with > > more lines of code for the same work. Is it worth making the code a > > little longer here as it's not actually fixing anything to do it this > > other way? DECLARE_BITMAP() is a bit of an obscure way to declare a > > single unsigned long (in this case) where the compiler does the right > > thing anyway with a simple assign+cast making it easier to read/follow IMHO. > > What we can do is to declare BITS_PER_U32. > Also we can pay attention on these definitions: > https://elixir.bootlin.com/linux/latest/source/kernel/bpf/btf.c#L168 > https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/ebitmap.c#L27
> And define BITMAP_FROM_U32() macro. > > Then It would be written like > > DECLARE_BITMAP(comp_mask_local, BITS_PER_U32) = BITMAP_FROM_U32(comp_mask); > > But this is a bit verbose. > > Also, it can be something like DECLARE_BITMAP_U32(...) = BITMAP_FROM_U32(...); > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > > recipient, please notify the sender immediately and do not disclose the > > contents to any other person, use it for any purpose, or store or copy the > > information in any medium. Thank you. > > Hmm... you probably have to get rid of this footer. > > -- > With Best Regards, > Andy Shevchenko
