2010-11-12 01:17:27

by James Jones

[permalink] [raw]
Subject: [PATCH] ARM: Fix find_next_zero_bit and related assembly

The find_next_bit, find_first_bit, find_next_zero_bit
and find_first_zero_bit functions were not properly
clamping to the maxbit argument at the bit level. They
were instead only checking maxbit at the byte level.
To fix this, add a compare and a conditional move
instruction to the end of the common bit-within-the-
byte code used by all the functions and be sure not to
clobber the maxbit argument before it is used.

Signed-off-by: James Jones <[email protected]>
Tested-by: Stephen Warren <[email protected]>
---
arch/arm/lib/findbit.S | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/lib/findbit.S b/arch/arm/lib/findbit.S
index 1e4cbd4..64f6bc1 100644
--- a/arch/arm/lib/findbit.S
+++ b/arch/arm/lib/findbit.S
@@ -174,8 +174,8 @@ ENDPROC(_find_next_bit_be)
*/
.L_found:
#if __LINUX_ARM_ARCH__ >= 5
- rsb r1, r3, #0
- and r3, r3, r1
+ rsb r0, r3, #0
+ and r3, r3, r0
clz r3, r3
rsb r3, r3, #31
add r0, r2, r3
@@ -190,5 +190,7 @@ ENDPROC(_find_next_bit_be)
addeq r2, r2, #1
mov r0, r2
#endif
+ cmp r1, r0 @ Clamp to maxbit
+ movlo r0, r1
mov pc, lr

--
1.7.1


2010-11-23 21:26:50

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Thu, 11 Nov 2010, James Jones wrote:

> The find_next_bit, find_first_bit, find_next_zero_bit
> and find_first_zero_bit functions were not properly
> clamping to the maxbit argument at the bit level. They
> were instead only checking maxbit at the byte level.
> To fix this, add a compare and a conditional move
> instruction to the end of the common bit-within-the-
> byte code used by all the functions and be sure not to
> clobber the maxbit argument before it is used.
>
> Signed-off-by: James Jones <[email protected]>
> Tested-by: Stephen Warren <[email protected]>

Reviewed-by: Nicolas Pitre <[email protected]>

Please send to RMK's patch system.



> ---
> arch/arm/lib/findbit.S | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/lib/findbit.S b/arch/arm/lib/findbit.S
> index 1e4cbd4..64f6bc1 100644
> --- a/arch/arm/lib/findbit.S
> +++ b/arch/arm/lib/findbit.S
> @@ -174,8 +174,8 @@ ENDPROC(_find_next_bit_be)
> */
> .L_found:
> #if __LINUX_ARM_ARCH__ >= 5
> - rsb r1, r3, #0
> - and r3, r3, r1
> + rsb r0, r3, #0
> + and r3, r3, r0
> clz r3, r3
> rsb r3, r3, #31
> add r0, r2, r3
> @@ -190,5 +190,7 @@ ENDPROC(_find_next_bit_be)
> addeq r2, r2, #1
> mov r0, r2
> #endif
> + cmp r1, r0 @ Clamp to maxbit
> + movlo r0, r1
> mov pc, lr
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2010-11-23 22:29:41

by James Jones

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
> On Thu, 11 Nov 2010, James Jones wrote:
> > The find_next_bit, find_first_bit, find_next_zero_bit
> > and find_first_zero_bit functions were not properly
> > clamping to the maxbit argument at the bit level. They
> > were instead only checking maxbit at the byte level.
> > To fix this, add a compare and a conditional move
> > instruction to the end of the common bit-within-the-
> > byte code used by all the functions and be sure not to
> > clobber the maxbit argument before it is used.
> >
> > Signed-off-by: James Jones <[email protected]>
> > Tested-by: Stephen Warren <[email protected]>
>
> Reviewed-by: Nicolas Pitre <[email protected]>
>
> Please send to RMK's patch system.

Thanks for the review. It's already in the patch system, but I updated the
entry to include your reviewed-by line.

-James

