Messages in this thread | | | Subject | Re: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks | From | Jens Axboe <> | Date | Fri, 13 Nov 2015 08:47:20 -0700 |
| |
On 11/10/2015 11:14 PM, Darrick J. Wong wrote: > On Wed, Nov 11, 2015 at 05:30:07AM +0000, Seymour, Shane M wrote: >> A quick question about this part of the patch: >> >>> + uint64_t end = start + len - 1; >> >>> + if (end >= i_size_read(bdev->bd_inode)) >> return -EINVAL; >> >>> + /* Invalidate the page cache, including dirty pages */ >>> + mapping = bdev->bd_inode->i_mapping; >>> + truncate_inode_pages_range(mapping, start, end); >> >> blk_ioctl_zeroout accepts unsigned values for start and end (uint64_t) but >> loff_t types are turned from i_size_read() and passed as the 2nd and 3rd >> values to truncate_inode_pages_range() and loff_t is a signed value. It >> should be possible to pass in some values would overflow the calculation of >> end causing the test on the value of end and the result of i_size_read to >> pass but then end up passing a large unsigned value for in start that would >> be implicitly converted to signed in truncate_inode_pages_range. I was >> wondering if you'd tested passing in data that would cause sign conversion >> issues when passed into truncate_inode_pages_range (does it handle it >> gracefully?) or should this code: >> >> if (start & 511) >> return -EINVAL; >> if (len & 511) >> return -EINVAL; >> >> be something more like this (for better sanity checking of your arguments) >> which will ensure that you don't have implicit conversion issues from >> unsigned to signed and ensure that the result of adding them together won't >> either: >> >> if ((start & 511) || (start > (uint64_t)LLONG_MAX)) >> return -EINVAL; >> if ((len & 511) ) || (len > (uint64_t)LLONG_MAX)) >> return -EINVAL; >> if (end > (uint64_t)LLONG_MAX) >> return -EINVAL; >> >> My apologies in advance if I've made a mistake when looking at this and my >> concerns about unsigned values being implicitly converted to signed are >> unfounded (I would have hoped for compiler warnings about any implicit >> conversions though). > > I don't have a device large enough to test for signedness errors, since passing > huge values for start and len never make it past the i_size_read check. > However, I do see that I forgot to check the padding values, so I'll update > that.
modprobe null_blk nr_devices=1 gb=512000
will get you a /dev/nullb0 that is 500TB. Adjust 'gb' at will. Or use loop with a big ass sparse file.
-- Jens Axboe
| |