[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 1/10] sh: 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>

---
 arch/sh/kernel/perf_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -281,7 +281,7 @@ static int sh_pmu_add(struct perf_event
 
 	if (__test_and_set_bit(idx, cpuc->used_mask)) {
 		idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events);
-		if (idx == sh_pmu->num_events)
+		if (idx >= sh_pmu->num_events)
 			goto out;
 
 		__set_bit(idx, cpuc->used_mask);

[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);

[PATCH 3/10] video: 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/video/fbdev/sh_mobile_meram.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/drivers/video/fbdev/sh_mobile_meram.c b/drivers/video/fbdev/sh_mobile_meram.c
--- a/drivers/video/fbdev/sh_mobile_meram.c
+++ b/drivers/video/fbdev/sh_mobile_meram.c
@@ -222,7 +222,7 @@ static int meram_plane_alloc(struct sh_m
 	unsigned long idx;
 
 	idx = find_first_zero_bit(&priv->used_icb, 28);
-	if (idx == 28)
+	if (idx >= 28)
 		return -ENOMEM;
 	plane->cache = &priv->icbs[idx];
 

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On 04/06/14 12:07, Julia Lawall wrote:
> 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.

Thanks, queued for 3.17.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Julia Lawall]

On Mon, 23 Jun 2014, Tomi Valkeinen wrote:

> On 04/06/14 12:07, Julia Lawall wrote:
> > 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.
>
> Thanks, queued for 3.17.

No, sorry you can drop it.  It doesn't hurt anything, but it is not
necessary either.  Returning a larger number is actually not possible.

julia

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On 23/06/14 17:12, Julia Lawall wrote:
> On Mon, 23 Jun 2014, Tomi Valkeinen wrote:
> 
>> On 04/06/14 12:07, Julia Lawall wrote:
>>> 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.
>>
>> Thanks, queued for 3.17.
> 
> No, sorry you can drop it.  It doesn't hurt anything, but it is not
> necessary either.  Returning a larger number is actually not possible.

Ok, dropped.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/10] staging: tidspbridge: 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/staging/tidspbridge/rmgr/node.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -u -p a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -935,7 +935,7 @@ int node_connect(struct node_object *nod
 				 node2_type == NODE_DAISSOCKET)) {
 		/* Find available pipe */
 		pipe_id = find_first_zero_bit(hnode_mgr->pipe_map, MAXPIPES);
-		if (pipe_id == MAXPIPES) {
+		if (pipe_id >= MAXPIPES) {
 			status = -ECONNREFUSED;
 			goto out_unlock;
 		}
@@ -1008,7 +1008,7 @@ int node_connect(struct node_object *nod
 			status = -EINVAL;
 			goto out_unlock;
 		}
-		if (chnl_id == CHNL_MAXCHANNELS) {
+		if (chnl_id >= CHNL_MAXCHANNELS) {
 			status = -ECONNREFUSED;
 			goto out_unlock;
 		}

[PATCH 5/10] ARC: 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>

---
 arch/arc/kernel/perf_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/arch/arc/kernel/perf_event.c b/arch/arc/kernel/perf_event.c
--- a/arch/arc/kernel/perf_event.c
+++ b/arch/arc/kernel/perf_event.c
@@ -205,7 +205,7 @@ static int arc_pmu_add(struct perf_event
 	if (__test_and_set_bit(idx, arc_pmu->used_mask)) {
 		idx = find_first_zero_bit(arc_pmu->used_mask,
 					  arc_pmu->n_counters);
-		if (idx == arc_pmu->n_counters)
+		if (idx >= arc_pmu->n_counters)
 			return -EAGAIN;
 
 		__set_bit(idx, arc_pmu->used_mask);

[PATCH 6/10] hpsa: 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/scsi/hpsa.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
 	spin_lock_irqsave(&h->lock, flags);
 	do {
 		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
+		if (i >= h->nr_cmds) {
 			spin_unlock_irqrestore(&h->lock, flags);
 			return NULL;
 		}

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

[scameron]

On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> 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/scsi/hpsa.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
>  	spin_lock_irqsave(&h->lock, flags);
>  	do {
>  		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> -		if (i == h->nr_cmds) {
> +		if (i >= h->nr_cmds) {
>  			spin_unlock_irqrestore(&h->lock, flags);
>  			return NULL;
>  		}

Thanks, Ack.

You can add

Reviewed-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>

to this patch if you want.

You might also consider adding "Cc: stable@vger.kernel.org" to the sign-off area.

-- steve

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

[Julia Lawall]

On Wed, 4 Jun 2014, scameron@beardog.cce.hp.com wrote:

> On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> > 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/scsi/hpsa.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
> >  	spin_lock_irqsave(&h->lock, flags);
> >  	do {
> >  		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> > -		if (i == h->nr_cmds) {
> > +		if (i >= h->nr_cmds) {
> >  			spin_unlock_irqrestore(&h->lock, flags);
> >  			return NULL;
> >  		}
>
> Thanks, Ack.
>
> You can add
>
> Reviewed-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> to this patch if you want.
>
> You might also consider adding "Cc: stable@vger.kernel.org" to the sign-off area.

Actually, it seems that the function can never overshoot the specified
limit.  So the change is not needed.

julia

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

[scameron]

On Wed, Jun 04, 2014 at 05:06:44PM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 4 Jun 2014, scameron@beardog.cce.hp.com wrote:
> 
> > On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> > > 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/scsi/hpsa.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > > --- a/drivers/scsi/hpsa.c
> > > +++ b/drivers/scsi/hpsa.c
> > > @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
> > >  	spin_lock_irqsave(&h->lock, flags);
> > >  	do {
> > >  		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> > > -		if (i == h->nr_cmds) {
> > > +		if (i >= h->nr_cmds) {
> > >  			spin_unlock_irqrestore(&h->lock, flags);
> > >  			return NULL;
> > >  		}
> >
> > Thanks, Ack.
> >
> > You can add
> >
> > Reviewed-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >
> > to this patch if you want.
> >
> > You might also consider adding "Cc: stable@vger.kernel.org" to the sign-off area.
> 
> Actually, it seems that the function can never overshoot the specified
> limit.  So the change is not needed.

Well, that would explain why nobody has complained about it in all these years.
I figured that must have been the case at least on x86, but I thought maybe
other archictectures might behave differently, and the change seemed harmless
in any case. 

-- steve

[PATCH 7/10] cciss: 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/block/cciss.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
 
 	do {
 		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds)
+		if (i >= h->nr_cmds)
 			return NULL;
 	} while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
 	c = h->cmd_pool + i;

[PATCH 8/10] s390/pci: 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>

---
 arch/s390/pci/pci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -u -p a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -566,7 +566,7 @@ static int zpci_alloc_iomap(struct zpci_
 
 	spin_lock(&zpci_iomap_lock);
 	entry = find_first_zero_bit(zpci_iomap, ZPCI_IOMAP_MAX_ENTRIES);
-	if (entry == ZPCI_IOMAP_MAX_ENTRIES) {
+	if (entry >= ZPCI_IOMAP_MAX_ENTRIES) {
 		spin_unlock(&zpci_iomap_lock);
 		return -ENOSPC;
 	}
@@ -746,7 +746,7 @@ static int zpci_alloc_domain(struct zpci
 {
 	spin_lock(&zpci_domain_lock);
 	zdev->domain = find_first_zero_bit(zpci_domain, ZPCI_NR_DEVICES);
-	if (zdev->domain == ZPCI_NR_DEVICES) {
+	if (zdev->domain >= ZPCI_NR_DEVICES) {
 		spin_unlock(&zpci_domain_lock);
 		return -ENOSPC;
 	}

Re: [PATCH 8/10] s390/pci: use safer test on the result of find_first_zero_bit

[Sebastian Ott]

On Wed, 4 Jun 2014, Julia Lawall wrote:
> 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>
> 
> ---
>  arch/s390/pci/pci.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -u -p a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -566,7 +566,7 @@ static int zpci_alloc_iomap(struct zpci_
> 
>  	spin_lock(&zpci_iomap_lock);
>  	entry = find_first_zero_bit(zpci_iomap, ZPCI_IOMAP_MAX_ENTRIES);
> -	if (entry == ZPCI_IOMAP_MAX_ENTRIES) {
> +	if (entry >= ZPCI_IOMAP_MAX_ENTRIES) {
>  		spin_unlock(&zpci_iomap_lock);
>  		return -ENOSPC;
>  	}
> @@ -746,7 +746,7 @@ static int zpci_alloc_domain(struct zpci
>  {
>  	spin_lock(&zpci_domain_lock);
>  	zdev->domain = find_first_zero_bit(zpci_domain, ZPCI_NR_DEVICES);
> -	if (zdev->domain == ZPCI_NR_DEVICES) {
> +	if (zdev->domain >= ZPCI_NR_DEVICES) {
>  		spin_unlock(&zpci_domain_lock);
>  		return -ENOSPC;
>  	}
> 
> 

Thanks, applied.
Sebastian

[PATCH 9/10] IB/qib: 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/infiniband/hw/qib/qib_file_ops.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -1187,7 +1187,7 @@ static void assign_ctxt_affinity(struct
 		int cpu;
 		cpu = find_first_zero_bit(qib_cpulist,
 					  qib_cpulist_count);
-		if (cpu == qib_cpulist_count)
+		if (cpu >= qib_cpulist_count)
 			qib_dev_err(dd,
 			"no cpus avail for affinity PID %u\n",
 			current->pid);

RE: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit

[Marciniszyn, Mike]

> Subject: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit
> 
> From: Julia Lawall <Julia.Lawall@lip6.fr>

Thanks for the patch!

Roland, I'm marking this as stable since a memory corruption can occur in the _set_bit().

Cc: <stable@vger.kernel.org>
Acked-by: Mike Marciniszyn <mike.marciniszyn@intel.com>

RE: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit

[Julia Lawall]

On Wed, 4 Jun 2014, Marciniszyn, Mike wrote:

> > Subject: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit
> > 
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Thanks for the patch!
> 
> Roland, I'm marking this as stable since a memory corruption can occur in the _set_bit().

No, it's not necessary.  It turns out that the result cannot be greater 
than the requested maximum value.

julia

RE: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit

[Marciniszyn, Mike]

> 
> No, it's not necessary.  It turns out that the result cannot be greater than the
> requested maximum value.
> 
> Julia

Ok. 

No stable then.

Mike

[PATCH 10/10] blackfin: 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>

---
 arch/blackfin/kernel/perf_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -u -p a/arch/blackfin/kernel/perf_event.c b/arch/blackfin/kernel/perf_event.c
--- a/arch/blackfin/kernel/perf_event.c
+++ b/arch/blackfin/kernel/perf_event.c
@@ -354,7 +354,7 @@ static int bfin_pmu_add(struct perf_even
 
 	if (__test_and_set_bit(idx, cpuc->used_mask)) {
 		idx = find_first_zero_bit(cpuc->used_mask, MAX_HWEVENTS);
-		if (idx == MAX_HWEVENTS)
+		if (idx >= MAX_HWEVENTS)
 			goto out;
 
 		__set_bit(idx, cpuc->used_mask);

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