check name_len before down_read xattr_sem and sb_read in ext2_xattr_get

PATCH In review crosslonelyover 4 participants 8 messages Jul 12, 2010 – Jul 25, 2010

Subsystems (2)

Kernel janitors ext2 file system

lore

Share & tools

lore

crosslonelyover

author

2010-07-12 14:28 UTC #0 lore raw

PATCH 1/1 check name_len before down_read xattr_sem and sb_read in ext2_xattr_get

Hi,
I walked through ext2_xattr_get, and felt that we can
do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:

         down_read(&EXT2_I(inode)->xattr_sem);
 ...
         bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
 ...
         /* find named attribute */
         name_len = strlen(name);

         error = -ERANGE;
         if (name_len > 255)
                 goto cleanup;

       Most of the case, you'll get one valid block, but if the

name len > 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4.
Please check it.

Signed-off-by Wang Sheng-Hui <[email protected]>

---
 fs/ext2/xattr.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

fs/ext2/xattr.c +7 -5

index 7c39157..0b94d61 100644
@@ -161,6 +161,13 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 
 	if (name == NULL)
 		return -EINVAL;
+
+	/* find named attribute */
+	name_len = strlen(name);
+	error = -ERANGE;
+	if (name_len > 255)
+		goto cleanup;
+
 	down_read(&EXT2_I(inode)->xattr_sem);
 	error = -ENODATA;
 	if (!EXT2_I(inode)->i_file_acl)

@@ -181,12 +188,7 @@ bad_block:	ext2_error(inode->i_sb, "ext2_xattr_get",
 		error = -EIO;
 		goto cleanup;
 	}
-	/* find named attribute */
-	name_len = strlen(name);
 
