Re: [PATCH] Btrfs: make xattr replace operations atomic

[Chris Mason <clm@fb.com>]

On Fri, Nov 7, 2014 at 3:39 PM, Filipe Manana <fdmanana@suse.com> wrote:
> Replacing a xattr consists of doing a lookup for its existing value, 
> delete
> the current value from the respective leaf, release the search path 
> and then
> finally insert the new value. This leaves a time window where readers 
> (getxattr,
> listxattrs) won't see any value for the xattr. Xattrs are used to 
> store ACLs,
> so this has security implications.
> 
> This change also fixes 2 other existing issues which were:
> 
> *) Deleting the old xattr value without verifying first if the new 
> xattr will
>    fit in the existing leaf item (in case multiple xattrs are packed 
> in the
>    same item due to name hash collision);
> 
> *) Returning -EEXIST when the flag XATTR_CREATE is given and the 
> xattr doesn't
>    exist but we have have an existing item that packs muliple xattrs 
> with
>    the same name hash as the input xattr. In this case we should 
> return ENOSPC.
> 
> A test case for xfstests follows soon.
> 
> Thanks to Alexandre Oliva for reporting the non-atomicity of the 
> xattr replace
> implementation.

Thanks Filipe, one problem:

> +
> +		if (size > old_data_len) {
> +			if (btrfs_leaf_free_space(root, leaf) <
> +			    (size - old_data_len)) {
> +				ret = -ENOSPC;
> +				goto out;
> +			}
>  		}

This means replacing an xattr item can fail if the leaf doesn't happen 
to have free space.  btrfs_search_slot already has code meant to extend 
the item, even if the item you're searching for is already found.  (see 
the call to split_leaf with extend == 1).

So what you want to do is find out the current size of the item, and 
call btrfs search_slot again with that size + the extra needed for the 
new name.  From there you should be able to call btrfs_extend_item like 
you currently are.

-chris