Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753826AbcKNPaS (ORCPT ); Mon, 14 Nov 2016 10:30:18 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:25356 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751736AbcKNPaQ (ORCPT ); Mon, 14 Nov 2016 10:30:16 -0500 Subject: Re: [PATCH 3.2 009/152] ext4: check for extents that wrap around To: Ben Hutchings , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Greg Kroah-Hartman References: Cc: akpm@linux-foundation.org, Phil Turnbull , Eryu Guan , "Theodore Ts'o" From: Vegard Nossum Message-ID: <8ccb2d1b-cc2d-7e30-836b-47d56591365b@oracle.com> Date: Mon, 14 Nov 2016 16:29:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3058 Lines: 88 On 11/14/2016 01:14 AM, Ben Hutchings wrote: > 3.2.84-rc1 review patch. If anyone has any objections, please let me know. Just a general comment on stable review workflow, really: It might be more useful to send the diff-of-diffs with the upstream commit so I can easily see if you had any conflicts when cherry-picking this and how they were resolved. That's generally much more interesting than just the plain patch, where I can't really tell if there were any changes at all (or conversely, much more boring in case there were no changes, and thus easier to review). If you could push this commit to git before sending the review, you could also include a command that I can use to quickly do the diff-of-diffs myself without having to download and apply the patch (or look for it), e.g. something like (using the 3.12 stable commit vs upstream): """ diff -yw \ <(echo upstream; git log -p -W f70749c^..f70749c) \ <(echo 3.2; git log -p -W 33234c6^..33234c6) """ At least that would make it a lot easier for me (and I suspect other casual stable contributors) to glance at a stable review email and tell if the backport is correct or not. It should be pretty easy to script on your end(s) for the benefit of everybody. Just my 2 cents. Thanks, Vegard > ------------------ > > From: Vegard Nossum > > commit f70749ca42943faa4d4dcce46dfdcaadb1d0c4b6 upstream. > > An extent with lblock = 4294967295 and len = 1 will pass the > ext4_valid_extent() test: > > ext4_lblk_t last = lblock + len - 1; > > if (len == 0 || lblock > last) > return 0; > > since last = 4294967295 + 1 - 1 = 4294967295. This would later trigger > the BUG_ON(es->es_lblk + es->es_len < es->es_lblk) in ext4_es_end(). > > We can simplify it by removing the - 1 altogether and changing the test > to use lblock + len <= lblock, since now if len = 0, then lblock + 0 == > lblock and it fails, and if len > 0 then lblock + len > lblock in order > to pass (i.e. it doesn't overflow). > > Fixes: 5946d0893 ("ext4: check for overlapping extents in ext4_valid_extent_entries()") > Fixes: 2f974865f ("ext4: check for zero length extent explicitly") > Cc: Eryu Guan > Signed-off-by: Phil Turnbull > Signed-off-by: Vegard Nossum > Signed-off-by: Theodore Ts'o > Signed-off-by: Ben Hutchings > --- > fs/ext4/extents.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -319,9 +319,13 @@ static int ext4_valid_extent(struct inod > ext4_fsblk_t block = ext4_ext_pblock(ext); > int len = ext4_ext_get_actual_len(ext); > ext4_lblk_t lblock = le32_to_cpu(ext->ee_block); > - ext4_lblk_t last = lblock + len - 1; > > - if (len == 0 || lblock > last) > + /* > + * We allow neither: > + * - zero length > + * - overflow/wrap-around > + */ > + if (lblock + len <= lblock) > return 0; > return ext4_data_block_valid(EXT4_SB(inode->i_sb), block, len); > } >