> > ---
> >
> > arch/arm/lib/findbit.S | 6 ++++--
> > 1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/lib/findbit.S b/arch/arm/lib/findbit.S
> > index 1e4cbd4..64f6bc1 100644
> > --- a/arch/arm/lib/findbit.S
> > +++ b/arch/arm/lib/findbit.S
> > @@ -174,8 +174,8 @@ ENDPROC(_find_next_bit_be)
> >
> > */
> >
> > .L_found:
> > #if __LINUX_ARM_ARCH__ >= 5
> >
> > - rsb r1, r3, #0
> > - and r3, r3, r1
> > + rsb r0, r3, #0
> > + and r3, r3, r0
> >
> > clz r3, r3
> > rsb r3, r3, #31
> > add r0, r2, r3
> >
> > @@ -190,5 +190,7 @@ ENDPROC(_find_next_bit_be)
> >
> > addeq r2, r2, #1
> > mov r0, r2
> >
> > #endif
> >
> > + cmp r1, r0 @ Clamp to maxbit
> > + movlo r0, r1
> >
> > mov pc, lr

2010-11-24 18:12:56

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On 11/23/2010 03:28 PM, James Jones wrote:
> On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
>> On Thu, 11 Nov 2010, James Jones wrote:
>>> The find_next_bit, find_first_bit, find_next_zero_bit
>>> and find_first_zero_bit functions were not properly
>>> clamping to the maxbit argument at the bit level. They
>>> were instead only checking maxbit at the byte level.
>>> To fix this, add a compare and a conditional move
>>> instruction to the end of the common bit-within-the-
>>> byte code used by all the functions and be sure not to
>>> clobber the maxbit argument before it is used.
>>>
>>> Signed-off-by: James Jones <[email protected]>
>>> Tested-by: Stephen Warren <[email protected]>
>> Reviewed-by: Nicolas Pitre <[email protected]>
>>
>> Please send to RMK's patch system.
> Thanks for the review. It's already in the patch system, but I updated the
> entry to include your reviewed-by line.

Should this be sent to the stable tree too?

2010-11-24 19:00:14

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Wed, 24 Nov 2010, Stephen Boyd wrote:

> On 11/23/2010 03:28 PM, James Jones wrote:
> > On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
> >> On Thu, 11 Nov 2010, James Jones wrote:
> >>> The find_next_bit, find_first_bit, find_next_zero_bit
> >>> and find_first_zero_bit functions were not properly
> >>> clamping to the maxbit argument at the bit level. They
> >>> were instead only checking maxbit at the byte level.
> >>> To fix this, add a compare and a conditional move
> >>> instruction to the end of the common bit-within-the-
> >>> byte code used by all the functions and be sure not to
> >>> clobber the maxbit argument before it is used.
> >>>
> >>> Signed-off-by: James Jones <[email protected]>
> >>> Tested-by: Stephen Warren <[email protected]>
> >> Reviewed-by: Nicolas Pitre <[email protected]>
> >>
> >> Please send to RMK's patch system.
> > Thanks for the review. It's already in the patch system, but I updated the
> > entry to include your reviewed-by line.
>
> Should this be sent to the stable tree too?

It could, yes. This is hardly an urgent fix though, as the bug has been
there virtually forever.


Nicolas

2010-11-24 19:08:55

by James Jones

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Wednesday 24 November 2010 11:00:12 am Nicolas Pitre wrote:
> On Wed, 24 Nov 2010, Stephen Boyd wrote:
> > On 11/23/2010 03:28 PM, James Jones wrote:
> > > On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
> > >> On Thu, 11 Nov 2010, James Jones wrote:
> > >>> The find_next_bit, find_first_bit, find_next_zero_bit
> > >>> and find_first_zero_bit functions were not properly
> > >>> clamping to the maxbit argument at the bit level. They
> > >>> were instead only checking maxbit at the byte level.
> > >>> To fix this, add a compare and a conditional move
> > >>> instruction to the end of the common bit-within-the-
> > >>> byte code used by all the functions and be sure not to
> > >>> clobber the maxbit argument before it is used.
> > >>>
> > >>> Signed-off-by: James Jones <[email protected]>
> > >>> Tested-by: Stephen Warren <[email protected]>
> > >>
> > >> Reviewed-by: Nicolas Pitre <[email protected]>
> > >>
> > >> Please send to RMK's patch system.
> > >
> > > Thanks for the review. It's already in the patch system, but I updated
> > > the entry to include your reviewed-by line.
> >
> > Should this be sent to the stable tree too?
>
> It could, yes. This is hardly an urgent fix though, as the bug has been
> there virtually forever.
>
>
> Nicolas

