> 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
