[Top] [All Lists]

Re: [PATCH 2/2] xfs: prevent bogus assert when trying to remove non-exis

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: prevent bogus assert when trying to remove non-existent attribute
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 31 May 2011 17:19:26 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1306815659-23346-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1306815659-23346-1-git-send-email-david@xxxxxxxxxxxxx> <1306815659-23346-3-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Tue, 2011-05-31 at 14:20 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> If the attribute fork on an inode is in btree format and has
> multiple levels (i.e node format rather than leaf format), then a
> lookup failure will trigger an assert failure in xfs_da_path_shift
> if the flag XFS_DA_OP_OKNOENT is not set. This flag is used to
> indicate to the directory btree code that not finding an entry is
> not a fatal error. In the case of doing a lookup for a directory
> name removal, this is valid as a user cannot insert an arbitrary
> name to remove from the directory btree.
> However, in the case of the attribute tree, a user has direct
> control over the attribute name and can ask for any random name to
> be removed without any validation. In this case, fsstress is asking
> for a non-existent user.selinux attribute to be removed, and that is
> causing xfs_da_path_shift() to fall off the bottom of the tree where
> it asserts that a lookup failure is allowed. Because the flag is not
> set, we die a horrible death on a debug enable kernel.
> Prevent this assert from firing on attribute removes by adding the
> op_flag XFS_DA_OP_OKNOENT to atribute removal operations.
> Discovered when testing on a SELinux enabled system by fsstress in
> test 070 by trying to remove a non-existent user.selinux attribute.
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

I have not carefully verified this change, but your
description of the problem was very good so based
on this the change looks right to me.

Signed-off-by: Alex Elder <aelder@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>