On 02/25/2014 09:37 AM, Vyacheslav Dubeyko wrote:
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.
Thank you for taking time to look into it.
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?
I agree, although I think this should he done in a separate patch. Also
there are other instances of hard-coding. We can clean them with a patch. ?
+ 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.
I looked into it while writing the patch. I think this was already
handled before. Please see. catalog.c#hfsplus_find_cat the only place it
is called.
{code}
if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
pr_err("catalog name length corrupted\n");
return -EIO;
}
{code}
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. :)
Thank you for taking time to read it.
Thanks,
Vyacheslav Dubeyko.
Best regards,
Sougata
--
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
