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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 24 Apr 2017, at 12.24, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> 
>>> 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

Attachment: signature.asc
Description: Message signed with OpenPGP


[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux