diff mbox

[1/2] lightnvm: potential underflow in pblk_read_rq()

Message ID 20170421205301.3ds7jrgcktuldvrp@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter April 21, 2017, 8:53 p.m. UTC
This is a static checker fix, and perhaps not a real bug.  The static
checker thinks that nr_secs could be negative.  It would result in
zeroing more memory than intended.  Anyway, even if it's not a bug,
changing this variable to unsigned makes the code easier to audit.

Fixes: a4bd217b4326 ("lightnvm: physical block device (pblk) target")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

=?UTF-8?q?Javier=20Gonz=C3=A1lez?= April 21, 2017, 10:24 p.m. UTC | #1
> On 21 Apr 2017, at 22.53, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> This is a static checker fix, and perhaps not a real bug.  The static
> checker thinks that nr_secs could be negative.  It would result in
> zeroing more memory than intended.  Anyway, even if it's not a bug,
> changing this variable to unsigned makes the code easier to audit.
> 
> Fixes: a4bd217b4326 ("lightnvm: physical block device (pblk) target")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index bce7ed5fc73f..c9daa33e8d9c 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -288,7 +288,7 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
> int pblk_submit_read(struct pblk *pblk, struct bio *bio)
> {
> 	struct nvm_tgt_dev *dev = pblk->dev;
> -	int nr_secs = pblk_get_secs(bio);
> +	unsigned int nr_secs = pblk_get_secs(bio);
> 	struct nvm_rq *rqd;
> 	unsigned long read_bitmap; /* Max 64 ppas per request */
> 	unsigned int bio_init_idx;

Thanks Dan. While you are at it, can you also modify the type on the
other 2 calls to pblk_get_secs in pblk-cache and pblk-core?

Otherwise, it is a good catch.

Reviewed-by: Javier González <javier@cnexlabs.com>

Javier
Jens Axboe April 21, 2017, 10:50 p.m. UTC | #2
On 04/21/2017 02:53 PM, Dan Carpenter wrote:
> This is a static checker fix, and perhaps not a real bug.  The static
> checker thinks that nr_secs could be negative.  It would result in
> zeroing more memory than intended.  Anyway, even if it's not a bug,
> changing this variable to unsigned makes the code easier to audit.
> 
> Fixes: a4bd217b4326 ("lightnvm: physical block device (pblk) target")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Added, thanks Dan.
Dan Carpenter April 24, 2017, 10:24 a.m. UTC | #3
On Sat, Apr 22, 2017 at 12:24:50AM +0200, Javier González wrote:
> > On 21 Apr 2017, at 22.53, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > This is a static checker fix, and perhaps not a real bug.  The static
> > checker thinks that nr_secs could be negative.  It would result in
> > zeroing more memory than intended.  Anyway, even if it's not a bug,
> > changing this variable to unsigned makes the code easier to audit.
> > 
> > Fixes: a4bd217b4326 ("lightnvm: physical block device (pblk) target")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> > index bce7ed5fc73f..c9daa33e8d9c 100644
> > --- a/drivers/lightnvm/pblk-read.c
> > +++ b/drivers/lightnvm/pblk-read.c
> > @@ -288,7 +288,7 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
> > int pblk_submit_read(struct pblk *pblk, struct bio *bio)
> > {
> > 	struct nvm_tgt_dev *dev = pblk->dev;
> > -	int nr_secs = pblk_get_secs(bio);
> > +	unsigned int nr_secs = pblk_get_secs(bio);
> > 	struct nvm_rq *rqd;
> > 	unsigned long read_bitmap; /* Max 64 ppas per request */
> > 	unsigned int bio_init_idx;
> 
> Thanks Dan. While you are at it, can you also modify the type on the
> other 2 calls to pblk_get_secs in pblk-cache and pblk-core?
> 

Ugh...  My patch wasn't needed at all.  I should have looked more
carefully.  pblk_get_secs() can only return up to UINT_MAX / 4096 so
it can't overflow to negative.

pblk_get_secs() should probably return sector_t instead of unsigned int?

I do get another static checker warning caused by the pblk-cache code.
drivers/lightnvm/pblk-rl.c:30 pblk_rl_user_may_insert() XXX: potential integer overflow from user 'rb_user_cnt + nr_entries'

It's seems like a true warning but harmless.

drivers/lightnvm/pblk-rb.c
   425  /*
   426   * Atomically check that (i) there is space on the write buffer for the
   427   * incoming I/O, and (ii) the current I/O type has enough budget in the write
   428   * buffer (rate-limiter).
   429   */
   430  int pblk_rb_may_write_user(struct pblk_rb *rb, struct bio *bio,
   431                             unsigned int nr_entries, unsigned int *pos)
                                                ^^^^^^^^^^
Assume this is very high.

   432  {
   433          struct pblk *pblk = container_of(rb, struct pblk, rwb);
   434          int flush_done;
   435  
   436          spin_lock(&rb->w_lock);
   437          if (!pblk_rl_user_may_insert(&pblk->rl, nr_entries)) {
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We can have an integer overflow in here so this passes.

   438                  spin_unlock(&rb->w_lock);
   439                  return NVM_IO_REQUEUE;
   440          }
   441  
   442          if (!pblk_rb_may_write_flush(rb, nr_entries, pos, bio, &flush_done)) {
                     ^^^^^^^^^^^^^^^^^^^^^^^
But this check won't pass, so it's harmless.

   443                  spin_unlock(&rb->w_lock);
   444                  return NVM_IO_REQUEUE;
   445          }
   446  
   447          pblk_rl_user_in(&pblk->rl, nr_entries);
   448          spin_unlock(&rb->w_lock);
   449  
   450          return flush_done;
   451  }

regards,
dan carpenter
=?UTF-8?q?Javier=20Gonz=C3=A1lez?= April 24, 2017, 11:01 a.m. UTC | #4
> On 24 Apr 2017, at 12.24, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Sat, Apr 22, 2017 at 12:24:50AM +0200, Javier González wrote:
>>> On 21 Apr 2017, at 22.53, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>> 
>>> This is a static checker fix, and perhaps not a real bug.  The static
>>> checker thinks that nr_secs could be negative.  It would result in
>>> zeroing more memory than intended.  Anyway, even if it's not a bug,
>>> changing this variable to unsigned makes the code easier to audit.
>>> 
>>> Fixes: a4bd217b4326 ("lightnvm: physical block device (pblk) target")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> 
>>> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
>>> index bce7ed5fc73f..c9daa33e8d9c 100644
>>> --- a/drivers/lightnvm/pblk-read.c
>>> +++ b/drivers/lightnvm/pblk-read.c
>>> @@ -288,7 +288,7 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>> int pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>> {
>>> 	struct nvm_tgt_dev *dev = pblk->dev;
>>> -	int nr_secs = pblk_get_secs(bio);
>>> +	unsigned int nr_secs = pblk_get_secs(bio);
>>> 	struct nvm_rq *rqd;
>>> 	unsigned long read_bitmap; /* Max 64 ppas per request */
>>> 	unsigned int bio_init_idx;
>> 
>> Thanks Dan. While you are at it, can you also modify the type on the
>> other 2 calls to pblk_get_secs in pblk-cache and pblk-core?
> 
> Ugh...  My patch wasn't needed at all.  I should have looked more
> carefully.  pblk_get_secs() can only return up to UINT_MAX / 4096 so
> it can't overflow to negative.
> 
> pblk_get_secs() should probably return sector_t instead of unsigned int?

Since pblk_get_secs() divides an unsigned int (bio->bi_iter.bi_size), I
think unsigned int would suffice. Why sector_t? In any case, the calls
to this function do convert the type unnecessarily and should be
changed; so I would say that your patch is still a good cleanup.

> 
> I do get another static checker warning caused by the pblk-cache code.
> drivers/lightnvm/pblk-rl.c:30 pblk_rl_user_may_insert() XXX: potential integer overflow from user 'rb_user_cnt + nr_entries'
> 

Can I ask which static checker you use? I don't see these warning
neither on sparse nor in smatch.

> It's seems like a true warning but harmless.
> 
> drivers/lightnvm/pblk-rb.c
>   425  /*
>   426   * Atomically check that (i) there is space on the write buffer for the
>   427   * incoming I/O, and (ii) the current I/O type has enough budget in the write
>   428   * buffer (rate-limiter).
>   429   */
>   430  int pblk_rb_may_write_user(struct pblk_rb *rb, struct bio *bio,
>   431                             unsigned int nr_entries, unsigned int *pos)
>                                                ^^^^^^^^^^
> Assume this is very high.

This can be bigger than the reserved entries for user I/O on the write
buffer. In pblk-init, we split the bio if necessary (pblk_rw_io); I
should probably add a comment. on pblk_rl_user_may_insert().

> 
>   432  {
>   433          struct pblk *pblk = container_of(rb, struct pblk, rwb);
>   434          int flush_done;
>   435
>   436          spin_lock(&rb->w_lock);
>   437          if (!pblk_rl_user_may_insert(&pblk->rl, nr_entries)) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We can have an integer overflow in here so this passes.
> 
>   438                  spin_unlock(&rb->w_lock);
>   439                  return NVM_IO_REQUEUE;
>   440          }
>   441
>   442          if (!pblk_rb_may_write_flush(rb, nr_entries, pos, bio, &flush_done)) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^
> But this check won't pass, so it's harmless.

Exactly. This is related to the above.

> 
>   443                  spin_unlock(&rb->w_lock);
>   444                  return NVM_IO_REQUEUE;
>   445          }
>   446
>   447          pblk_rl_user_in(&pblk->rl, nr_entries);
>   448          spin_unlock(&rb->w_lock);
>   449
>   450          return flush_done;
>   451  }
> 

How do you typically get rid of these "harmless" warnings?

> regards,
> dan carpenter

Thanks,
Javier
diff mbox

Patch

diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index bce7ed5fc73f..c9daa33e8d9c 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -288,7 +288,7 @@  static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
 int pblk_submit_read(struct pblk *pblk, struct bio *bio)
 {
 	struct nvm_tgt_dev *dev = pblk->dev;
-	int nr_secs = pblk_get_secs(bio);
+	unsigned int nr_secs = pblk_get_secs(bio);
 	struct nvm_rq *rqd;
 	unsigned long read_bitmap; /* Max 64 ppas per request */
 	unsigned int bio_init_idx;