[bug report] dm: add statistics support

[bug report] dm: add statistics support

[Dan Carpenter]

[ This code is half a decade old so probably removing the dead code is
  fine?  - dan ]

Hello Mikulas Patocka,

The patch fd2ed4d25270: "dm: add statistics support" from Aug 16,
2013, leads to the following static checker warning:

	drivers/md/dm-stats.c:371 dm_stats_create()
	warn: dead code because of 's->id == ((~0 >> 1))' and 'tmp_s->id < s->id'

drivers/md/dm-stats.c
   361          mutex_lock(&stats->mutex);
   362          s->id = 0;
   363          list_for_each(l, &stats->list) {
   364                  tmp_s = container_of(l, struct dm_stat, list_entry);
   365                  if (WARN_ON(tmp_s->id < s->id)) {
                                    ^^^^^^^^^^^^^^^^^
This condition means that s->id can't be INT_MAX.

   366                          r = -EINVAL;
   367                          goto out_unlock_resume;
   368                  }
   369                  if (tmp_s->id > s->id)
   370                          break;
   371                  if (unlikely(s->id == INT_MAX)) {
                                     ^^^^^^^^^^^^^^^^
So we can probably remove this dead code?  Was something else intended?

   372                          r = -ENFILE;
   373                          goto out_unlock_resume;
   374                  }
   375                  s->id++;
   376          }
   377          ret_id = s->id;
   378          list_add_tail_rcu(&s->list_entry, l);
   379          mutex_unlock(&stats->mutex);
   380  
   381          resume_callback(md);
   382  
   383          return ret_id;
   384  
   385  out_unlock_resume:
   386          mutex_unlock(&stats->mutex);
   387          resume_callback(md);
   388  out:
   389          dm_stat_free(&s->rcu_head);
   390          return r;
   391  }

regards,
dan carpenter

Re: [bug report] dm: add statistics support

[Mikulas Patocka]

Hi

On Thu, 15 Mar 2018, Dan Carpenter wrote:

> [ This code is half a decade old so probably removing the dead code is
>   fine?  - dan ]
> 
> Hello Mikulas Patocka,
> 
> The patch fd2ed4d25270: "dm: add statistics support" from Aug 16,
> 2013, leads to the following static checker warning:
> 
> 	drivers/md/dm-stats.c:371 dm_stats_create()
> 	warn: dead code because of 's->id == ((~0 >> 1))' and 'tmp_s->id < s->id'

((~0 >> 1)) is -1 and we are comparing it against INT_MAX. Perhaps the 
static checker is buggy because it believes that INT_MAX is -1.

INT_MAX definition in the linux kernel is ((int)(~0U>>1)).

> drivers/md/dm-stats.c
>    361          mutex_lock(&stats->mutex);
>    362          s->id = 0;
>    363          list_for_each(l, &stats->list) {
>    364                  tmp_s = container_of(l, struct dm_stat, list_entry);
>    365                  if (WARN_ON(tmp_s->id < s->id)) {
>                                     ^^^^^^^^^^^^^^^^^
> This condition means that s->id can't be INT_MAX.
> 
>    366                          r = -EINVAL;
>    367                          goto out_unlock_resume;
>    368                  }
>    369                  if (tmp_s->id > s->id)
>    370                          break;
>    371                  if (unlikely(s->id == INT_MAX)) {
>                                      ^^^^^^^^^^^^^^^^
> So we can probably remove this dead code?  Was something else intended?

I don't think this is dead code. If tmp_s->id == INT_MAX and s->id == 
INT_MAX, then the first condition (tmp_s->id < s->id) is false, the second 
condition (tmp_s->id > s->id) is false. And we can hit this line and the 
condition (s->id == INT_MAX) is true.

This condition prevents an integer overflow if the user creates 2^31 
entries. It is unlikely to happen, but it is theoretically possible.

Mikulas

>    372                          r = -ENFILE;
>    373                          goto out_unlock_resume;
>    374                  }
>    375                  s->id++;
>    376          }
>    377          ret_id = s->id;
>    378          list_add_tail_rcu(&s->list_entry, l);
>    379          mutex_unlock(&stats->mutex);
>    380  
>    381          resume_callback(md);
>    382  
>    383          return ret_id;
>    384  
>    385  out_unlock_resume:
>    386          mutex_unlock(&stats->mutex);
>    387          resume_callback(md);
>    388  out:
>    389          dm_stat_free(&s->rcu_head);
>    390          return r;
>    391  }
> 
> regards,
> dan carpenter
> 

Re: [bug report] dm: add statistics support

[Dan Carpenter]

Gar.  I'm sorry, this is really new code and you're right that it's
buggy.

On Thu, Mar 15, 2018 at 03:25:52PM -0400, Mikulas Patocka wrote:
> > The patch fd2ed4d25270: "dm: add statistics support" from Aug 16,
> > 2013, leads to the following static checker warning:
> > 
> > 	drivers/md/dm-stats.c:371 dm_stats_create()
> > 	warn: dead code because of 's->id == ((~0 >> 1))' and 'tmp_s->id < s->id'
> 
> ((~0 >> 1)) is -1 and we are comparing it against INT_MAX. Perhaps the 
> static checker is buggy because it believes that INT_MAX is -1.
> 
> INT_MAX definition in the linux kernel is ((int)(~0U>>1)).
> 

Yeah, the format of the smatch outputs is bad.  It gets INT_MAX correct
internally it just prints it out badly.  The problem here is that Smatch
is crap at parsing loops.  Parsing seems like an essential thing, but
you'd be surprised how many times you can get away with parsing them
badly.  The main issue is that parsing them correctly is probably a big
slow down because you'd have to go through a lot of code twice...

Anyway, thanks for looking at this and sorry for the noise.

regards,
dan carpenter

Re: [bug report] dm: add statistics support

[Dan Carpenter]

On Fri, Mar 16, 2018 at 01:52:20PM +0300, Dan Carpenter wrote:
> Gar.  I'm sorry, this is really new code and you're right that it's
> buggy.
> 
> On Thu, Mar 15, 2018 at 03:25:52PM -0400, Mikulas Patocka wrote:
> > > The patch fd2ed4d25270: "dm: add statistics support" from Aug 16,
> > > 2013, leads to the following static checker warning:
> > > 
> > > 	drivers/md/dm-stats.c:371 dm_stats_create()
> > > 	warn: dead code because of 's->id == ((~0 >> 1))' and 'tmp_s->id < s->id'
> > 
> > ((~0 >> 1)) is -1 and we are comparing it against INT_MAX. Perhaps the 
> > static checker is buggy because it believes that INT_MAX is -1.
> > 
> > INT_MAX definition in the linux kernel is ((int)(~0U>>1)).
> > 
> 
> Yeah, the format of the smatch outputs is bad.  It gets INT_MAX correct
> internally it just prints it out badly.  The problem here is that Smatch
> is crap at parsing loops.  Parsing seems like an essential thing, but

Parsing *loops*, I meant.

regards,
dan carpenter

> you'd be surprised how many times you can get away with parsing them
> badly.  The main issue is that parsing them correctly is probably a big
> slow down because you'd have to go through a lot of code twice...
> 
> Anyway, thanks for looking at this and sorry for the noise.
> 
> regards,
> dan carpenter