Hi Sougata,
On Mon, 2014-02-24 at 21:28 +0200, Sougata Santra wrote:
> -ENAMETOOLONG returned from hfsplus_asc2uni was not propaged to iops. This
> allowed hfsplus to create files/directories with HFSPLUS_MAX_STRLEN and
> incorrect keys, leaving the FS in an inconsistent state. This patch fixes
> this issue.
>
Good fix. Thank you.
I have some small remarks. Please, see below.
> Signed-off-by: Sougata Santra <sougata@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/hfsplus/catalog.c | 89 ++++++++++++++++++++++++++++++++++++-------------
> fs/hfsplus/dir.c | 11 ++++--
> fs/hfsplus/hfsplus_fs.h | 4 ++-
> fs/hfsplus/super.c | 4 ++-
> 4 files changed, 79 insertions(+), 29 deletions(-)
>
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..389c474 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -38,21 +38,30 @@ int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *k1,
> return hfsplus_strcmp(&k1->cat.name, &k2->cat.name);
> }
>
> -void hfsplus_cat_build_key(struct super_block *sb, hfsplus_btree_key *key,
> - u32 parent, struct qstr *str)
> +/* Generates key for catalog file/folders record. */
> +int hfsplus_cat_build_key(struct super_block *sb,
> + hfsplus_btree_key *key, u32 parent, struct qstr *str)
> {
> - int len;
> + int len, err;
>
> key->cat.parent = cpu_to_be32(parent);
> - if (str) {
> - hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> - str->name, str->len);
> - len = be16_to_cpu(key->cat.name.length);
> - } else {
> - key->cat.name.length = 0;
> - len = 0;
> - }
> + err = hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
> + str->name, str->len);
> + if (unlikely(err < 0))
> + return err;
> +
> + len = be16_to_cpu(key->cat.name.length);
> key->key_len = cpu_to_be16(6 + 2 * len);
I think that maybe it is time to change hardcoded 6 on sensible named
constant. What do you think?
> + return 0;
> +}
> +
> +/* Generates key for catalog thread record. */
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
> + hfsplus_btree_key *key, u32 parent)
> +{
> + key->cat.parent = cpu_to_be32(parent);
> + key->cat.name.length = 0;
> + key->key_len = cpu_to_be16(6);
Ditto.
> }
>
> static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
We have such code for hfsplus_cat_build_key_uni():
58 static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
59 struct hfsplus_unistr *name)
60 {
61 int ustrlen;
62
63 ustrlen = be16_to_cpu(name->length);
I suppose that it makes sense to check name->length here and return
error. We can check possible volume corruption here.
64 key->cat.parent = cpu_to_be32(parent);
65 key->cat.name.length = cpu_to_be16(ustrlen);
66 ustrlen *= 2;
67 memcpy(key->cat.name.unicode, name->unicode, ustrlen);
68 key->key_len = cpu_to_be16(6 + ustrlen);
69 }
What do you think about such suggestion?
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..66671c4 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -443,8 +443,10 @@ int hfsplus_cat_case_cmp_key(const hfsplus_btree_key *,
> const hfsplus_btree_key *);
> int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *,
> const hfsplus_btree_key *);
> -void hfsplus_cat_build_key(struct super_block *sb,
> +int hfsplus_cat_build_key(struct super_block *sb,
> hfsplus_btree_key *, u32, struct qstr *);
> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
The whole style of the fix is good. But such mess looks not very good.
But it is not critical, of course. :)
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
