[PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[Alexander Gordeev]

Commit 2d6261583be0 ("lib: rework bitmap_parse()") does not
take into account order of halfwords on 64-bit big endian
architectures. As result (at least) Receive Packet Steering,
IRQ affinity masks and runtime kernel test "test_bitmap" get
broken on s390.

Fixes: 2d6261583be0 ("lib: rework bitmap_parse()")
Cc: stable@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Amritha Nambiar <amritha.nambiar@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Miklos Szeredi <mszeredi@redhat.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: "Tobin C . Harding" <tobin@kernel.org>
Cc: Vineet Gupta <vineet.gupta1@synopsys.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 lib/bitmap.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 89260aa342d6..a725e4612984 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -717,6 +717,19 @@ static const char *bitmap_get_x32_reverse(const char *start,
 	return end;
 }
 
+#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
+static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
+{
+	maskp += (chunk_idx / 2);
+	((u32 *)maskp)[(chunk_idx & 1) ^ 1] = chunk;
+}
+#else
+static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
+{
+	((u32 *)maskp)[chunk_idx] = chunk;
+}
+#endif
+
 /**
  * bitmap_parse - convert an ASCII hex string into a bitmap.
  * @start: pointer to buffer containing string.
@@ -738,7 +751,8 @@ int bitmap_parse(const char *start, unsigned int buflen,
 {
 	const char *end = strnchrnul(start, buflen, '\n') - 1;
 	int chunks = BITS_TO_U32(nmaskbits);
-	u32 *bitmap = (u32 *)maskp;
+	int chunk_idx = 0;
+	u32 chunk;
 	int unset_bit;
 
 	while (1) {
@@ -749,9 +763,11 @@ int bitmap_parse(const char *start, unsigned int buflen,
 		if (!chunks--)
 			return -EOVERFLOW;
 
-		end = bitmap_get_x32_reverse(start, end, bitmap++);
+		end = bitmap_get_x32_reverse(start, end, &chunk);
 		if (IS_ERR(end))
 			return PTR_ERR(end);
+
+		save_x32_chunk(maskp, chunk, chunk_idx++);
 	}
 
 	unset_bit = (BITS_TO_U32(nmaskbits) - chunks) * 32;
-- 
2.23.0

Re: [PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[Andy Shevchenko]

On Mon, Jun 8, 2020 at 1:26 PM Alexander Gordeev <agordeev@linux.ibm.com> wrote:
>
> Commit 2d6261583be0 ("lib: rework bitmap_parse()") does not
> take into account order of halfwords on 64-bit big endian
> architectures. As result (at least) Receive Packet Steering,
> IRQ affinity masks and runtime kernel test "test_bitmap" get
> broken on s390.

...

> +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)

I think it's better to re-use existing patterns.

ipc/sem.c:1682:#if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)

> +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> +{
> +       maskp += (chunk_idx / 2);
> +       ((u32 *)maskp)[(chunk_idx & 1) ^ 1] = chunk;
> +}
> +#else
> +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> +{
> +       ((u32 *)maskp)[chunk_idx] = chunk;
> +}
> +#endif

See below.

...

> -               end = bitmap_get_x32_reverse(start, end, bitmap++);
> +               end = bitmap_get_x32_reverse(start, end, &chunk);
>                 if (IS_ERR(end))
>                         return PTR_ERR(end);
> +
> +               save_x32_chunk(maskp, chunk, chunk_idx++);

Can't we simple do

        int chunk_index = 0;
        ...
        do {
#if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
               end = bitmap_get_x32_reverse(start, end,
bitmap[chunk_index ^ 1]);
#else
               end = bitmap_get_x32_reverse(start, end, bitmap[chunk_index]);
#endif
        ...
        } while (++chunk_index);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[Andy Shevchenko]

On Mon, Jun 8, 2020 at 3:03 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jun 8, 2020 at 1:26 PM Alexander Gordeev <agordeev@linux.ibm.com> wrote:

...

> Can't we simple do
>
>         int chunk_index = 0;
>         ...
>         do {
> #if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
>                end = bitmap_get_x32_reverse(start, end,
> bitmap[chunk_index ^ 1]);
> #else
>                end = bitmap_get_x32_reverse(start, end, bitmap[chunk_index]);
> #endif
>         ...
>         } while (++chunk_index);
>
> ?

And moreover, we simple can replace bitmap by maskp here, and drop it
from definition block.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[Alexander Gordeev]

On Mon, Jun 08, 2020 at 03:03:05PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 8, 2020 at 1:26 PM Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> >
> > Commit 2d6261583be0 ("lib: rework bitmap_parse()") does not
> > take into account order of halfwords on 64-bit big endian
> > architectures. As result (at least) Receive Packet Steering,
> > IRQ affinity masks and runtime kernel test "test_bitmap" get
> > broken on s390.
> 
> ...
> 
> > +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
> 
> I think it's better to re-use existing patterns.
> 
> ipc/sem.c:1682:#if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
> 
> > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> > +{
> > +       maskp += (chunk_idx / 2);
> > +       ((u32 *)maskp)[(chunk_idx & 1) ^ 1] = chunk;
> > +}
> > +#else
> > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> > +{
> > +       ((u32 *)maskp)[chunk_idx] = chunk;
> > +}
> > +#endif
> 
> See below.
> 
> ...
> 
> > -               end = bitmap_get_x32_reverse(start, end, bitmap++);
> > +               end = bitmap_get_x32_reverse(start, end, &chunk);
> >                 if (IS_ERR(end))
> >                         return PTR_ERR(end);
> > +
> > +               save_x32_chunk(maskp, chunk, chunk_idx++);
> 
> Can't we simple do
> 
>         int chunk_index = 0;
>         ...
>         do {
> #if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
>                end = bitmap_get_x32_reverse(start, end,
> bitmap[chunk_index ^ 1]);
> #else
>                end = bitmap_get_x32_reverse(start, end, bitmap[chunk_index]);
> #endif
>         ...
>         } while (++chunk_index);
> 
> ?

Well, unless we ignore coding style 21) Conditional Compilation
we could. Do you still insist it would be better?

Thanks for the review!

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs

[Andy Shevchenko]

On Mon, Jun 08, 2020 at 02:44:34PM +0200, Alexander Gordeev wrote:
> On Mon, Jun 08, 2020 at 03:03:05PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 8, 2020 at 1:26 PM Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> > >
> > > Commit 2d6261583be0 ("lib: rework bitmap_parse()") does not
> > > take into account order of halfwords on 64-bit big endian
> > > architectures. As result (at least) Receive Packet Steering,
> > > IRQ affinity masks and runtime kernel test "test_bitmap" get
> > > broken on s390.
> > 
> > ...
> > 
> > > +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
> > 
> > I think it's better to re-use existing patterns.
> > 
> > ipc/sem.c:1682:#if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
> > 
> > > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> > > +{
> > > +       maskp += (chunk_idx / 2);
> > > +       ((u32 *)maskp)[(chunk_idx & 1) ^ 1] = chunk;
> > > +}
> > > +#else
> > > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx)
> > > +{
> > > +       ((u32 *)maskp)[chunk_idx] = chunk;
> > > +}
> > > +#endif
> > 
> > See below.
> > 
> > ...
> > 
> > > -               end = bitmap_get_x32_reverse(start, end, bitmap++);
> > > +               end = bitmap_get_x32_reverse(start, end, &chunk);
> > >                 if (IS_ERR(end))
> > >                         return PTR_ERR(end);
> > > +
> > > +               save_x32_chunk(maskp, chunk, chunk_idx++);
> > 
> > Can't we simple do
> > 
> >         int chunk_index = 0;
> >         ...
> >         do {
> > #if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN)
> >                end = bitmap_get_x32_reverse(start, end,
> > bitmap[chunk_index ^ 1]);
> > #else
> >                end = bitmap_get_x32_reverse(start, end, bitmap[chunk_index]);
> > #endif
> >         ...
> >         } while (++chunk_index);
> > 
> > ?
> 
> Well, unless we ignore coding style 21) Conditional Compilation
> we could. Do you still insist it would be better?

I think it's okay to do here
 - it's not a big function
 - it has no stub versions (you always do something)
 - the result pretty much readable (5 lines any editor can keep on screen)
 - and it's not ignoring, see "Wherever possible...", compare readability of
   two versions, for yours reader needs to go somewhere to read, calculate and
   return, when everything already being forgotten
 - last but not least, I bet it makes code shorter (at least in C)

-- 
With Best Regards,
Andy Shevchenko