From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751826AbbKKGPM (ORCPT ); Wed, 11 Nov 2015 01:15:12 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:31154 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750961AbbKKGPI (ORCPT ); Wed, 11 Nov 2015 01:15:08 -0500 Date: Tue, 10 Nov 2015 22:14:35 -0800 From: "Darrick J. Wong" To: "Seymour, Shane M" Cc: Jens Axboe , Christoph Hellwig , "linux-kernel@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "linux-api@vger.kernel.org" , Jeff Layton , "J. Bruce Fields" , "martin.petersen@oracle.com" Subject: Re: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Message-ID: <20151111061435.GA32272@birch.djwong.org> References: <20151110051526.GA2217@birch.djwong.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Darrick J. Wong" Subject: Re: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Date: Tue, 10 Nov 2015 22:14:35 -0800 Message-ID: <20151111061435.GA32272@birch.djwong.org> References: <20151110051526.GA2217@birch.djwong.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Seymour, Shane M" Cc: Jens Axboe , Christoph Hellwig , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jeff Layton , "J. Bruce Fields" , "martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org" List-Id: linux-api@vger.kernel.org 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