On 09/18/2012 02:34 PM, Tino Reichardt wrote:
> * Dan Carpenter <[email protected]> wrote:
>> Hello Tino Reichardt,
>>
>> The patch 0d59722ea777: "fs/jfs: TRIM support for JFS Filesystem"
>> from Aug 29, 2012, leads to the following static checker warning:
>> fs/jfs/jfs_dmap.c:1650 dbDiscardAG()
>> warn: check 'range_cnt' for negative values
>
> I just compiled smatch from http://smatch.sourceforge.net/ and run the
> building of JFS with the C=1 and C=2 options. It seems no problem for
> this issue, but there are several others[1].
>
> What is static checker should I run, to get the warning ?
>
>>
>> 1648 nblocks = bmp->db_agfree[agno];
>> 1649 range_cnt = min_t(int, range_cnt, nblocks / minlen + 1);
>> ^^^
>> Could we make this unsigned? The caller checks that minlen is >= 1 and
>> probably someone checks nblocks as well, but it's annoying to have to
>> audit this.
>
> Yes, unsigned should be fine, minlen can also be changed to u64 and
> nblocks should be >= 0 ... I have no idea why dn_agfree[MAXAG] is
> definded as s64 - Dave, du you know more about that?
A lot of the data structures in jfs that should never have a value large
enough to use the sign bit have been defined this way since the initial
design. I'm not crazy about it, but I don't plan on changing it all over
the code.
Anyway, I've already changed that code, first because division of a
64-bit integer is not allowed in a 32-bit kernel, and again because my
first fix was lacking.
I thought I had cc'ed you on both patches, but looking back, it doesn't
look like I did on the second one. I did cc jfs-discussion.
> @ jfs_dmap.h: s64 dn_agfree[MAXAG]; /* per AG free count */
> There are also 3 other variables, which could be used as u64 there ...
> or not? I am not sure about that... ;)
>
>
> I could also make a patch for it, but I have no idea about the
> checker... is it @ linux/scripts? Can you give me some hint?
I've already patched it.
> So I can check that min_t() issue myself ;)
>
>> 1650 totrim = kmalloc(sizeof(struct range2trim) * range_cnt,
>> GFP_NOFS);
>> 1651 if (totrim == NULL) {
>> 1652 jfs_error(bmp->db_ipbmap->i_sb,
>> 1653 "dbDiscardAG: no memory for trim array");
>> 1654 IWRITE_UNLOCK(ipbmap);
>> 1655 return 0;
>> 1656 }
>
>
> [1] Errors from smatch, Dave - we should take a look @ it...
> http://www.mcmilk.de/projects/jfs-trim/jfs-smatch-errors.txt
Agreed. We should look at these. I'll happily take patches if you get to
it before me.
Thanks,
Dave
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Jfs-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jfs-discussion