While ancient, it does cause per-cpu allocations to fail in some situations,
which generally causes panics.

Thanks,
-James

2010-11-24 19:13:12

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Wed, 24 Nov 2010, James Jones wrote:

> On Wednesday 24 November 2010 11:00:12 am Nicolas Pitre wrote:
> > On Wed, 24 Nov 2010, Stephen Boyd wrote:
> > > On 11/23/2010 03:28 PM, James Jones wrote:
> > > > On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
> > > >> On Thu, 11 Nov 2010, James Jones wrote:
> > > >>> The find_next_bit, find_first_bit, find_next_zero_bit
> > > >>> and find_first_zero_bit functions were not properly
> > > >>> clamping to the maxbit argument at the bit level. They
> > > >>> were instead only checking maxbit at the byte level.
> > > >>> To fix this, add a compare and a conditional move
> > > >>> instruction to the end of the common bit-within-the-
> > > >>> byte code used by all the functions and be sure not to
> > > >>> clobber the maxbit argument before it is used.
> > > >>>
> > > >>> Signed-off-by: James Jones <[email protected]>
> > > >>> Tested-by: Stephen Warren <[email protected]>
> > > >>
> > > >> Reviewed-by: Nicolas Pitre <[email protected]>
> > > >>
> > > >> Please send to RMK's patch system.
> > > >
> > > > Thanks for the review. It's already in the patch system, but I updated
> > > > the entry to include your reviewed-by line.
> > >
> > > Should this be sent to the stable tree too?
> >
> > It could, yes. This is hardly an urgent fix though, as the bug has been
> > there virtually forever.
> >
> >
> > Nicolas
>
> While ancient, it does cause per-cpu allocations to fail in some situations,
> which generally causes panics.

That would be a good justification for the stable tree then.


Nicolas

2010-11-24 19:15:28

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH] ARM: Fix find_next_zero_bit and related assembly

On Wed, Nov 24, 2010 at 02:13:05PM -0500, Nicolas Pitre wrote:
> On Wed, 24 Nov 2010, James Jones wrote:
>
> > On Wednesday 24 November 2010 11:00:12 am Nicolas Pitre wrote:
> > > On Wed, 24 Nov 2010, Stephen Boyd wrote:
> > > > On 11/23/2010 03:28 PM, James Jones wrote:
> > > > > On Tuesday 23 November 2010 13:26:47 Nicolas Pitre wrote:
> > > > >> On Thu, 11 Nov 2010, James Jones wrote:
> > > > >>> The find_next_bit, find_first_bit, find_next_zero_bit
> > > > >>> and find_first_zero_bit functions were not properly
> > > > >>> clamping to the maxbit argument at the bit level. They
> > > > >>> were instead only checking maxbit at the byte level.
> > > > >>> To fix this, add a compare and a conditional move
> > > > >>> instruction to the end of the common bit-within-the-
> > > > >>> byte code used by all the functions and be sure not to
> > > > >>> clobber the maxbit argument before it is used.
> > > > >>>
> > > > >>> Signed-off-by: James Jones <[email protected]>
> > > > >>> Tested-by: Stephen Warren <[email protected]>
> > > > >>
> > > > >> Reviewed-by: Nicolas Pitre <[email protected]>
> > > > >>
> > > > >> Please send to RMK's patch system.
> > > > >
> > > > > Thanks for the review. It's already in the patch system, but I updated
> > > > > the entry to include your reviewed-by line.
> > > >
> > > > Should this be sent to the stable tree too?
> > >
> > > It could, yes. This is hardly an urgent fix though, as the bug has been
> > > there virtually forever.
> > >
> > >
> > > Nicolas
> >
> > While ancient, it does cause per-cpu allocations to fail in some situations,
> > which generally causes panics.
>
> That would be a good justification for the stable tree then.

And the best way to do that is _not_ to send it to [email protected],
but when you submit the patch to the patch system, add a line in
the sign-off area:

CC: <[email protected]>

and when it goes into mainline (which is a requirement for stable
patches) the stable team will automatically pick it up - without anyone
needing to do any additional work.