Hi Andrew, (and others)
I think I want to pull back this, and submit a V4 later.
After looking at the code carefully, and continue on fixing the rest
of the issues with setting/getting non-English attributes, I think there is
no need to do kzalloc(), and kmalloc is sufficient - the buffer is
always used with its length.
The original stack allocation, doing null termination on init (and other similiar uses
elsewhere in the extended attribute code) is too conservative.
Many of the uses overwrites it with a static string, so the null termination on
init doesn't do anything useful.
It looks a bit obvious that's the case when switching to dynamic allocation - and
allocate it as late as possible, just before use.
Hin-Tak
------------------------------
On Thu, Apr 10, 2014 2:39 PM BST Hin-Tak Leung wrote:
>------------------------------
>On Thu, Apr 10, 2014 8:30 AM BST Vyacheslav Dubeyko wrote:
>
>>On Wed, 2014-04-09 at 22:53 +0100, Hin-Tak Leung wrote:
>>
>>[snip]
>> @@ -127,7 +128,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>> struct inode *inode = file_inode(file);
>> struct super_block *sb = inode->i_sb;
>> int len, err;
>> - char strbuf[HFSPLUS_MAX_STRLEN + 1];
>> + char *strbuf;
>> hfsplus_cat_entry entry;
>> struct hfs_find_data fd;
>> struct hfsplus_readdir_data *rd;
>> @@ -139,6 +140,11 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>> err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
>> if (err)
>> return err;
>> + strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
>>
>>I think that if you use kzalloc for second case then it makes sense to
>>use kzalloc and here. Anyway, kzalloc can be much more safe way, from my
>>viewpoint.
>>
>
>The difference is simply to avoid unforseeable regression - the stack buffer for names
>wasn't null terminated before the switch to dynamic allocation, therefore
>it is switch to plain kmalloc; but the xattr one is null-terminated at initialization, so
>I used kzalloc when switching, just to be on the safe side. It is really just about
>"changing like to like", avoiding unforseeable regression.
>
>I can also see in the latter case, it is being used subsequently in strcpy,
>so null-terminate is probably important. Whereas the name is being passed
>around with its length.
>
>I think Anton has a point there - it might be more efficient to do kmalloc, and if
>necessary, a single assignment to the first byte only, rather than calling kzalloc
>(which we are doing, BTW, 127 x 6 ~ over 750 bytes). I am hoping kzalloc
>is cleverer and does say, blocks of 4 bytes or even 64 at one go, which won't suck too
>much, compared to just one byte assignment. I wish there is an intermediate
>between kmalloc and kzalloc where it zero's only the first byte.
>
>I hope this answer the question. Perhaps kzalloc is not needed, but
>since the code started with "strbuf[size] = {0};", it is safer to switch to kzalloc
>without making the change too ugly.
>
>I mean "safer" in the sense of "without causing
>unforseeable regressions", *not* safer in the general philosophical sense of
>"zero'ed buffers always better than non-zero'ed".
>
>Hin-Tak
>
>>[snip]
>>
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 4e27edc..3034ce6 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -8,6 +8,7 @@
>>
>>
>>
>> + strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
>> + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
>> + if (!strbuf) {
>> + res = -ENOMEM;
>> + goto out;
>> + }
>> +
>>
>>Thanks,
>>Vyacheslav Dubeyko.
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
