fs/ext3: use kzalloc instead of kmalloc

PATCH In review Chen Gang 2 participants 6 messages Dec 24, 2012 – Dec 26, 2012

lore

Share & tools

lore

Chen Gang

author

2012-12-24 05:27 UTC #0 lore raw

PATCH 1/1 fs/ext3: use kzalloc instead of kmalloc

better to use kzalloc instead of kmalloc.

    if acl_e->e_tag is neither ACL_USER, nor ACL_GROUP.
    entry->e_id will not be initialized

we can not say it is a bug, but suggest to initialize it, too.

Signed-off-by Chen Gang <[email protected]>

---
 fs/ext3/acl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/ext3/acl.c +1 -1

index dbb5ad5..5121c5b 100644
@@ -91,7 +91,7 @@ ext3_acl_to_disk(const struct posix_acl *acl, size_t *size)
 	size_t n;
 
 	*size = ext3_acl_size(acl->a_count);
-	ext_acl = kmalloc(sizeof(ext3_acl_header) + acl->a_count *
+	ext_acl = kzalloc(sizeof(ext3_acl_header) + acl->a_count *
 			sizeof(ext3_acl_entry), GFP_NOFS);
 	if (!ext_acl)
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

1.7.10.4

Theodore Ts'o

2012-12-25 18:48 UTC #1 lore raw

Quoted 8 lines · Chen Gang

better to use kzalloc instead of kmalloc.
    if acl_e->e_tag is neither ACL_USER, nor ACL_GROUP.
    entry->e_id will not be initialized
we can not say it is a bug, but suggest to initialize it, too.
Signed-off-by Chen Gang <[email protected]>

This shouldn't be a problem, since if e_tag is not ACL_USER nor ACL_GROUP, the on-disk encoding does not include e_id at all.

That being said, it looks to me there's another bug hiding here. The size of the extended attribute is calculated by ext3_acl_size(), and it looks totally wrong. For one thing, it caluclates the size of the xattr assuming all of the stored encoding ext3_acl_entry_short --- which would not be the case if we had a acl entry of type ACL_USER or ACL_GROUP.

But if that were the case, it would mean that we would not be storing the full acl entry on disk, which would be a pretty horrible and obvious breakage.

I haven't had time to check this yet, but I wanted to flag this so hopefully someone else should double check this..... It would seem to me that the better thing to do would be to calculate the size as part of the for loop in ext3_acl_to_disk(), and drop ext3_acl_size() from acl.h. (This code exists in ext4 as well, so if we have a bug in ext3, we would have a similar bug in ext4.)

- Ted

Chen Gang

author

2012-12-26 03:14 UTC #2 lore raw

Quoted 6 lines · Theodore Ts'o

Quoted 5 lines · Chen Gang
better to use kzalloc instead of kmalloc.
    if acl_e->e_tag is neither ACL_USER, nor ACL_GROUP.
    entry->e_id will not be initialized

Quoted 7 lines

Quoted 3 lines
we can not say it is a bug, but suggest to initialize it, too.
Signed-off-by Chen Gang <[email protected]>
This shouldn't be a problem, since if e_tag is not ACL_USER nor ACL_GROUP, the on-disk encoding does not include e_id at all.

ok, thanks. it is my fault.

:-)

Quoted 11 lines

That being said, it looks to me there's another bug hiding here. The size of the extended attribute is calculated by ext3_acl_size(), and it looks totally wrong. For one thing, it caluclates the size of the xattr assuming all of the stored encoding ext3_acl_entry_short --- which would not be the case if we had a acl entry of type ACL_USER or ACL_GROUP.
But if that were the case, it would mean that we would not be storing the full acl entry on disk, which would be a pretty horrible and obvious breakage.

checking the ext3_acl_size, it does not like what you said above. but we can say, the design for ext3_acl_size is really not quit well.
(maybe can cause issue).

26 static inline size_t ext3_acl_size(int count)
27 {
28 if (count <= 4) {
29 return sizeof(ext3_acl_header) +
30 count * sizeof(ext3_acl_entry_short);
31 } else {
32 return sizeof(ext3_acl_header) +
33 4 * sizeof(ext3_acl_entry_short) +
34 (count - 4) * sizeof(ext3_acl_entry);
35 }
36 }

Quoted 7 lines

I haven't had time to check this yet, but I wanted to flag this so hopefully someone else should double check this..... It would seem to me that the better thing to do would be to calculate the size as part of the for loop in ext3_acl_to_disk(), and drop ext3_acl_size() from acl.h. (This code exists in ext4 as well, so if we have a bug in ext3, we would have a similar bug in ext4.)

at least, for my idea:
your design for ext3_acl_size is a standard one.
it is necessary to use your design instead of original design.

if you also like me to provide the relative patch, please tell me.

thanks.

gchen.

Quoted 4 lines

- Ted

Signature · 7 lines

Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Theodore Ts'o

2012-12-26 04:45 UTC #3 lore raw

Quoted 4 lines · Chen Gang

checking the ext3_acl_size, it does not like what you said above. but we can say, the design for ext3_acl_size is really not quit well.
(maybe can cause issue).

Ah, I see. What's there is OK, but it's not at all obvious that it's OK. A valid acl must have a very specific order of tags, as enforced by posix_acl_valid() in fs/posix_acl.c:

ACL_USER_OBJ ACL_USER*[1] ACL_GROUP_OBJ ACL_GROUP*[1] ACL_MASK[2] ACL_OTHER

[1] Where * is the regexp sense of "0 or more times"
[2] Only if there is at least one ACL_USER or ACL_GROUP tag; otherwise skip ACL_MASK.

Hence, a valid acl can have at most 4 short acl entry types (ACL_USER_OBJ, ACL_GROUP, ACL_MASK, and ACL_OTHER), and if there is less than 4 acl entries, they must all be short acl types.

All I can say is, this is a horrible way of coding things, and I wish this was documented explicitly somewhere either in fs/posix_acl.c or in include/linux/posix_acl.h. Yuck, yuck, yuck....

- Ted

Chen Gang

author

2012-12-26 05:07 UTC #4 lore raw

Quoted 20 lines · Theodore Ts'o

Quoted 4 lines · Chen Gang
checking the ext3_acl_size, it does not like what you said above. but we can say, the design for ext3_acl_size is really not quit well.
(maybe can cause issue).
Ah, I see. What's there is OK, but it's not at all obvious that it's OK. A valid acl must have a very specific order of tags, as enforced by posix_acl_valid() in fs/posix_acl.c:
ACL_USER_OBJ ACL_USER*[1] ACL_GROUP_OBJ ACL_GROUP*[1] ACL_MASK[2] ACL_OTHER
[1] Where * is the regexp sense of "0 or more times"
[2] Only if there is at least one ACL_USER or ACL_GROUP tag; otherwise skip ACL_MASK.
Hence, a valid acl can have at most 4 short acl entry types (ACL_USER_OBJ, ACL_GROUP, ACL_MASK, and ACL_OTHER), and if there is less than 4 acl entries, they must all be short acl types.

Quoted 5 lines

All I can say is, this is a horrible way of coding things, and I wish this was documented explicitly somewhere either in fs/posix_acl.c or in include/linux/posix_acl.h. Yuck, yuck, yuck....
- Ted

learned.

:-)

also better to give a comment above the function ext3_acl_size.

thanks.

Signature · 7 lines

Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chen Gang

author

2012-12-26 05:33 UTC #5 lore raw

I prefer:
define a macro in .h file and give comments with relative information. for function ext3_acl_size, use the macro instead of hard code number "4"
(then not need to comment for the function ext3_acl_size)

gchen.

Quoted 26 lines · Chen Gang

Quoted 19 lines
Ah, I see. What's there is OK, but it's not at all obvious that it's
Quoted 18 lines
OK. A valid acl must have a very specific order of tags, as enforced by posix_acl_valid() in fs/posix_acl.c:
ACL_USER_OBJ ACL_USER*[1] ACL_GROUP_OBJ ACL_GROUP*[1] ACL_MASK[2] ACL_OTHER
[1] Where * is the regexp sense of "0 or more times"
[2] Only if there is at least one ACL_USER or ACL_GROUP tag; otherwise skip ACL_MASK.
Hence, a valid acl can have at most 4 short acl entry types (ACL_USER_OBJ, ACL_GROUP, ACL_MASK, and ACL_OTHER), and if there is less than 4 acl entries, they must all be short acl types.
All I can say is, this is a horrible way of coding things, and I wish this was documented explicitly somewhere either in fs/posix_acl.c or in include/linux/posix_acl.h. Yuck, yuck, yuck....
- Ted
learned.
:-)
also better to give a comment above the function ext3_acl_size.
thanks.

Signature · 7 lines

Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html