From: "Darrick J. Wong" <darrick.wong@oracle.com> To: "Seymour, Shane M" <shane.seymour@hpe.com> Cc: Jens Axboe <axboe@kernel.dk>, Christoph Hellwig <hch@infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>, Jeff Layton <jlayton@poochiereds.net>, "J. Bruce Fields" <bfields@fieldses.org>, "martin.petersen@oracle.com" <martin.petersen@oracle.com> Subject: Re: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Date: Tue, 10 Nov 2015 22:14:35 -0800 [thread overview] Message-ID: <20151111061435.GA32272@birch.djwong.org> (raw) In-Reply-To: <DDB9C85B850785449757F9914A034FCB444D1676@G9W0766.americas.hpqcorp.net> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Darrick J. Wong" <darrick.wong-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> To: "Seymour, Shane M" <shane.seymour-ZPxbGqLxI0U@public.gmane.org> Cc: Jens Axboe <axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>, "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>, "martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org" <martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> Subject: Re: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Date: Tue, 10 Nov 2015 22:14:35 -0800 [thread overview] Message-ID: <20151111061435.GA32272@birch.djwong.org> (raw) In-Reply-To: <DDB9C85B850785449757F9914A034FCB444D1676-4I1V4pQFGigSZAcGdq5asR6epYMZPwEe5NbjCUgZEJk@public.gmane.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
next prev parent reply other threads:[~2015-11-11 6:15 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-10 5:15 [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Darrick J. Wong 2015-11-10 5:15 ` Darrick J. Wong 2015-11-11 5:30 ` Seymour, Shane M 2015-11-11 6:14 ` Darrick J. Wong [this message] 2015-11-11 6:14 ` Darrick J. Wong 2015-11-11 23:30 ` Seymour, Shane M 2015-11-12 0:56 ` Seymour, Shane M 2015-11-13 3:36 ` Seymour, Shane M 2015-11-13 15:47 ` Jens Axboe 2015-11-13 15:47 ` Jens Axboe 2015-11-13 21:36 ` Darrick J. Wong -- strict thread matches above, loose matches on Subject: below -- 2015-01-21 1:06 [PATCH] block: Add discard flag to blkdev_issue_zeroout() function Martin K. Petersen 2015-01-21 10:10 ` [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Darrick J. Wong 2014-11-07 5:08 [RFC] Discard update for 3.19 Martin K. Petersen 2014-11-07 5:08 ` [PATCH 3/3] block: Introduce blkdev_issue_zeroout_discard() function Martin K. Petersen 2014-11-11 0:04 ` Darrick J. Wong 2014-11-17 19:28 ` [PATCH] block: create ioctl to discard-or-zeroout a range of blocks Darrick J. Wong
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151111061435.GA32272@birch.djwong.org \ --to=darrick.wong@oracle.com \ --cc=axboe@kernel.dk \ --cc=bfields@fieldses.org \ --cc=hch@infradead.org \ --cc=jlayton@poochiereds.net \ --cc=linux-api@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=shane.seymour@hpe.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.