| Message ID | 20190318154520.4086-4-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Couple of coverity fixes | expand |
On 2019/3/18 下午11:45, Nikolay Borisov wrote: > qgroup_rsv_size is calculated as the product of > outstanding_extent * fs_info->nodesize. The product is calculated with > 32 bith precision since both variables are defined as u32. Yet > qgroup_rsv_size expects a 64 bit result. > > Avoid possible multiplication overflow by casting outstanding_extent to > u64. > > Fixes-coverity-id: 1435101 > ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv") > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/extent-tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index b085d8215f0e..beddf9eef4a2 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info, > * > * This is overestimating in most cases. > */ > - qgroup_rsv_size = outstanding_extents * fs_info->nodesize; > + qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize; I'm a little uncertain about what's the proper way to do a u32 * u32 and get a u64 in C. For division we have DIV macro but not for multiple. Thanks, Qu > > spin_lock(&block_rsv->lock); > block_rsv->size = reserve_size; >
On 19.03.19 г. 6:46 ч., Qu Wenruo wrote: > > > On 2019/3/18 下午11:45, Nikolay Borisov wrote: >> qgroup_rsv_size is calculated as the product of >> outstanding_extent * fs_info->nodesize. The product is calculated with >> 32 bith precision since both variables are defined as u32. Yet >> qgroup_rsv_size expects a 64 bit result. >> >> Avoid possible multiplication overflow by casting outstanding_extent to >> u64. >> >> Fixes-coverity-id: 1435101 >> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv") >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> --- >> fs/btrfs/extent-tree.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >> index b085d8215f0e..beddf9eef4a2 100644 >> --- a/fs/btrfs/extent-tree.c >> +++ b/fs/btrfs/extent-tree.c >> @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info, >> * >> * This is overestimating in most cases. >> */ >> - qgroup_rsv_size = outstanding_extents * fs_info->nodesize; >> + qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize; > > I'm a little uncertain about what's the proper way to do a u32 * u32 and > get a u64 in C. > > For division we have DIV macro but not for multiple. You should definitely read this: https://wiki.sei.cmu.edu/confluence/display/c/INT18-C.+Evaluate+integer+expressions+in+a+larger+size+before+comparing+or+assigning+to+that+size In particular the 2nd "Noncompliant Code Example " described there is exactly the case you have in this code. > > Thanks, > Qu > >> >> spin_lock(&block_rsv->lock); >> block_rsv->size = reserve_size; >> >
On 2019/3/19 下午2:48, Nikolay Borisov wrote: > > > On 19.03.19 г. 6:46 ч., Qu Wenruo wrote: >> >> >> On 2019/3/18 下午11:45, Nikolay Borisov wrote: >>> qgroup_rsv_size is calculated as the product of >>> outstanding_extent * fs_info->nodesize. The product is calculated with >>> 32 bith precision since both variables are defined as u32. Yet >>> qgroup_rsv_size expects a 64 bit result. >>> >>> Avoid possible multiplication overflow by casting outstanding_extent to >>> u64. >>> >>> Fixes-coverity-id: 1435101 >>> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv") >>> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>> --- >>> fs/btrfs/extent-tree.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>> index b085d8215f0e..beddf9eef4a2 100644 >>> --- a/fs/btrfs/extent-tree.c >>> +++ b/fs/btrfs/extent-tree.c >>> @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info, >>> * >>> * This is overestimating in most cases. >>> */ >>> - qgroup_rsv_size = outstanding_extents * fs_info->nodesize; >>> + qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize; >> >> I'm a little uncertain about what's the proper way to do a u32 * u32 and >> get a u64 in C. >> >> For division we have DIV macro but not for multiple. > > You should definitely read this: > > https://wiki.sei.cmu.edu/confluence/display/c/INT18-C.+Evaluate+integer+expressions+in+a+larger+size+before+comparing+or+assigning+to+that+size > > In particular the 2nd "Noncompliant Code Example > " described there is exactly the case you have in this code. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > >> >> Thanks, >> Qu >> >>> >>> spin_lock(&block_rsv->lock); >>> block_rsv->size = reserve_size; >>> >>
On Mon, Mar 18, 2019 at 05:45:20PM +0200, Nikolay Borisov wrote: > qgroup_rsv_size is calculated as the product of > outstanding_extent * fs_info->nodesize. The product is calculated with > 32 bith precision since both variables are defined as u32. Yet > qgroup_rsv_size expects a 64 bit result. > > Avoid possible multiplication overflow by casting outstanding_extent to > u64. > > Fixes-coverity-id: 1435101 > ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv") Fixes: hash ("subject") I've added a note about the worst case when the overflow would happen, which is 65536 outstanding extents with 64K nodesize. Unlikely to happen.
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index b085d8215f0e..beddf9eef4a2 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info, * * This is overestimating in most cases. */ - qgroup_rsv_size = outstanding_extents * fs_info->nodesize; + qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize; spin_lock(&block_rsv->lock); block_rsv->size = reserve_size;
qgroup_rsv_size is calculated as the product of outstanding_extent * fs_info->nodesize. The product is calculated with 32 bith precision since both variables are defined as u32. Yet qgroup_rsv_size expects a 64 bit result. Avoid possible multiplication overflow by casting outstanding_extent to u64. Fixes-coverity-id: 1435101 ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv") Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/extent-tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)