block: create ioctl to discard-or-zeroout a range of blocks
diff mbox

Message ID 20151110051526.GA2217@birch.djwong.org
State New
Headers show

Commit Message

Darrick J. Wong Nov. 10, 2015, 5:15 a.m. UTC
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

Comments

Seymour, Shane M Nov. 11, 2015, 5:30 a.m. UTC | #1
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
Darrick J. Wong Nov. 11, 2015, 6:14 a.m. UTC | #2
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
Seymour, Shane M Nov. 11, 2015, 11:30 p.m. UTC | #3
> 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
Seymour, Shane M Nov. 12, 2015, 12:56 a.m. UTC | #4
> 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
Seymour, Shane M Nov. 13, 2015, 3:36 a.m. UTC | #5
> 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
Jens Axboe Nov. 13, 2015, 3:47 p.m. UTC | #6
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.
Darrick J. Wong Nov. 13, 2015, 9:36 p.m. UTC | #7
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

Patch
diff mbox

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 */