Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201228194343.88880-1-yury.norov@gmail.com> <20201228195016.GD4077@smile.fi.intel.com>
 <CAAH8bW9UvDAB4NSQB6CuhBU6D=R6Ex6=5V=Ld74hYGK474_Trw@mail.gmail.com>
 <808b7555-5ba2-f1fe-3800-1d5f59c47b52@arm.com> <CAHp75Vd8PZM5Z5A0bCapeOEbvzqXtfUXUDK=6G1nxD527E3=rA@mail.gmail.com>
In-Reply-To: <CAHp75Vd8PZM5Z5A0bCapeOEbvzqXtfUXUDK=6G1nxD527E3=rA@mail.gmail.com>
From:   Yury Norov <yury.norov@gmail.com>
Date:   Tue, 29 Dec 2020 10:09:21 -0800
Message-ID: <CAAH8bW_gaA9m_tFX_HGJZ-fy0FHeVonO-jMU6Zpu7roo7kXQwA@mail.gmail.com>
Subject: Re: [PATCH] drm/komeda: use bitmap API to convert U32 to bitmap
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Carsten Haitzler <Carsten.Haitzler@arm.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Liviu Dudau <Liviu.Dudau@arm.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 29, 2020 at 5:50 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Dec 29, 2020 at 2:24 PM Carsten Haitzler
> <Carsten.Haitzler@arm.com> wrote:
> >
> > On 12/28/20 8:10 PM, Yury Norov wrote:
> > > On Mon, Dec 28, 2020 at 11:49 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> 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 =3D (unsigned long)comp_mask;
> > >> Here we convert u32 to unsigned long (LSB is kept LSB since it happe=
ns in
> > >> native endianess).
> > >>
> > >>> -     id =3D find_first_bit(&comp_mask_local, 32);
> > >> Here it takes an address to unsigned long and tries only lower 32 bi=
ts.
> > >>
> > >> Are you telling that find_first_bit() has an issue?
> > > It seems you're right, there's no issue with endianness in existing c=
ode.
> > > 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 =3D (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 cod=
e 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" wit=
h
> > 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/ebitma=
p.c#L27


> And define BITMAP_FROM_U32() macro.
>
> Then It would be written like
>
> DECLARE_BITMAP(comp_mask_local, BITS_PER_U32) =3D BITMAP_FROM_U32(comp_ma=
sk);
>
> But this is a bit verbose.
>
> Also, it can be something like DECLARE_BITMAP_U32(...) =3D BITMAP_FROM_U3=
2(...);
>
> > IMPORTANT NOTICE: The contents of this email and any attachments are co=
nfidential and may also be privileged. If you are not the intended recipien=
t, 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