lkml.org 
[lkml]   [2016]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 3.2 009/152] ext4: check for extents that wrap around
    From
    Date
    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 <vegard.nossum@oracle.com>
    >
    > 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 <guaneryu@gmail.com>
    > Signed-off-by: Phil Turnbull <phil.turnbull@oracle.com>
    > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
    > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
    > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    > ---
    > 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);
    > }
    >

    \
     
     \ /
      Last update: 2016-11-14 16:31    [W:5.217 / U:0.372 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site