ocfs2: xattr fixups

PATCH New Joel Becker 2 participants 7 messages Oct 21, 2008 – Oct 22, 2008 OCFS2

Share & tools

Joel Becker

author

2008-10-21 02:08 UTC #0 lore raw

cover ocfs2: xattr fixups

Tao, Mark,
Here are four fixups for extended attributes that should go
upstream. The fifth patch is a cleanup that I have no other place for right now - it can probably go with, but I'd be fine with a different resolution.
Tao, please review and make sure I didn't screw anything up.
Mark, they can also be pulled from my xattr-fixups branch.

Joel

Joel Becker

author

2008-10-21 02:08 UTC #1 lore raw

PATCH 1/5 ocfs2: Check xattr block signatures properly.

The xattr.c code is currently memcmp()ing naking buffer pointers. Create the OCFS2_IS_VALID_XATTR_BLOCK() macro to match its peers and use that.

In addition, failed signature checks were returning -EFAULT, which is completely wrong. Return -EIO.

Signed-off-by Joel Becker <[email protected]>

---
 fs/ocfs2/ocfs2.h |    3 +++
 fs/ocfs2/xattr.c |   38 ++++++++++++++++----------------------
 2 files changed, 19 insertions(+), 22 deletions(-)

fs/ocfs2/ocfs2.h +3 -0

index a21a465..fef7ece 100644
@@ -473,6 +473,9 @@ static inline int ocfs2_uses_extended_slot_map(struct ocfs2_super *osb)
 		(____gd)->bg_signature);				\
 } while (0)
 