-	error = -ERANGE;
-	if (name_len > 255)
-		goto cleanup;
 	entry = FIRST_ENTRY(bh);
 	while (!IS_LAST_ENTRY(entry)) {
 		struct ext2_xattr_entry *next =

Signature · 1 line

1.7.1.1

Wang Sheng-Hui

author

2010-07-13 14:28 UTC #1 lore raw

Quoted 49 lines · crosslonelyover

Hi,
I walked through ext2_xattr_get, and felt that we can
do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:
          down_read(&EXT2_I(inode)->xattr_sem);
  ...
          bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
  ...
          /* find named attribute */
          name_len = strlen(name);

          error = -ERANGE;
          if (name_len>  255)
                  goto cleanup;

        Most of the case, you'll get one valid block, but if the
name len> 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4.
Please check it.
Signed-off-by Wang Sheng-Hui<[email protected]>
---
  fs/ext2/xattr.c |   12 +++++++-----
  1 files changed, 7 insertions(+), 5 deletions(-)
fs/ext2/xattr.c +7 -0
index 7c39157..0b94d61 100644
@@ -161,6 +161,13 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 
  	if (name == NULL)
  		return -EINVAL;
+
+	/* find named attribute */
+	name_len = strlen(name);
+	error = -ERANGE;
+	if (name_len>  255)
+		goto cleanup;
+
  	down_read(&EXT2_I(inode)->xattr_sem);
  	error = -ENODATA;
  	if (!EXT2_I(inode)->i_file_acl)
@@ -181,12 +188,7 @@ bad_block:	ext2_error(inode->i_sb, "ext2_xattr_get",
  		error = -EIO;
  		goto cleanup;

fs/ext2/xattr.c bad_block: ext2_error(inode->i_sb, "ext2_xattr_get", · line 194

Quoted 10 lines

fs/ext2/xattr.c +0 -5

  	}
-	/* find named attribute */
-	name_len = strlen(name);
 
-	error = -ERANGE;
-	if (name_len>  255)
-		goto cleanup;
  	entry = FIRST_ENTRY(bh);
  	while (!IS_LAST_ENTRY(entry)) {
  		struct ext2_xattr_entry *next =

Hi, I noticed in ext2_xattr_set, name_len check is done before
down_write(&EXT2_I(inode)->xattr_sem);
So I think ext2_xattr_get should do in the same way.

Please check this patch.

Signature · 2 lines

Thanks and Regards,
shenghui

Dan Carpenter

Reviewed-by on v1 1/1

2010-07-13 14:56 UTC #2 lore raw

In reply to crosslonelyover

Quoted 23 lines · crosslonelyover

Hi,
I walked through ext2_xattr_get, and felt that we can
do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:
         down_read(&EXT2_I(inode)->xattr_sem);
 ...
         bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
 ...
         /* find named attribute */
         name_len = strlen(name);

         error = -ERANGE;
         if (name_len > 255)
                 goto cleanup;

       Most of the case, you'll get one valid block, but if the
name len > 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4.
Please check it.

Quoted 4 lines

Signed-off-by Wang Sheng-Hui <[email protected]>
---
 fs/ext2/xattr.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

For what it's worth, this change looks OK to me.

Reviewed-by Dan Carpenter <[email protected]>

regards,
dan carpenter

Jan Kara

2010-07-21 17:44 UTC #3 lore raw

In reply to crosslonelyover

Quoted 33 lines

Hi,
I walked through ext2_xattr_get, and felt that we can
do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:
         down_read(&EXT2_I(inode)->xattr_sem);
 ...
         bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
 ...
         /* find named attribute */
         name_len = strlen(name);

         error = -ERANGE;
         if (name_len > 255)
                 goto cleanup;

       Most of the case, you'll get one valid block, but if the
name len > 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4.
Please check it.
Signed-off-by Wang Sheng-Hui <[email protected]>
---
 fs/ext2/xattr.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
fs/ext2/xattr.c +0 -0
index 7c39157..0b94d61 100644

fs/ext2/xattr.c ext2_xattr_get(struct inode *inode, int name_index, const char *name, · line 169

Quoted 9 lines

fs/ext2/xattr.c +6 -0

@@ -161,6 +161,13 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 
 	if (name = NULL)
 		return -EINVAL;
+
+	/* find named attribute */
+	name_len = strlen(name);
+	error = -ERANGE;
+	if (name_len > 255)
+		goto cleanup;

But you cannot go to cleanup here because you don't hold xattr_sem...

Honza

Quoted 4 lines

fs/ext2/xattr.c +1 -0

+
 	down_read(&EXT2_I(inode)->xattr_sem);
 	error = -ENODATA;
 	if (!EXT2_I(inode)->i_file_acl)

Signature · 2 lines

Jan Kara <[email protected]>
SuSE CR Labs

shenghui

author

2010-07-22 00:03 UTC #4 lore raw

2010/7/22 Jan Kara <[email protected]>:

Quoted 34 lines

Quoted 34 lines
Hi,
I walked through ext2_xattr_get, and felt that we can do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:
down_read(&EXT2_I(inode)->xattr_sem);
...
bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
...
/* find named attribute */
name_len = strlen(name);
error = -ERANGE;
if (name_len > 255)
goto cleanup;
Most of the case, you'll get one valid block, but if the name len > 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4. Please check it.
Signed-off-by Wang Sheng-Hui <[email protected]>
---
fs/ext2/xattr.c | 12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
fs/ext2/xattr.c +0 -0
index 7c39157..0b94d61 100644
@@ -161,6 +161,13 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 

Quoted 10 lines

fs/ext2/xattr.c ext2_xattr_get(struct inode *inode, int name_index, const char *name, · line 169
Quoted 8 lines
fs/ext2/xattr.c +6 -0
       if (name = NULL)
               return -EINVAL;
+
+     /* find named attribute */
+     name_len = strlen(name);
+     error = -ERANGE;
+     if (name_len > 255)
+             goto cleanup;
But you cannot go to cleanup here because you don't hold xattr_sem...

Sorry, I'm a little confused by your words.
The patch just checks name_len, and it
doesn't need xattr_sem.

Signature · 2 lines

Thanks and Best Regards,
shenghui

Jan Kara

2010-07-23 08:37 UTC #5 lore raw

Quoted 46 lines · shenghui

2010/7/22 Jan Kara <[email protected]>:
Quoted 34 lines
Quoted 34 lines
Hi,
I walked through ext2_xattr_get, and felt that we can do some optimization on it. For name_len check, it's done after down xattr_sem and sb_read, both of which are time consuming operation compared with strlen:
down_read(&EXT2_I(inode)->xattr_sem);
...
bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
...
/* find named attribute */
name_len = strlen(name);
error = -ERANGE;
if (name_len > 255)
goto cleanup;
Most of the case, you'll get one valid block, but if the name len > 255, then the xattr_sem down and sb_bread operation can be seen as a waste of time. So I think we'd better do name len check as early as possible.
Following is my patch, and it's against 2.6.35-rc4. Please check it.
Signed-off-by Wang Sheng-Hui <[email protected]>
---
fs/ext2/xattr.c | 12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
fs/ext2/xattr.c +0 -0
index 7c39157..0b94d61 100644
@@ -161,6 +161,13 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 
Quoted 10 lines
fs/ext2/xattr.c ext2_xattr_get(struct inode *inode, int name_index, const char *name, · line 169
Quoted 8 lines
fs/ext2/xattr.c +6 -0
       if (name = NULL)
               return -EINVAL;
+
+     /* find named attribute */
+     name_len = strlen(name);
+     error = -ERANGE;
+     if (name_len > 255)
+             goto cleanup;
But you cannot go to cleanup here because you don't hold xattr_sem...

Quoted 3 lines

Sorry, I'm a little confused by your words.
The patch just checks name_len, and it
doesn't need xattr_sem.

Checking of name_len is fine as you did it. But I wanted to point out
that if name_len is greater than 255, you then go to 'cleanup' label which tries to do up_read(&EXT2_I(inode)->xattr_sem). But that's a bug because after you moved the code, we don't hold xattr_sem at the moment we check name_len.

Honza

Signature · 6 lines

Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ted Ts'o

2010-07-23 12:46 UTC #6 lore raw

Quoted 5 lines · Jan Kara

Checking of name_len is fine as you did it. But I wanted to point out
that if name_len is greater than 255, you then go to 'cleanup' label which tries to do up_read(&EXT2_I(inode)->xattr_sem). But that's a bug because after you moved the code, we don't hold xattr_sem at the moment we check name_len.

Yup, you could just return -ERANGE right there.

The simpler fix though might be to just delete the check altogether. Neither ext3 nor ext4 checks for the length of the xattr name in their _xattr_get() function. Instead they'll just do the search, and then return -ENODATA. That seems legit; there can be no entries larger than 255, so saying the extended attribute doesn't exist is quite correct. There is a check in the _xattr_set() functions for both ext3 and ext4, which is quite correct and proper.

Does that mean we'll end up doing a search before returning an error --- yes, but I don't think that matters. Why should we care about optimizing an error case? It's not like this is going to be in a timing sensitive part of an application.... (of course the same consideration could apply to your patch as well).

- Ted

shenghui

author

2010-07-25 13:12 UTC #7 lore raw

Thanks for your instructions and explanation!

Signature · 2 lines

Thanks and Best Regards,
shenghui