[PATCH 0/10] use safer test on the result of find_first_zero_bit

[PATCH 0/10] use safer test on the result of find_first_zero_bit

[Julia Lawall]

Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.

[PATCH 2/10] ath10k: use safer test on the result of find_first_zero_bit

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@

e1 = find_first_zero_bit(e2,e3)
...
if (e1 
- ==
+ >=
  e3)
S1 else S2
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/net/wireless/ath/ath10k/htt_tx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -64,7 +64,7 @@ int ath10k_htt_tx_alloc_msdu_id(struct a
 
 	msdu_id = find_first_zero_bit(htt->used_msdu_ids,
 				      htt->max_num_pending_tx);
-	if (msdu_id == htt->max_num_pending_tx)
+	if (msdu_id >= htt->max_num_pending_tx)
 		return -ENOBUFS;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Geert Uytterhoeven]

Hi Julia,

On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.

Shouldn't this be fixed in find_first_zero_bit() instead?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Julia Lawall]

On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:

> Hi Julia,
>
> On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > return a larger number than the maximum position argument if that position
> > is not a multiple of BITS_PER_LONG.
>
> Shouldn't this be fixed in find_first_zero_bit() instead?

OK, I could do that as well.  Most of the callers currently test with >=.
Should they be left as is, or changed to use ==?

julia

RE: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[David Laight]

From: Julia Lawall
> On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> 
> > Hi Julia,
> >
> > On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > > return a larger number than the maximum position argument if that position
> > > is not a multiple of BITS_PER_LONG.
> >
> > Shouldn't this be fixed in find_first_zero_bit() instead?
> 
> OK, I could do that as well.  Most of the callers currently test with >=.
> Should they be left as is, or changed to use ==?

Do we want to add an extra test to find_first_zero_bit() and effectively
slow down all the calls - especially those where the length is a
multiple of 8 (probably the most common).

Maybe the documented return code should be changed to allow for the
existing behaviour.

	David

RE: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Julia Lawall]

On Wed, 4 Jun 2014, David Laight wrote:

> From: Julia Lawall
> > On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> >
> > > Hi Julia,
> > >
> > > On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > > > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > > > return a larger number than the maximum position argument if that position
> > > > is not a multiple of BITS_PER_LONG.
> > >
> > > Shouldn't this be fixed in find_first_zero_bit() instead?
> >
> > OK, I could do that as well.  Most of the callers currently test with >=.
> > Should they be left as is, or changed to use ==?
>
> Do we want to add an extra test to find_first_zero_bit() and effectively
> slow down all the calls - especially those where the length is a
> multiple of 8 (probably the most common).

Currently, most of the calls test with >=, and most of the others seem to
need to (either the size value did not look like a multiple of anything in
particular, or it was eg read from a device).

Note that it is BITS_PER_LONG, so it seems like it is typically 32 or 64,
not 8.

> Maybe the documented return code should be changed to allow for the
> existing behaviour.

Sorry, I'm not sure to understand what you suggest here.

thanks,
julia

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Geert Uytterhoeven]

Hi Julia,

On Wed, Jun 4, 2014 at 11:52 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> Maybe the documented return code should be changed to allow for the
>> existing behaviour.
>
> Sorry, I'm not sure to understand what you suggest here.

include/asm-generic/bitops/find.h:

| /**
|  * find_first_zero_bit - find the first cleared bit in a memory region
|  * @addr: The address to start the search at
|  * @size: The maximum number of bits to search
|  *
|  * Returns the bit number of the first cleared bit.
|  * If no bits are zero, returns @size.

"If no bits are zero, returns @size or a number larger than @size."

|  */
| extern unsigned long find_first_zero_bit(const unsigned long *addr,
|                                          unsigned long size);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Julia Lawall]

On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:

> Hi Julia,
>
> On Wed, Jun 4, 2014 at 11:52 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> Maybe the documented return code should be changed to allow for the
> >> existing behaviour.
> >
> > Sorry, I'm not sure to understand what you suggest here.
>
> include/asm-generic/bitops/find.h:
>
> | /**
> |  * find_first_zero_bit - find the first cleared bit in a memory region
> |  * @addr: The address to start the search at
> |  * @size: The maximum number of bits to search
> |  *
> |  * Returns the bit number of the first cleared bit.
> |  * If no bits are zero, returns @size.
>
> "If no bits are zero, returns @size or a number larger than @size."

OK, thanks.  I was only looking at the C code.

But the C code contains a loop that is followed by:

        if (!size)
                return result;
        tmp = *p;

found_first:
        tmp |= ~0UL << size;
        if (tmp == ~0UL)        /* Are any bits zero? */
                return result + size;   /* Nope. */

In the first return, it would seem that result == size.  Could the second
one be changed to just return size?  It should not hurt performance.

julia

>
> |  */
> | extern unsigned long find_first_zero_bit(const unsigned long *addr,
> |                                          unsigned long size);
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Geert Uytterhoeven]

Hi Julia,

On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> OK, thanks.  I was only looking at the C code.
>
> But the C code contains a loop that is followed by:
>
>         if (!size)
>                 return result;
>         tmp = *p;
>
> found_first:
>         tmp |= ~0UL << size;
>         if (tmp == ~0UL)        /* Are any bits zero? */
>                 return result + size;   /* Nope. */
>
> In the first return, it would seem that result == size.  Could the second
> one be changed to just return size?  It should not hurt performance.

"size" may have been changed between function entry and this line.
So you have to store it in a temporary.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[Julia Lawall]

On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:

> Hi Julia,
>
> On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > OK, thanks.  I was only looking at the C code.
> >
> > But the C code contains a loop that is followed by:
> >
> >         if (!size)
> >                 return result;
> >         tmp = *p;
> >
> > found_first:
> >         tmp |= ~0UL << size;
> >         if (tmp == ~0UL)        /* Are any bits zero? */
> >                 return result + size;   /* Nope. */
> >
> > In the first return, it would seem that result == size.  Could the second
> > one be changed to just return size?  It should not hurt performance.
>
> "size" may have been changed between function entry and this line.
> So you have to store it in a temporary.

Sorry, after reflection it seems that indeed size + result is always the
original size, so it is actually all of the code that uses >= that is
doing something unnecessary.  == for the failure test is fine.

julia

RE: [PATCH 0/10] use safer test on the result of find_first_zero_bit

[David Laight]

From: Julia Lawall
> On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> 
> > Hi Julia,
> >
> > On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > > OK, thanks.  I was only looking at the C code.
> > >
> > > But the C code contains a loop that is followed by:
> > >
> > >         if (!size)
> > >                 return result;
> > >         tmp = *p;
> > >
> > > found_first:
> > >         tmp |= ~0UL << size;
> > >         if (tmp == ~0UL)        /* Are any bits zero? */
> > >                 return result + size;   /* Nope. */
> > >
> > > In the first return, it would seem that result == size.  Could the second
> > > one be changed to just return size?  It should not hurt performance.
> >
> > "size" may have been changed between function entry and this line.
> > So you have to store it in a temporary.
> 
> Sorry, after reflection it seems that indeed size + result is always the
> original size, so it is actually all of the code that uses >= that is
> doing something unnecessary.  == for the failure test is fine.

There is nothing wrong with defensive coding.
The 'tmp |= ~0UL << size' ensures that the return value is 'correct'
when there are no bits set.
The function could have been defined so that this wasn't needed.

If you assume that the 'no zero bits' is unlikely, then checking the
return value from ffz() could well be slightly faster.
Not that anything is likely to notice.

	David