| Message ID | 20151110051526.GA2217@birch.djwong.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). Thanks Shane -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. --D > > Thanks > Shane -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. Then do you want to at least consider converting end to be of type loff_t to match the type of value returned by i_get_size() and the type of the value passed to truncate_inode_pages_range()? Then you only need one additional check, something like: /* Check for an overflow when adding start+len into end */ if (start > (uint64_t)LLONG_MAX - len) return -EINVAL; Looking at the maximum values if we have start=0 and len=(uint64_t)LLONG_MAX we would still continue but len=(uint64_t)LLONG_MAX+1 would return -EINVAL. Similarly for start=(uint64_t)LLONG_MAX and len=0 it would continue but start=(uint64_t)LLONG_MAX+1 would return -EINVAL and that should allow start + len to be safely added and stored into an loff_t without overflow. With this check start+len can never be more than (uint64_t)LLONG_MAX which would make the other checks I suggested to ensure that neither start or end were more than (uint64_t)LLONG_MAX unnecessary. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> which would make the other checks I suggested to ensure that neither start > or end were more than (uint64_t)LLONG_MAX unnecessary. My apologies I was wrong about what I said above - after thinking about it for longer you still need to make sure that at least len is not greater than (uint64_t)LLONG_MAX because in the calculation: if (start > (uint64_t)LLONG_MAX - len) return -EINVAL; if len was more than (uint64_t)LLONG_MAX it would underflow and become a very large positive number and start would never be greater than that (and I said end when I should have said len). -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. If you have someone trying to bypass your sanity checks then if start=18446744073709551104 and len=1024 the result of adding them together will be 512 (subtracting an extra 1 in the patched code to get 511 for end). That will pass the i_size_read check won't it? If so that would cause lstart in truncate_inode_pages_range() to be -512. I don't know what truncate_inode_pages_range() will do with a negative lstart value like that but it seems like an unusual value for your code to be willing to pass into truncate_inode_pages_range(). Shane -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Fri, Nov 13, 2015 at 08:47:20AM -0700, Jens Axboe wrote: > 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 I tried doing that for some huge device sizes and this is what I saw: # modprobe null_blk nr_devices=1 gb=17179869184 modprobe: ERROR: could not insert 'null_blk': Numerical result out of range # modprobe null_blk nr_devices=1 gb=8589934591 modprobe: ERROR: could not insert 'null_blk': Numerical result out of range # modprobe null_blk nr_devices=1 gb=8589934592 modprobe: ERROR: could not insert 'null_blk': Numerical result out of range # modprobe null_blk nr_devices=1 gb=4294967295 modprobe: ERROR: could not insert 'null_blk': Numerical result out of range # modprobe null_blk nr_devices=1 gb=4294967294 modprobe: ERROR: could not insert 'null_blk': Numerical result out of range # modprobe null_blk nr_devices=1 gb=2147483647 # lsblk /dev/nullb0 NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT nullb0 249:0 0 2E 0 disk Looks like the biggest nullblk device I can create is 2E? (Same result with "modprobe scsi-debug virtual_gb=...") > will get you a /dev/nullb0 that is 500TB. Adjust 'gb' at will. Or > use loop with a big ass sparse file. I tried that, too. XFS only wanted to let me create an 8E file. So did btrfs. Eventually, I figured out the following: # echo '0 18446744073709551616 zero' | dmsetup create MOO-backing # echo '0 18446744073709551616 snapshot /dev/mapper/MOO-backing /dev/sda N 512' | dmsetup create MOO # lsblk /dev/mapper/MOO NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT MOO (dm-1) 251:1 0 16E 0 dm Well, now we're getting somewhere. It's a little funny that we asked for 2^64 sectors, the dm table says 2^64, but the device claims a size of 2^55... but it's 16E which exhausts our loff_t. Let's try wrapping around the end: # /djwong/t/blkzero/test /dev/mapper/MOO 18446744073709543424 4096 OFFSET=-8192 BUFSZ=4096 USER_COHERENCE=0 DIRECTIO=0 EXCLUSIVE=0 # /djwong/t/blkzero/test /dev/mapper/MOO 18446744073709543424 8192 OFFSET=-8192 BUFSZ=8192 USER_COHERENCE=0 DIRECTIO=0 EXCLUSIVE=0 /dev/mapper/MOO: Invalid argument # /djwong/t/blkzero/test /dev/mapper/MOO 18446744073709543424 12288 OFFSET=-8192 BUFSZ=12288 USER_COHERENCE=0 DIRECTIO=0 EXCLUSIVE=0 # Hmm. The third case should fail, since that clearly goes off the end of the device. However, it's not correct to compare start or end against LLONG_MAX because the block layer clearly supports devices that are larger than LLONG_MAX bytes. However, the case where the "end" calculation overflows should be easy to spot with a quick "if (end < start) return -EINVAL;" Curiously, if I create the DM device with 2^55 sectors, the dm snapshot errors out: # echo '0 36028797018963967 zero' | dmsetup create MOO-backing # echo '0 36028797018963967 snapshot /dev/mapper/MOO-backing /dev/sda N 512' | dmsetup create MOO # /djwong/t/blkzero/test /dev/mapper/MOO 18446744073709543424 4096 OFFSET=-8192 BUFSZ=4096 USER_COHERENCE=0 DIRECTIO=0 EXCLUSIVE=0 /dev/mapper/MOO: Input/output error and dmesg spits out: "device-mapper: snapshots: Invalidating snapshot: Error reading/writing." FWIW, truncate_inode_pages_range takes the loff_t and converts that into a pgoff_t, which is unsigned long. If the start pgoff_t > the end pgoff_t, the function does nothing. On the off chance the blkdev_issue_zeroout call doesn't fail when it's given weird arguments, invalidate_inode_pages2_range seems to do roughly the same thing with pgoff_t. Will add the one check and resend. --D -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/ioctl.c b/block/ioctl.c index 8061eba..79a2e1c 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -213,19 +213,39 @@ static int blk_ioctl_discard(struct block_device *bdev, uint64_t start, } static int blk_ioctl_zeroout(struct block_device *bdev, uint64_t start, - uint64_t len) + uint64_t len, uint32_t flags) { + int ret; + struct address_space *mapping; + uint64_t end = start + len - 1; + + if (flags & ~BLKZEROOUT2_DISCARD_OK) + return -EINVAL; if (start & 511) return -EINVAL; if (len & 511) return -EINVAL; - start >>= 9; - len >>= 9; - - if (start + len > (i_size_read(bdev->bd_inode) >> 9)) + if (end >= i_size_read(bdev->bd_inode)) return -EINVAL; - return blkdev_issue_zeroout(bdev, start, len, GFP_KERNEL, false); + /* Invalidate the page cache, including dirty pages */ + mapping = bdev->bd_inode->i_mapping; + truncate_inode_pages_range(mapping, start, end); + + ret = blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL, + flags & BLKZEROOUT2_DISCARD_OK); + if (ret) + goto out; + + /* + * Invalidate again; if someone wandered in and dirtied a page, + * the caller will be given -EBUSY. + */ + ret = invalidate_inode_pages2_range(mapping, + start >> PAGE_CACHE_SHIFT, + end >> PAGE_CACHE_SHIFT); +out: + return ret; } static int put_ushort(unsigned long arg, unsigned short val) @@ -353,7 +373,18 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd, if (copy_from_user(range, (void __user *)arg, sizeof(range))) return -EFAULT; - return blk_ioctl_zeroout(bdev, range[0], range[1]); + return blk_ioctl_zeroout(bdev, range[0], range[1], 0); + } + case BLKZEROOUT2: { + struct blkzeroout2 p; + + if (!(mode & FMODE_WRITE)) + return -EBADF; + + if (copy_from_user(&p, (void __user *)arg, sizeof(p))) + return -EFAULT; + + return blk_ioctl_zeroout(bdev, p.start, p.length, p.flags); } case HDIO_GETGEO: { diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index 9b964a5..b811fa4 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -152,6 +152,15 @@ struct inodes_stat_t { #define BLKSECDISCARD _IO(0x12,125) #define BLKROTATIONAL _IO(0x12,126) #define BLKZEROOUT _IO(0x12,127) +struct blkzeroout2 { + __u64 start; + __u64 length; + __u32 flags; + __u32 padding; + __u64 padding2; +}; +#define BLKZEROOUT2_DISCARD_OK 1 +#define BLKZEROOUT2 _IOR(0x12, 127, struct blkzeroout2) #define BMAP_IOCTL 1 /* obsolete - kept for compatibility */ #define FIBMAP _IO(0x00,1) /* bmap access */
Create a new ioctl to expose the block layer's newfound ability to issue either a zeroing discard, a WRITE SAME with a zero page, or a regular write with the zero page. This BLKZEROOUT2 ioctl takes {start, length, flags} as parameters. So far, the only flag available is to enable the zeroing discard part -- without it, the call invokes the old BLKZEROOUT behavior. start and length have the same meaning as in BLKZEROOUT. Furthermore, because BLKZEROOUT2 issues commands directly to the storage device, we must invalidate the page cache (as a regular O_DIRECT write would do) to avoid returning stale cache contents at a later time. BLKZEROOUT never did this, which lead to coherency problems when e2fsprogs tried to use it. v2: Padded the structure with extra space for future fun, since the maintainer never responded... Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- block/ioctl.c | 45 ++++++++++++++++++++++++++++++++++++++------- include/uapi/linux/fs.h | 9 +++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html