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
2008-Oct-21 02:08 UTC
[Ocfs2-devel] [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 <joel.becker at oracle.com>
---
fs/ocfs2/ocfs2.h | 3 +++
fs/ocfs2/xattr.c | 38 ++++++++++++++++----------------------
2 files changed, 19 insertions(+), 22 deletions(-)
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index a21a465..fef7ece 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -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)
{
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 802c414..44d58f1 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -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;
--
1.5.6.5
Joel Becker
2008-Oct-21 02:08 UTC
[Ocfs2-devel] [PATCH 2/5] ocfs2: Don't return -EFAULT from a corrupt xattr entry.
If the xattr disk structures are corrupt, return -EIO, not -EFAULT.
Signed-off-by: Joel Becker <joel.becker at oracle.com>
---
fs/ocfs2/xattr.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 44d58f1..ec262ef 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -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;
--
1.5.6.5
Joel Becker
2008-Oct-21 02:08 UTC
[Ocfs2-devel] [PATCH 3/5] ocfs2: Check errors from ocfs2_xattr_update_xattr_search()
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 <joel.becker at oracle.com>
---
fs/ocfs2/xattr.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index ec262ef..90cd7d0 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -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 -
--
1.5.6.5
Joel Becker
2008-Oct-21 02:08 UTC
[Ocfs2-devel] [PATCH 4/5] ocfs2: Specify appropriate journal access for new xattr buckets.
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 <joel.becker at oracle.com>
---
fs/ocfs2/xattr.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 90cd7d0..5f11db2 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -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;
--
1.5.6.5
Joel Becker
2008-Oct-21 02:08 UTC
[Ocfs2-devel] [PATCH 5/5] ocfs2: Don't repeat ocfs2_xattr_block_find()
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 <joel.becker at oracle.com>
---
fs/ocfs2/xattr.c | 39 +++++++++------------------------------
1 files changed, 9 insertions(+), 30 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 5f11db2..15158bc 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -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;
}
--
1.5.6.5
Hi Joel/Mark, all the 5 patches look good to me. Thanks. Regards, Tao Joel Becker wrote:> 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 > >