+#define OCFS2_IS_VALID_XATTR_BLOCK(ptr)					\
+	(!strcmp((ptr)->xb_signature, OCFS2_XATTR_BLOCK_SIGNATURE))
+
 static inline unsigned long ino_from_blkno(struct super_block *sb,
 					   u64 blkno)
 {

fs/ocfs2/xattr.c +16 ~~-22~~

index 802c414..44d58f1 100644
@@ -542,14 +542,12 @@ static int ocfs2_xattr_block_list(struct inode *inode,
 		mlog_errno(ret);
 		return ret;
 	}
-	/*Verify the signature of xattr block*/
-	if (memcmp((void *)blk_bh->b_data, OCFS2_XATTR_BLOCK_SIGNATURE,
-		   strlen(OCFS2_XATTR_BLOCK_SIGNATURE))) {
-		ret = -EFAULT;
-		goto cleanup;
-	}
 
 	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
+	if (!OCFS2_IS_VALID_XATTR_BLOCK(xb)) {
+		ret = -EIO;
+		goto cleanup;
+	}
 
 	if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED)) {
 		struct ocfs2_xattr_header *header = &xb->xb_attrs.xb_header;

@@ -766,15 +764,14 @@ static int ocfs2_xattr_block_get(struct inode *inode,
 		mlog_errno(ret);
 		return ret;
 	}
-	/*Verify the signature of xattr block*/
-	if (memcmp((void *)blk_bh->b_data, OCFS2_XATTR_BLOCK_SIGNATURE,
-		   strlen(OCFS2_XATTR_BLOCK_SIGNATURE))) {
-		ret = -EFAULT;
+
+	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
+	if (!OCFS2_IS_VALID_XATTR_BLOCK(xb)) {
+		ret = -EIO;
 		goto cleanup;
 	}
 
 	xs->xattr_bh = blk_bh;
-	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
 
 	if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED)) {
 		xs->header = &xb->xb_attrs.xb_header;

@@ -1514,10 +1511,9 @@ static int ocfs2_xattr_free_block(struct inode *inode,
 		goto out;
 	}
 
-	/*Verify the signature of xattr block*/
-	if (memcmp((void *)blk_bh->b_data, OCFS2_XATTR_BLOCK_SIGNATURE,
-		   strlen(OCFS2_XATTR_BLOCK_SIGNATURE))) {
-		ret = -EFAULT;
+	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
+	if (!OCFS2_IS_VALID_XATTR_BLOCK(xb)) {
+		ret = -EIO;
 		goto out;
 	}

@@ -1527,7 +1523,6 @@ static int ocfs2_xattr_free_block(struct inode *inode,
 		goto out;
 	}
 
-	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
 	blk = le64_to_cpu(xb->xb_blkno);
 	bit = le16_to_cpu(xb->xb_suballoc_bit);
 	bg_blkno = ocfs2_which_suballoc_group(blk, bit);

@@ -1771,15 +1766,14 @@ static int ocfs2_xattr_block_find(struct inode *inode,
 		mlog_errno(ret);
 		return ret;
 	}
-	/*Verify the signature of xattr block*/
-	if (memcmp((void *)blk_bh->b_data, OCFS2_XATTR_BLOCK_SIGNATURE,
-		   strlen(OCFS2_XATTR_BLOCK_SIGNATURE))) {
-			ret = -EFAULT;
-			goto cleanup;
+
+	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
+	if (!OCFS2_IS_VALID_XATTR_BLOCK(xb)) {
+		ret = -EIO;
+		goto cleanup;
 	}
 
 	xs->xattr_bh = blk_bh;
-	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
 
 	if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED)) {
 		xs->header = &xb->xb_attrs.xb_header;

Signature · 1 line

1.5.6.5

Joel Becker

author

2008-10-21 02:08 UTC #2 lore raw

PATCH 2/5 ocfs2: Don't return -EFAULT from a corrupt xattr entry.

In reply to Joel Becker

If the xattr disk structures are corrupt, return -EIO, not -EFAULT.

Signed-off-by Joel Becker <[email protected]>

---
 fs/ocfs2/xattr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/ocfs2/xattr.c +1 -1

index 44d58f1..ec262ef 100644
@@ -1226,7 +1226,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 
 	free = min_offs - ((void *)last - xs->base) - sizeof(__u32);
 	if (free < 0)
-		return -EFAULT;
+		return -EIO;
 
 	if (!xs->not_found) {
 		size_t size = 0;

Signature · 1 line

1.5.6.5

Joel Becker

author

2008-10-21 02:08 UTC #3 lore raw

PATCH 3/5 ocfs2: Check errors from ocfs2_xattr_update_xattr_search()

In reply to Joel Becker

The ocfs2_xattr_update_xattr_search() function can return an error when trying to read blocks off of disk. The caller needs to check this error before using those (possibly invalid) blocks.

Signed-off-by Joel Becker <[email protected]>

---
 fs/ocfs2/xattr.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

fs/ocfs2/xattr.c +5 -1

index ec262ef..90cd7d0 100644
@@ -2812,7 +2812,11 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	if (data_bh)
 		ocfs2_journal_dirty(handle, data_bh);
 
-	ocfs2_xattr_update_xattr_search(inode, xs, xb_bh, xh_bh);
+	ret = ocfs2_xattr_update_xattr_search(inode, xs, xb_bh, xh_bh);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_commit;
+	}
 
 	/* Change from ocfs2_xattr_header to ocfs2_xattr_tree_root */
 	memset(&xb->xb_attrs, 0, inode->i_sb->s_blocksize -

Signature · 1 line

1.5.6.5

Joel Becker

author

2008-10-21 02:08 UTC #4 lore raw

PATCH 4/5 ocfs2: Specify appropriate journal access for new xattr buckets.

In reply to Joel Becker

There are a couple places that get an xattr bucket that may be reading an existing one or may be allocating a new one. They should specify the correct journal access mode depending.

Signed-off-by Joel Becker <[email protected]>

---
 fs/ocfs2/xattr.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

fs/ocfs2/xattr.c +5 -1

index 90cd7d0..5f11db2 100644
@@ -3218,7 +3218,9 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 
 	for (i = 0; i < blk_per_bucket; i++) {
 		ret = ocfs2_journal_access(handle, inode, t_bhs[i],
-					   OCFS2_JOURNAL_ACCESS_CREATE);
+					   new_bucket_head ?
+					   OCFS2_JOURNAL_ACCESS_CREATE :
+					   OCFS2_JOURNAL_ACCESS_WRITE);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;

@@ -3380,6 +3382,8 @@ static int ocfs2_cp_xattr_bucket(struct inode *inode,
 
 	for (i = 0; i < blk_per_bucket; i++) {
 		ret = ocfs2_journal_access(handle, inode, t_bhs[i],
+					   t_is_new ?
+					   OCFS2_JOURNAL_ACCESS_CREATE :
 					   OCFS2_JOURNAL_ACCESS_WRITE);
 		if (ret)
 			goto out;

Signature · 1 line

1.5.6.5

Joel Becker

author

2008-10-21 02:08 UTC #5 lore raw

PATCH 5/5 ocfs2: Don't repeat ocfs2_xattr_block_find()

In reply to Joel Becker

ocfs2_xattr_block_get() looks up the xattr in a startlingly familiar way; it's identical to the function ocfs2_xattr_block_find(). Let's just use the later in the former.

Signed-off-by Joel Becker <[email protected]>

---
 fs/ocfs2/xattr.c |   39 +++++++++------------------------------
 1 files changed, 9 insertions(+), 30 deletions(-)

fs/ocfs2/xattr.c +9 ~~-30~~

index 5f11db2..15158bc 100644
@@ -116,6 +116,10 @@ static int ocfs2_xattr_bucket_get_name_value(struct inode *inode,
 					     int *block_off,
 					     int *new_offset);
 
+static int ocfs2_xattr_block_find(struct inode *inode,
+				  int name_index,
+				  const char *name,
+				  struct ocfs2_xattr_search *xs);
 static int ocfs2_xattr_index_block_find(struct inode *inode,
 					struct buffer_head *root_bh,
 					int name_index,

@@ -747,46 +751,20 @@ static int ocfs2_xattr_block_get(struct inode *inode,
 				 size_t buffer_size,
 				 struct ocfs2_xattr_search *xs)
 {
-	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
-	struct buffer_head *blk_bh = NULL;
 	struct ocfs2_xattr_block *xb;
 	struct ocfs2_xattr_value_root *xv;
 	size_t size;
 	int ret = -ENODATA, name_offset, name_len, block_off, i;
 
-	if (!di->i_xattr_loc)
-		return ret;
-
 	memset(&xs->bucket, 0, sizeof(xs->bucket));
 
-	ret = ocfs2_read_block(inode, le64_to_cpu(di->i_xattr_loc), &blk_bh);
-	if (ret < 0) {
+	ret = ocfs2_xattr_block_find(inode, name_index, name, xs);
+	if (ret) {
 		mlog_errno(ret);
-		return ret;
-	}
-
-	xb = (struct ocfs2_xattr_block *)blk_bh->b_data;
-	if (!OCFS2_IS_VALID_XATTR_BLOCK(xb)) {
-		ret = -EIO;
 		goto cleanup;
 	}
 
-	xs->xattr_bh = blk_bh;
-
-	if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED)) {
-		xs->header = &xb->xb_attrs.xb_header;
-		xs->base = (void *)xs->header;
-		xs->end = (void *)(blk_bh->b_data) + blk_bh->b_size;
-		xs->here = xs->header->xh_entries;
-
-		ret = ocfs2_xattr_find_entry(name_index, name, xs);
-	} else
-		ret = ocfs2_xattr_index_block_find(inode, blk_bh,
-						   name_index,
-						   name, xs);
-
-	if (ret)
-		goto cleanup;
+	xb = (struct ocfs2_xattr_block *)xs->xattr_bh->b_data;
 	size = le64_to_cpu(xs->here->xe_value_size);
 	if (buffer) {
 		ret = -ERANGE;

@@ -825,7 +803,8 @@ cleanup:
 		brelse(xs->bucket.bhs[i]);
 	memset(&xs->bucket, 0, sizeof(xs->bucket));
 
-	brelse(blk_bh);
+	brelse(xs->xattr_bh);
+	xs->xattr_bh = NULL;
 	return ret;
 }

Signature · 1 line

1.5.6.5

Tao Ma

2008-10-22 04:12 UTC #6 lore raw

cover ocfs2: xattr fixups

In reply to Joel Becker

Hi Joel/Mark,
all the 5 patches look good to me. Thanks.

Regards,
Tao

Quoted 11 lines · Joel Becker

Tao, Mark,
Here are four fixups for extended attributes that should go
upstream. The fifth patch is a cleanup that I have no other place for right now - it can probably go with, but I'd be fine with a different resolution.
Tao, please review and make sure I didn't screw anything up.
Mark, they can also be pulled from my xattr-fixups branch.
Joel