[PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle() - Kernel

This is a discussion on [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle() - Kernel ; Hi, I met a bug when I try to replace a xattr entry in ibody with a big size value. But in ibody there has no space for the new value. So it should set new xattr entry in block ...

+ Reply to Thread
Results 1 to 13 of 13

Thread: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

  1. [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Hi,
    I met a bug when I try to replace a xattr entry in ibody with a big size
    value. But in ibody there has no space for the new value. So it should
    set new xattr entry in block and remove the old xattr entry in ibody.

    Best regards,
    tiger


  2. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Mon, 12 May 2008 11:24:40 +0800
    Tiger Yang wrote:

    > I met a bug when I try to replace a xattr entry in ibody with a big size
    > value. But in ibody there has no space for the new value. So it should
    > set new xattr entry in block and remove the old xattr entry in ibody.
    >
    > Best regards,
    > tiger
    >
    >
    > [xattr.patch text/x-patch (1.3KB)]
    > This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
    >
    > Signed-off-by: Tiger Yang
    >
    > diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
    > index a6ea4d6..e1af9bd 100644
    > --- a/fs/ext3/xattr.c
    > +++ b/fs/ext3/xattr.c
    > @@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
    > i.value = NULL;
    > error = ext3_xattr_block_set(handle, inode, &i, &bs);
    > } else if (error == -ENOSPC) {
    > + if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
    > + error = ext3_xattr_block_find(inode, &i, &bs);
    > + if (error)
    > + goto cleanup;
    > + }
    > error = ext3_xattr_block_set(handle, inode, &i, &bs);
    > if (error)
    > goto cleanup;


    That sounds fairly bad.

    What are the consequences of this bug, when someone hits it?

    It appears that we should backport this fix into 2.6.25.x (and perhaps
    earlier). What do you think?

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Tiger Yang wrote:
    > Hi,
    > I met a bug when I try to replace a xattr entry in ibody with a big size
    > value. But in ibody there has no space for the new value. So it should
    > set new xattr entry in block and remove the old xattr entry in ibody.
    >
    > Best regards,
    > tiger
    >


    Tiger, do you have a testcase handy to demonstrate this?

    Is the new, large out-of-inode xattr unique so that it does not match
    any existing attribute block, I assume?

    Thanks,

    -Eric
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Hi, Andrew

    This situation only happens we format ext3/4 with inode size more than
    128 and we have put xattr entries both in ibody and block.
    The consequences about this bug is we will lost the xattr block which
    pointed by i_file_acl with all xattr entires in it. We will alloc a new
    xattr block and put that large value entry in it. The old xattr block
    will become orphan block.

    Best regards,
    tiger

    Andrew Morton wrote:
    > On Mon, 12 May 2008 11:24:40 +0800
    > Tiger Yang wrote:
    >
    >
    >> I met a bug when I try to replace a xattr entry in ibody with a big size
    >> value. But in ibody there has no space for the new value. So it should
    >> set new xattr entry in block and remove the old xattr entry in ibody.
    >>
    >> Best regards,
    >> tiger
    >>
    >>
    >> [xattr.patch text/x-patch (1.3KB)]
    >> This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
    >>
    >> Signed-off-by: Tiger Yang
    >>
    >> diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
    >> index a6ea4d6..e1af9bd 100644
    >> --- a/fs/ext3/xattr.c
    >> +++ b/fs/ext3/xattr.c
    >> @@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
    >> i.value = NULL;
    >> error = ext3_xattr_block_set(handle, inode, &i, &bs);
    >> } else if (error == -ENOSPC) {
    >> + if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
    >> + error = ext3_xattr_block_find(inode, &i, &bs);
    >> + if (error)
    >> + goto cleanup;
    >> + }
    >> error = ext3_xattr_block_set(handle, inode, &i, &bs);
    >> if (error)
    >> goto cleanup;
    >>

    >
    > That sounds fairly bad.
    >
    > What are the consequences of this bug, when someone hits it?
    >
    > It appears that we should backport this fix into 2.6.25.x (and perhaps
    > earlier). What do you think?
    >
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Hi, Eric,

    I don't have tesecase about this bug. I did the test manually. I use
    khexedit to confirm the attributes whether in inody or block.
    The problem about this bug is we want to replace an existing attribute
    in ibody with big size value which larger than free space in ibody.
    Because we didn't do block_find(), so the struct bs have not been
    initialized. Then when we try to set attribute in block by block_set(),
    we find bs->base is empty, we need alloc a new block for attributes. The
    old block pointed by i_file_acl will lost with attributes in it.

    Best regards,
    tiger

    Eric Sandeen wrote:
    > Tiger Yang wrote:
    >
    >> Hi,
    >> I met a bug when I try to replace a xattr entry in ibody with a big size
    >> value. But in ibody there has no space for the new value. So it should
    >> set new xattr entry in block and remove the old xattr entry in ibody.
    >>
    >> Best regards,
    >> tiger
    >>
    >>

    >
    > Tiger, do you have a testcase handy to demonstrate this?
    >
    > Is the new, large out-of-inode xattr unique so that it does not match
    > any existing attribute block, I assume?
    >
    > Thanks,
    >
    > -Eric
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Hi Eric,

    On Mon, 2008-05-12 at 19:18 -0500, Eric Sandeen wrote:
    > Tiger Yang wrote:
    > > Hi,
    > > I met a bug when I try to replace a xattr entry in ibody with a big size
    > > value. But in ibody there has no space for the new value. So it should
    > > set new xattr entry in block and remove the old xattr entry in ibody.
    > >
    > > Best regards,
    > > tiger
    > >

    >
    > Tiger, do you have a testcase handy to demonstrate this?


    Attached is a simple script to reproduce the problem.

    >
    > Is the new, large out-of-inode xattr unique so that it does not match
    > any existing attribute block, I assume?


    I don't quite understand what you mean but the problem is that in
    ext3_xattr_set_handle(), the EA being replaced is found in the
    inode-body (by function ext3_xattr_ibody_find) and hence
    ext3_xattr_block_find() is not called initially. So in this test-case
    when we have to delete an EA from the inode and add it into the external
    block, bs turns out to be uninitialized and therefore a new EA block
    gets allocated instead of the existing one being used.

    Thanks,
    Kalpak

    >
    > Thanks,
    >
    > -Eric
    > --
    > 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



  7. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    Tiger Yang wrote:
    > Hi, Eric,
    >
    > I don't have tesecase about this bug. I did the test manually. I use
    > khexedit to confirm the attributes whether in inody or block.
    > The problem about this bug is we want to replace an existing attribute
    > in ibody with big size value which larger than free space in ibody.
    > Because we didn't do block_find(), so the struct bs have not been
    > initialized. Then when we try to set attribute in block by block_set(),
    > we find bs->base is empty, we need alloc a new block for attributes. The
    > old block pointed by i_file_acl will lost with attributes in it.


    Thanks, I'll go for a reproducer. We use xattrs a lot for selinux in
    Red Hat and Fedora, so a little surprised I haven't seen this bug... or
    maybe it explains some bugs I haven't yet figured out ...

    Thanks,
    -Eric
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On May 13, 2008 10:31 +0800, Tiger Yang wrote:
    > This situation only happens we format ext3/4 with inode size more than 128
    > and we have put xattr entries both in ibody and block.
    > The consequences about this bug is we will lost the xattr block which
    > pointed by i_file_acl with all xattr entires in it. We will alloc a new
    > xattr block and put that large value entry in it. The old xattr block will
    > become orphan block.


    Tiger, thanks for finding this bug, and the patch (which fixes the
    problem in our testing).

    Signed-off-by: Andreas Dilger

    > Andrew Morton wrote:
    >> On Mon, 12 May 2008 11:24:40 +0800
    >> Tiger Yang wrote:
    >>
    >>
    >>> I met a bug when I try to replace a xattr entry in ibody with a big size
    >>> value. But in ibody there has no space for the new value. So it should
    >>> set new xattr entry in block and remove the old xattr entry in ibody.
    >>>
    >>> Best regards,
    >>> tiger
    >>>
    >>>
    >>> [xattr.patch text/x-patch (1.3KB)]
    >>> This fix the uninitialized bs when we try to replace a xattr entry in ibody with the new value which require more than free space.
    >>>
    >>> Signed-off-by: Tiger Yang
    >>>
    >>> diff --git a/fs/ext3/xattr.c b/fs/ext3/xattr.c
    >>> index a6ea4d6..e1af9bd 100644
    >>> --- a/fs/ext3/xattr.c
    >>> +++ b/fs/ext3/xattr.c
    >>> @@ -1000,6 +1000,11 @@ ext3_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
    >>> i.value = NULL;
    >>> error = ext3_xattr_block_set(handle, inode, &i, &bs);
    >>> } else if (error == -ENOSPC) {
    >>> + if (EXT3_I(inode)->i_file_acl && !bs.s.base) {
    >>> + error = ext3_xattr_block_find(inode, &i, &bs);
    >>> + if (error)
    >>> + goto cleanup;
    >>> + }
    >>> error = ext3_xattr_block_set(handle, inode, &i, &bs);
    >>> if (error)
    >>> goto cleanup;
    >>>

    >>
    >> That sounds fairly bad.
    >>
    >> What are the consequences of this bug, when someone hits it?


    The EAs in the external block (except the one being added) are lost, and
    some blocks (or shared EA block references) are leaked. In most cases
    this is not fatal, but for Lustre I developed a test case where this
    causes the file data to be lost (because the file layout is stored in
    the external block if it is too large to store in the inode).

    >> It appears that we should backport this fix into 2.6.25.x (and perhaps
    >> earlier). What do you think?


    Code inspection shows this bug goes back to when the fast EA-in-inode
    support was added to the vanilla kernel, at least 2.6.12 (when Git
    history begins).

    Sadly, the bug was NOT in the original CFS EA-in-inode patches that we
    made for kernels 2.6.5 (SLES 9) and 2.6.9 (RHEL 4) (and still use today)
    that were in 2.6.11-rc1-mm1, but were added during the later rewrite of
    this code.

    I suspect the reasons this bug hasn't been reported even when large inodes
    are enabled (which is the default for newer e2fsprogs) are:
    - it uncommon to have multiple EAs on a file (usually SELinux is the
    only common one and it is relatively small)
    - one of the EAs must already be too large to fit in the inode
    - increasing the size of any EA after it is created is rare

    Cheers, Andreas
    --
    Andreas Dilger
    Sr. Staff Engineer, Lustre Group
    Sun Microsystems of Canada, Inc.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:
    > Hi, Andrew
    >
    > This situation only happens we format ext3/4 with inode size more than
    > 128 and we have put xattr entries both in ibody and block.
    > The consequences about this bug is we will lost the xattr block which
    > pointed by i_file_acl with all xattr entires in it. We will alloc a new
    > xattr block and put that large value entry in it. The old xattr block
    > will become orphan block.


    The patch looks good, and it obviously fixes the described problem. Thanks!

    Signed-off-by: Andreas Gruenbacher


    Could it please be added to -stable?

    Andreas
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Wed, May 14, 2008 at 12:56:42PM +0200, Andreas Gruenbacher wrote:
    > On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:
    > > Hi, Andrew
    > >
    > > This situation only happens we format ext3/4 with inode size more than
    > > 128 and we have put xattr entries both in ibody and block.
    > > The consequences about this bug is we will lost the xattr block which
    > > pointed by i_file_acl with all xattr entires in it. We will alloc a new
    > > xattr block and put that large value entry in it. The old xattr block
    > > will become orphan block.

    >
    > The patch looks good, and it obviously fixes the described problem. Thanks!
    >
    > Signed-off-by: Andreas Gruenbacher
    >
    >
    > Could it please be added to -stable?


    Can someone actually _send_ the patch to stable@kernel.org? I haven't
    seen it yet

    And is it in Linus's tree? We need to wait until it is there before we
    can add it to -stable.

    thanks,

    greg k-h
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Wed, 14 May 2008 09:00:36 -0700 Greg KH wrote:

    > On Wed, May 14, 2008 at 12:56:42PM +0200, Andreas Gruenbacher wrote:
    > > On Tuesday 13 May 2008 04:31:04 Tiger Yang wrote:
    > > > Hi, Andrew
    > > >
    > > > This situation only happens we format ext3/4 with inode size more than
    > > > 128 and we have put xattr entries both in ibody and block.
    > > > The consequences about this bug is we will lost the xattr block which
    > > > pointed by i_file_acl with all xattr entires in it. We will alloc a new
    > > > xattr block and put that large value entry in it. The old xattr block
    > > > will become orphan block.

    > >
    > > The patch looks good, and it obviously fixes the described problem. Thanks!
    > >
    > > Signed-off-by: Andreas Gruenbacher
    > >
    > >
    > > Could it please be added to -stable?

    >
    > Can someone actually _send_ the patch to stable@kernel.org? I haven't
    > seen it yet
    >
    > And is it in Linus's tree? We need to wait until it is there before we
    > can add it to -stable.


    It's in -mm and I just added the "Cc:stable" tag to it.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Wed, May 14, 2008 at 10:28:09AM -0700, Andrew Morton wrote:
    >
    > It's in -mm and I just added the "Cc:stable" tag to it.


    This patch has both ext3 and ext4 changes in it, so Andrew, I'm
    assuming that you'll push it directly to Linus in the near future?
    That's why I didn't suck it into the ext4 tree....

    - Ted
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: [stable] [PATCH] ext3/4: fix uninitialized bs in ext3/4_xattr_set_handle()

    On Wed, 14 May 2008 18:30:03 -0400
    Theodore Tso wrote:

    > On Wed, May 14, 2008 at 10:28:09AM -0700, Andrew Morton wrote:
    > >
    > > It's in -mm and I just added the "Cc:stable" tag to it.

    >
    > This patch has both ext3 and ext4 changes in it, so Andrew, I'm
    > assuming that you'll push it directly to Linus in the near future?


    Yes, today.

    > That's why I didn't suck it into the ext4 tree....


    Yup. Usually I have to split these things up but this time I decided
    to leave it as a single patch. Not sure why, really.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread