| Message ID | 50D7E815.6050503@asianux.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Dec 24, 2012 at 01:28:53PM +0800, Chen Gang wrote: > > 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 <gang.chen@asianux.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年12月26日 02:48, Theodore Ts'o 写道: > On Mon, Dec 24, 2012 at 01:28:53PM +0800, Chen Gang wrote: >> >> 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 <gang.chen@asianux.com> > > 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. :-) > 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 } > 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. > > - Ted > >
On Wed, Dec 26, 2012 at 11:15:19AM +0800, Chen Gang wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2012年12月26日 12:45, Theodore Ts'o 写道: > On Wed, Dec 26, 2012 at 11:15:19AM +0800, Chen Gang wrote: >> >> 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 learned. :-) also better to give a comment above the function ext3_acl_size. thanks.
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.
于 2012年12月26日 13:08, Chen Gang 写道:
>> 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
> learned.
>
> :-)
>
> also better to give a comment above the function ext3_acl_size.
>
> thanks.
diff --git a/fs/ext3/acl.c b/fs/ext3/acl.c index dbb5ad5..5121c5b 100644 --- a/fs/ext3/acl.c +++ b/fs/ext3/acl.c @@ -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);
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 <gang.chen@asianux.com> --- fs/ext3/acl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)