Hi Hin-Tak,
That now looks good.
I agree with you that doing a major cleanup of how attribute names are handled is a separate patch. As you say that is much more work and it is not as trivial as this. I think doing the attribute handling properly requires reworking how it all fits together because it is imposing the name length checks at the wrong level - they need to be done after conversion to UTF-16 not whilst they are still in the current NLS (except of course you want to check that they are not too big to fit in the buffer though it is not at all clear to me why the argument is being copied into a buffer in the first place at least in one of the places I looked at - seems a waste of CPU time to me).
Also, I don't think it is needed to set up a kmem cache for this - kmalloc is perfect - as at any one time there will be a vanishingly small number of these buffers allocated at any one time. After all, how many concurrent readdir() calls are there going to be on a volume at any one time? It is not like the struct inode or other long lived memory structures where you end up with thousands of them in memory at any one time...
Just one thing I wanted to mention in case you did not know it: kfree(NULL) is allowed - in fact kfree() function starts by doing "if (NULL argument) return;" so in your patch you could instead of adding an additional exit label, just use the generic one that also does a kfree() which is safe as the pointer is NULL. As this is only error handling code path it does not matter that you incur an additional function call overhead.
Whether you want to change the exit label or not, you can change the CC: to Reviewed-By: for me if you like and I would suggest you send it to Andrew Morton for inclusion through his patch series.
Best regards,
Anton
On 8 Apr 2014, at 03:19, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote:
> From: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>
> The HFS Plus Volume Format specification (TN1150) states that
> file names are stored internally as a maximum of 255 unicode
> characters, as defined by The Unicode Standard, Version 2.0
> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
> the NLS system on Linux before presented to the user.
>
> 255 CJK characters converts to UTF-8 with 1 unicode character
> to up to 3 bytes, and to GB18030 with 1 unicode character to
> up to 4 bytes. Thus, trying in a UTF-8 locale to list files
> with names of more than 85 CJK characters results in:
>
> $ ls /mnt
> ls: reading directory /mnt: File name too long
>
> The receiving buffer needs to be 255 x NLS_MAX_CHARSET_SIZE bytes,
> not 255 bytes as the code has always been.
>
> Similar consideration applies to attributes, which are stored
> internally as a maximum of 127 UTF-16BE units in HFS+.
>
> Strictly speaking, the maximum value of NLS_MAX_CHARSET_SIZE = 6
> is not attainable in the case of conversion to UTF-8, as that
> requires the use of surrogate pairs, i.e. consuming two storage units.
>
> Thanks Anton Altaparmakov for reviewing an earlier version of
> this change.
>
> Signed-off-by: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
> CC: Anton Altaparmakov <anton@xxxxxxxxxx>
> CC: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
> CC: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> CC: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> ---
> fs/hfsplus/dir.c | 12 ++++++++++--
> fs/hfsplus/xattr.c | 15 ++++++++++++---
> 2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index bdec665..f95abef 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -12,6 +12,7 @@
> #include <linux/fs.h>
> #include <linux/slab.h>
> #include <linux/random.h>
> +#include <linux/nls.h>
>
> #include "hfsplus_fs.h"
> #include "hfsplus_raw.h"
> @@ -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);
> + if (!strbuf) {
> + err = -ENOMEM;
> + goto failed_strbuf_alloc;
> + }
> hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
> err = hfs_brec_find(&fd, hfs_find_rec_by_key);
> if (err)
> @@ -193,7 +199,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
> hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
> fd.entrylength);
> type = be16_to_cpu(entry.type);
> - len = HFSPLUS_MAX_STRLEN;
> + len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
> err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
> if (err)
> goto out;
> @@ -246,6 +252,8 @@ next:
> }
> memcpy(&rd->key, fd.key, sizeof(struct hfsplus_cat_key));
> out:
> + kfree(strbuf);
> +failed_strbuf_alloc:
> hfs_find_exit(&fd);
> return err;
> }
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 4e27edc..cfbb23d 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -8,6 +8,7 @@
>
> #include "hfsplus_fs.h"
> #include <linux/posix_acl_xattr.h>
> +#include <linux/nls.h>
> #include "xattr.h"
> #include "acl.h"
>
> @@ -645,8 +646,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
> struct hfs_find_data fd;
> u16 key_len = 0;
> struct hfsplus_attr_key attr_key;
> - char strbuf[HFSPLUS_ATTR_MAX_STRLEN +
> - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
> + char *strbuf;
> int xattr_name_len;
>
> if ((!S_ISREG(inode->i_mode) &&
> @@ -666,6 +666,13 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
> return err;
> }
>
> + strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
> + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
> + if (!strbuf) {
> + res = -ENOMEM;
> + goto failed_strbuf_alloc;
> + }
> +
> err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
> if (err) {
> if (err == -ENOENT) {
> @@ -692,7 +699,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
> if (be32_to_cpu(attr_key.cnid) != inode->i_ino)
> goto end_listxattr;
>
> - xattr_name_len = HFSPLUS_ATTR_MAX_STRLEN;
> + xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN;
> if (hfsplus_uni2asc(inode->i_sb,
> (const struct hfsplus_unistr *)&fd.key->attr.key_name,
> strbuf, &xattr_name_len)) {
> @@ -718,6 +725,8 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
> }
>
> end_listxattr:
> + kfree(strbuf);
> +failed_strbuf_alloc:
> hfs_find_exit(&fd);
> return res;
> }
> --
> 1.9.0
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Senior Kernel Engineer, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
--
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
