Re: [PATCH 01/35] Btrfs xattr code

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  7 Jan 2009 22:56:51 -0500 Chris Mason <chris.mason@oracle.com> wrote:

> Xattrs in btrfs are stored in the btree close to the inode item.  They use an
> indexing scheme very similar to directories.
> 
> The max size of a btrfs xattr is limited by the size of a btree item,
> or about 4k.  A future version will support larger items.
> 
>
> ...
>
> +ssize_t __btrfs_getxattr(struct inode *inode, const char *name,
> +				void *buffer, size_t size)
> +{
> +	struct btrfs_dir_item *di;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_path *path;
> +	struct extent_buffer *leaf;
> +	int ret = 0;
> +	unsigned long data_ptr;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	/* lookup the xattr by name */
> +	di = btrfs_lookup_xattr(NULL, root, path, inode->i_ino, name,
> +				strlen(name), 0);
> +	if (!di || IS_ERR(di)) {

This seems quite bad.  An error return from btrfs_lookup_xattr() can
arise from (I assume) OOM, corrupted filesystem, I/O error, etc.

Yet here we treat such errors as being the same as "not found".  This
seems grossly misleading to userspace and possible a security problem?

(I'm kind of guessing here, and might be wrong and wasting everyone's
time, because the semantics of the btrfs_lookup_xattr() and
btrfs_getxattr() return values aren't documented).

> +		ret = -ENODATA;
> +		goto out;
> +	}
> +
> +	leaf = path->nodes[0];
> +	/* if size is 0, that means we want the size of the attr */
> +	if (!size) {
> +		ret = btrfs_dir_data_len(leaf, di);
> +		goto out;
> +	}
> +
> +	/* now get the data out of our dir_item */
> +	if (btrfs_dir_data_len(leaf, di) > size) {

OK, where did we hide the btrfs_dir_data_len() definition?

> +		ret = -ERANGE;
> +		goto out;
> +	}
> +	data_ptr = (unsigned long)((char *)(di + 1) +
> +				   btrfs_dir_name_len(leaf, di));
> +	read_extent_buffer(leaf, buffer, data_ptr,
> +			   btrfs_dir_data_len(leaf, di));

Please document read_extent_buffer().

Please rename read_extent_buffer() to something more appropriate to a
kernel-wide symbol.

<spies map_private_extent_buffer as well>

Please review all kernel-wide identifiers in btrfs for appropriateness.

I'm scratching my head wondering about this `data_ptr' thing.  Is it a
disk offset?  Is it really a pointer to kernel memory?  According to
this code it is indeed a kernel pointer, but it then gets stuffed into
an unsigned long (wtf?) and then passed to the mysterious
read_extent_buffer().

<reviewer throws in the towel on this part of the code>

> +	ret = btrfs_dir_data_len(leaf, di);
> +
> +out:
> +	btrfs_free_path(path);
> +	return ret;
> +}
> +
> +int __btrfs_setxattr(struct inode *inode, const char *name,
> +			    const void *value, size_t size, int flags)
> +{
> +	struct btrfs_dir_item *di;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_path *path;
> +	int ret = 0, mod = 0;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	trans = btrfs_start_transaction(root, 1);
> +	btrfs_set_trans_block_group(trans, inode);
> +
> +	/* first lets see if we already have this xattr */

let's :)

> +	di = btrfs_lookup_xattr(trans, root, path, inode->i_ino, name,
> +				strlen(name), -1);

<wonders what the -1 does>

<goes to the btrfs_lookup_xattr() definition site>

<towel goes flying again>

> +	if (IS_ERR(di)) {
> +		ret = PTR_ERR(di);
> +		goto out;
> +	}
> +
> +	/* ok we already have this xattr, lets remove it */
> +	if (di) {
> +		/* if we want create only exit */
> +		if (flags & XATTR_CREATE) {
> +			ret = -EEXIST;
> +			goto out;
> +		}
> +
> +		ret = btrfs_delete_one_dir_name(trans, root, path, di);
> +		if (ret)
> +			goto out;
> +		btrfs_release_path(root, path);
> +
> +		/* if we don't have a value then we are removing the xattr */
> +		if (!value) {
> +			mod = 1;
> +			goto out;
> +		}
> +	} else {
> +		btrfs_release_path(root, path);
> +
> +		if (flags & XATTR_REPLACE) {
> +			/* we couldn't find the attr to replace */
> +			ret = -ENODATA;
> +			goto out;
> +		}
> +	}
> +
> +	/* ok we have to create a completely new xattr */
> +	ret = btrfs_insert_xattr_item(trans, root, name, strlen(name),
> +				      value, size, inode->i_ino);
> +	if (ret)
> +		goto out;
> +	mod = 1;
> +
> +out:
> +	if (mod) {
> +		inode->i_ctime = CURRENT_TIME;
> +		ret = btrfs_update_inode(trans, root, inode);
> +	}
> +
> +	btrfs_end_transaction(trans, root);
> +	btrfs_free_path(path);
> +	return ret;
> +}
> +
> +ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
> +{
> +	struct btrfs_key key, found_key;
> +	struct inode *inode = dentry->d_inode;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_path *path;
> +	struct btrfs_item *item;
> +	struct extent_buffer *leaf;
> +	struct btrfs_dir_item *di;
> +	int ret = 0, slot, advance;
> +	size_t total_size = 0, size_left = size;
> +	unsigned long name_ptr;
> +	size_t name_len;
> +	u32 nritems;
> +
> +	/*
> +	 * ok we want all objects associated with this id.
> +	 * NOTE: we set key.offset = 0; because we want to start with the
> +	 * first xattr that we find and walk forward
> +	 */
> +	key.objectid = inode->i_ino;
> +	btrfs_set_key_type(&key, BTRFS_XATTR_ITEM_KEY);
> +	key.offset = 0;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +	path->reada = 2;

<gets interested in btrfs_path.reada>

<greps for a while>

It's snowing towels in here!

	path->reada = 2;

gack!

<greps some more>

	int direction = path->reada;

	...

		if (direction < 0) {
			if (nr == 0)
				break;
			nr--;
		} else if (direction > 0) {
			nr++;
			if (nr >= nritems)
				break;
		}

<delicately moves on...>

> +	/* search for our xattrs */
> +	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +	if (ret < 0)
> +		goto err;
> +	ret = 0;

this statement isn't needed.

> +	advance = 0;
> +	while (1) {
> +		leaf = path->nodes[0];
> +		nritems = btrfs_header_nritems(leaf);
> +		slot = path->slots[0];
> +
> +		/* this is where we start walking through the path */
> +		if (advance || slot >= nritems) {
> +			/*
> +			 * if we've reached the last slot in this leaf we need
> +			 * to go to the next leaf and reset everything
> +			 */
> +			if (slot >= nritems-1) {
> +				ret = btrfs_next_leaf(root, path);
> +				if (ret)
> +					break;
> +				leaf = path->nodes[0];
> +				nritems = btrfs_header_nritems(leaf);
> +				slot = path->slots[0];
> +			} else {
> +				/*
> +				 * just walking through the slots on this leaf
> +				 */
> +				slot++;
> +				path->slots[0]++;
> +			}
> +		}
> +		advance = 1;
> +
> +		item = btrfs_item_nr(leaf, slot);
> +		btrfs_item_key_to_cpu(leaf, &found_key, slot);
> +
> +		/* check to make sure this item is what we want */
> +		if (found_key.objectid != key.objectid)
> +			break;
> +		if (btrfs_key_type(&found_key) != BTRFS_XATTR_ITEM_KEY)
> +			break;
> +
> +		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
> +
> +		name_len = btrfs_dir_name_len(leaf, di);
> +		total_size += name_len + 1;
> +
> +		/* we are just looking for how big our buffer needs to be */
> +		if (!size)
> +			continue;
> +
> +		if (!buffer || (name_len + 1) > size_left) {
> +			ret = -ERANGE;
> +			goto err;
> +		}
> +
> +		name_ptr = (unsigned long)(di + 1);
> +		read_extent_buffer(leaf, buffer, name_ptr, name_len);
> +		buffer[name_len] = '\0';
> +
> +		size_left -= name_len + 1;
> +		buffer += name_len + 1;
> +	}
> +	ret = total_size;
> +
> +err:
> +	btrfs_free_path(path);
> +
> +	return ret;
> +}
>
> ...
>