[PATCH 0/7] fs/adfs fixes

[PATCH 0/7] fs/adfs fixes

[Russell King - ARM Linux admin]

Al Viro pointed out that adfs's filename truncation was not correct.

This raised a question about whether to keep fs/adfs support in the
kernel or to remove it - and asking in RISC OS forums for feedback
has shown that there is a desire to keep it.  Even though it has a
number of problems identifyable from code reviews, people do not
appear to notice these issues in practice.

This patch series addresses the filename truncation issue by removing
support for that.  Some opportunities for cleanup and fixing an issue
with filename translation which would result in names such as "." and
".." being generated were spotted, and are included in this patch
series.

There are further patches to come.

 fs/adfs/adfs.h      |  14 +-----
 fs/adfs/dir.c       | 137 ++++++++++++++++++++++++++++------------------------
 fs/adfs/dir_f.c     |  43 +++++------------
 fs/adfs/dir_fplus.c |  24 +--------
 4 files changed, 89 insertions(+), 129 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 1/7] fs/adfs: factor out filename comparison

[Russell King]

We have essentially the same code in adfs_compare() as adfs_match(), so
arrange to use a common implementation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/dir.c | 68 +++++++++++++++++++++++------------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index e18eff854e1a..bebe2ab86aae 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -100,37 +100,39 @@ adfs_dir_update(struct super_block *sb, struct object_info *obj, int wait)
 	return ret;
 }
 
-static int
-adfs_match(const struct qstr *name, struct object_info *obj)
+static int __adfs_compare(const unsigned char *qstr, u32 qlen,
+			  const char *str, u32 len)
 {
-	int i;
+	u32 i;
 
-	if (name->len != obj->name_len)
-		return 0;
+	if (qlen != len)
+		return 1;
 
-	for (i = 0; i < name->len; i++) {
-		char c1, c2;
+	for (i = 0; i < qlen; i++) {
+		unsigned char qc, c;
 
-		c1 = name->name[i];
-		c2 = obj->name[i];
+		qc = qstr[i];
+		c = str[i];
 
-		if (c1 >= 'A' && c1 <= 'Z')
-			c1 += 'a' - 'A';
-		if (c2 >= 'A' && c2 <= 'Z')
-			c2 += 'a' - 'A';
+		if (qc >= 'A' && qc <= 'Z')
+			qc += 'a' - 'A';
+		if (c >= 'A' && c <= 'Z')
+			c += 'a' - 'A';
 
-		if (c1 != c2)
-			return 0;
+		if (qc != c)
+			return 1;
 	}
-	return 1;
+	return 0;
 }
 
-static int
-adfs_dir_lookup_byname(struct inode *inode, const struct qstr *name, struct object_info *obj)
+static int adfs_dir_lookup_byname(struct inode *inode, const struct qstr *qstr,
+				  struct object_info *obj)
 {
 	struct super_block *sb = inode->i_sb;
 	const struct adfs_dir_ops *ops = ADFS_SB(sb)->s_dir;
+	const unsigned char *name;
 	struct adfs_dir dir;
+	u32 name_len;
 	int ret;
 
 	ret = ops->read(sb, inode->i_ino, inode->i_size, &dir);
@@ -153,8 +155,10 @@ adfs_dir_lookup_byname(struct inode *inode, const struct qstr *name, struct obje
 		goto unlock_out;
 
 	ret = -ENOENT;
+	name = qstr->name;
+	name_len = qstr->len;
 	while (ops->getnext(&dir, obj) == 0) {
-		if (adfs_match(name, obj)) {
+		if (!__adfs_compare(name, name_len, obj->name, obj->name_len)) {
 			ret = 0;
 			break;
 		}
@@ -212,30 +216,10 @@ adfs_hash(const struct dentry *parent, struct qstr *qstr)
  * Compare two names, taking note of the name length
  * requirements of the underlying filesystem.
  */
-static int
-adfs_compare(const struct dentry *dentry,
-		unsigned int len, const char *str, const struct qstr *name)
+static int adfs_compare(const struct dentry *dentry, unsigned int len,
+			const char *str, const struct qstr *qstr)
 {
-	int i;
-
-	if (len != name->len)
-		return 1;
-
-	for (i = 0; i < name->len; i++) {
-		char a, b;
-
-		a = str[i];
-		b = name->name[i];
-
-		if (a >= 'A' && a <= 'Z')
-			a += 'a' - 'A';
-		if (b >= 'A' && b <= 'Z')
-			b += 'a' - 'A';
-
-		if (a != b)
-			return 1;
-	}
-	return 0;
+	return __adfs_compare(qstr->name, qstr->len, str, len);
 }
 
 const struct dentry_operations adfs_dentry_operations = {
-- 
2.7.4

[PATCH 2/7] fs/adfs: factor out filename case lowering

[Russell King]

Factor out the filename case lowering of directory names when comparing
or hashing filenames.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/dir.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index bebe2ab86aae..be4b4f950500 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -100,6 +100,13 @@ adfs_dir_update(struct super_block *sb, struct object_info *obj, int wait)
 	return ret;
 }
 
+static unsigned char adfs_tolower(unsigned char c)
+{
+	if (c >= 'A' && c <= 'Z')
+		c += 'a' - 'A';
+	return c;
+}
+
 static int __adfs_compare(const unsigned char *qstr, u32 qlen,
 			  const char *str, u32 len)
 {
@@ -108,20 +115,10 @@ static int __adfs_compare(const unsigned char *qstr, u32 qlen,
 	if (qlen != len)
 		return 1;
 
-	for (i = 0; i < qlen; i++) {
-		unsigned char qc, c;
-
-		qc = qstr[i];
-		c = str[i];
-
-		if (qc >= 'A' && qc <= 'Z')
-			qc += 'a' - 'A';
-		if (c >= 'A' && c <= 'Z')
-			c += 'a' - 'A';
-
-		if (qc != c)
+	for (i = 0; i < qlen; i++)
+		if (adfs_tolower(qstr[i]) != adfs_tolower(str[i]))
 			return 1;
-	}
+
 	return 0;
 }
 
@@ -198,15 +195,8 @@ adfs_hash(const struct dentry *parent, struct qstr *qstr)
 	qstr->len = i = name_len;
 	name = qstr->name;
 	hash = init_name_hash(parent);
-	while (i--) {
-		char c;
-
-		c = *name++;
-		if (c >= 'A' && c <= 'Z')
-			c += 'a' - 'A';
-
-		hash = partial_name_hash(c, hash);
-	}
+	while (i--)
+		hash = partial_name_hash(adfs_tolower(*name++), hash);
 	qstr->hash = end_name_hash(hash);
 
 	return 0;
-- 
2.7.4

[PATCH 3/7] fs/adfs: factor out object fixups

[Russell King]

Factor out the directory object fixups, which parse the filetype and
optionally apply the filetype suffix to the filename.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/adfs.h      |  1 +
 fs/adfs/dir.c       | 21 +++++++++++++++++++++
 fs/adfs/dir_f.c     | 17 +----------------
 fs/adfs/dir_fplus.c | 18 +-----------------
 4 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/fs/adfs/adfs.h b/fs/adfs/adfs.h
index c76db75f02aa..1097bee65fa9 100644
--- a/fs/adfs/adfs.h
+++ b/fs/adfs/adfs.h
@@ -172,6 +172,7 @@ extern const struct dentry_operations adfs_dentry_operations;
 extern const struct adfs_dir_ops adfs_f_dir_ops;
 extern const struct adfs_dir_ops adfs_fplus_dir_ops;
 
+void adfs_object_fixup(struct adfs_dir *dir, struct object_info *obj);
 extern int adfs_dir_update(struct super_block *sb, struct object_info *obj,
 			   int wait);
 
diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index be4b4f950500..03490f16300d 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -16,6 +16,27 @@
  */
 static DEFINE_RWLOCK(adfs_dir_lock);
 
+void adfs_object_fixup(struct adfs_dir *dir, struct object_info *obj)
+{
+	obj->filetype = -1;
+
+	/*
+	 * object is a file and is filetyped and timestamped?
+	 * RISC OS 12-bit filetype is stored in load_address[19:8]
+	 */
+	if ((0 == (obj->attr & ADFS_NDA_DIRECTORY)) &&
+	    (0xfff00000 == (0xfff00000 & obj->loadaddr))) {
+		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
+
+		/* optionally append the ,xyz hex filetype suffix */
+		if (ADFS_SB(dir->sb)->s_ftsuffix)
+			obj->name_len +=
+				append_filetype_suffix(
+					&obj->name[obj->name_len],
+					obj->filetype);
+	}
+}
+
 static int
 adfs_readdir(struct file *file, struct dir_context *ctx)
 {
diff --git a/fs/adfs/dir_f.c b/fs/adfs/dir_f.c
index 0fbfd0b04ae0..1bab896918ed 100644
--- a/fs/adfs/dir_f.c
+++ b/fs/adfs/dir_f.c
@@ -216,23 +216,8 @@ adfs_dir2obj(struct adfs_dir *dir, struct object_info *obj,
 	obj->execaddr = adfs_readval(de->direxec, 4);
 	obj->size     = adfs_readval(de->dirlen,  4);
 	obj->attr     = de->newdiratts;
-	obj->filetype = -1;
 
-	/*
-	 * object is a file and is filetyped and timestamped?
-	 * RISC OS 12-bit filetype is stored in load_address[19:8]
-	 */
-	if ((0 == (obj->attr & ADFS_NDA_DIRECTORY)) &&
-		(0xfff00000 == (0xfff00000 & obj->loadaddr))) {
-		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
-
-		/* optionally append the ,xyz hex filetype suffix */
-		if (ADFS_SB(dir->sb)->s_ftsuffix)
-			obj->name_len +=
-				append_filetype_suffix(
-					&obj->name[obj->name_len],
-					obj->filetype);
-	}
+	adfs_object_fixup(dir, obj);
 }
 
 /*
diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index c92cfb638c18..308009d00a5b 100644
--- a/fs/adfs/dir_fplus.c
+++ b/fs/adfs/dir_fplus.c
@@ -197,23 +197,7 @@ adfs_fplus_getnext(struct adfs_dir *dir, struct object_info *obj)
 		if (obj->name[i] == '/')
 			obj->name[i] = '.';
 
-	obj->filetype = -1;
-
-	/*
-	 * object is a file and is filetyped and timestamped?
-	 * RISC OS 12-bit filetype is stored in load_address[19:8]
-	 */
-	if ((0 == (obj->attr & ADFS_NDA_DIRECTORY)) &&
-		(0xfff00000 == (0xfff00000 & obj->loadaddr))) {
-		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
-
-		/* optionally append the ,xyz hex filetype suffix */
-		if (ADFS_SB(dir->sb)->s_ftsuffix)
-			obj->name_len +=
-				append_filetype_suffix(
-					&obj->name[obj->name_len],
-					obj->filetype);
-	}
+	adfs_object_fixup(dir, obj);
 
 	dir->pos += 1;
 	ret = 0;
-- 
2.7.4

[PATCH 4/7] fs/adfs: factor out filename fixup

[Russell King]

Move the filename fixup to adfs_object_fixup() so we only have one
implementation of this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/dir.c       | 13 +++++++++++++
 fs/adfs/dir_f.c     | 26 ++++++++++----------------
 fs/adfs/dir_fplus.c |  6 +-----
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 03490f16300d..877d5cffe9e9 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -18,6 +18,19 @@ static DEFINE_RWLOCK(adfs_dir_lock);
 
 void adfs_object_fixup(struct adfs_dir *dir, struct object_info *obj)
 {
+	unsigned int i;
+
+	/*
+	 * RISC OS allows the use of '/' in directory entry names, so we need
+	 * to fix these up.  '/' is typically used for FAT compatibility to
+	 * represent '.', so do the same conversion here.  In any case, '.'
+	 * will never be in a RISC OS name since it is used as the pathname
+	 * separator.
+	 */
+	for (i = 0; i < obj->name_len; i++)
+		if (obj->name[i] == '/')
+			obj->name[i] = '.';
+
 	obj->filetype = -1;
 
 	/*
diff --git a/fs/adfs/dir_f.c b/fs/adfs/dir_f.c
index 1bab896918ed..033884541a63 100644
--- a/fs/adfs/dir_f.c
+++ b/fs/adfs/dir_f.c
@@ -41,21 +41,6 @@ static inline void adfs_writeval(unsigned char *p, int len, unsigned int val)
 	}
 }
 
-static inline int adfs_readname(char *buf, char *ptr, int maxlen)
-{
-	char *old_buf = buf;
-
-	while ((unsigned char)*ptr >= ' ' && maxlen--) {
-		if (*ptr == '/')
-			*buf++ = '.';
-		else
-			*buf++ = *ptr;
-		ptr++;
-	}
-
-	return buf - old_buf;
-}
-
 #define ror13(v) ((v >> 13) | (v << 19))
 
 #define dir_u8(idx)				\
@@ -210,7 +195,16 @@ static inline void
 adfs_dir2obj(struct adfs_dir *dir, struct object_info *obj,
 	struct adfs_direntry *de)
 {
-	obj->name_len =	adfs_readname(obj->name, de->dirobname, ADFS_F_NAME_LEN);
+	unsigned int name_len;
+
+	for (name_len = 0; name_len < ADFS_F_NAME_LEN; name_len++) {
+		if (de->dirobname[name_len] < ' ')
+			break;
+
+		obj->name[name_len] = de->dirobname[name_len];
+	}
+
+	obj->name_len =	name_len;
 	obj->file_id  = adfs_readval(de->dirinddiscadd, 3);
 	obj->loadaddr = adfs_readval(de->dirload, 4);
 	obj->execaddr = adfs_readval(de->direxec, 4);
diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 308009d00a5b..97b9f28f459b 100644
--- a/fs/adfs/dir_fplus.c
+++ b/fs/adfs/dir_fplus.c
@@ -169,7 +169,7 @@ adfs_fplus_getnext(struct adfs_dir *dir, struct object_info *obj)
 		(struct adfs_bigdirheader *) dir->bh_fplus[0]->b_data;
 	struct adfs_bigdirentry bde;
 	unsigned int offset;
-	int i, ret = -ENOENT;
+	int ret = -ENOENT;
 
 	if (dir->pos >= le32_to_cpu(h->bigdirentries))
 		goto out;
@@ -193,10 +193,6 @@ adfs_fplus_getnext(struct adfs_dir *dir, struct object_info *obj)
 	offset += le32_to_cpu(bde.bigdirobnameptr);
 
 	dir_memcpy(dir, offset, obj->name, obj->name_len);
-	for (i = 0; i < obj->name_len; i++)
-		if (obj->name[i] == '/')
-			obj->name[i] = '.';
-
 	adfs_object_fixup(dir, obj);
 
 	dir->pos += 1;
-- 
2.7.4

[PATCH 5/7] fs/adfs: remove truncated filename hashing

[Russell King]

fs/adfs support for truncated filenames is broken, and there is a desire
not to support this into the future.  Let's remove the fs/adfs support
for this.

Viro says:

"FWIW, the word from Linus had been basically "kill it off" on
truncation."

That being:

"Make it so. Make the rule be that d_hash() can only change the hash
itself, rather than the subtle special case for len that we had
because of legacy reasons.."

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/dir.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 877d5cffe9e9..5d88108339df 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -214,22 +214,17 @@ const struct file_operations adfs_dir_operations = {
 static int
 adfs_hash(const struct dentry *parent, struct qstr *qstr)
 {
-	const unsigned int name_len = ADFS_SB(parent->d_sb)->s_namelen;
 	const unsigned char *name;
 	unsigned long hash;
-	int i;
+	u32 len;
 
-	if (qstr->len < name_len)
-		return 0;
+	if (qstr->len > ADFS_SB(parent->d_sb)->s_namelen)
+		return -ENAMETOOLONG;
 
-	/*
-	 * Truncate the name in place, avoids
-	 * having to define a compare function.
-	 */
-	qstr->len = i = name_len;
+	len = qstr->len;
 	name = qstr->name;
 	hash = init_name_hash(parent);
-	while (i--)
+	while (len--)
 		hash = partial_name_hash(adfs_tolower(*name++), hash);
 	qstr->hash = end_name_hash(hash);
 
-- 
2.7.4

[PATCH 6/7] fs/adfs: move append_filetype_suffix() into adfs_object_fixup()

[Russell King]

append_filetype_suffix() is now only used in adfs_object_fixup(), so
move it there.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/adfs.h | 13 -------------
 fs/adfs/dir.c  | 13 ++++++++-----
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/fs/adfs/adfs.h b/fs/adfs/adfs.h
index 1097bee65fa9..804c6a77c5db 100644
--- a/fs/adfs/adfs.h
+++ b/fs/adfs/adfs.h
@@ -113,19 +113,6 @@ struct object_info {
 	__u16		filetype;
 };
 
-/* RISC OS 12-bit filetype converts to ,xyz hex filename suffix */
-static inline int append_filetype_suffix(char *buf, __u16 filetype)
-{
-	if (filetype == 0xffff)	/* no explicit 12-bit file type was set */
-		return 0;
-
-	*buf++ = ',';
-	*buf++ = hex_asc_lo(filetype >> 8);
-	*buf++ = hex_asc_lo(filetype >> 4);
-	*buf++ = hex_asc_lo(filetype >> 0);
-	return 4;
-}
-
 struct adfs_dir_ops {
 	int	(*read)(struct super_block *sb, unsigned int id, unsigned int sz, struct adfs_dir *dir);
 	int	(*setpos)(struct adfs_dir *dir, unsigned int fpos);
diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 5d88108339df..51ed80ff10a5 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -42,11 +42,14 @@ void adfs_object_fixup(struct adfs_dir *dir, struct object_info *obj)
 		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
 
 		/* optionally append the ,xyz hex filetype suffix */
-		if (ADFS_SB(dir->sb)->s_ftsuffix)
-			obj->name_len +=
-				append_filetype_suffix(
-					&obj->name[obj->name_len],
-					obj->filetype);
+		if (ADFS_SB(dir->sb)->s_ftsuffix) {
+			__u16 filetype = obj->filetype;
+
+			obj->name[obj->name_len++] = ',';
+			obj->name[obj->name_len++] = hex_asc_lo(filetype >> 8);
+			obj->name[obj->name_len++] = hex_asc_lo(filetype >> 4);
+			obj->name[obj->name_len++] = hex_asc_lo(filetype >> 0);
+		}
 	}
 }
 
-- 
2.7.4

[PATCH 7/7] fs/adfs: fix filename fixup handling for "/" and "//" names

[Russell King]

Avoid translating "/" and "//" directory entry names to the special
"." and ".." names by instead converting the first character to "^".

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 fs/adfs/dir.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 51ed80ff10a5..fe39310c1a0a 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -18,18 +18,25 @@ static DEFINE_RWLOCK(adfs_dir_lock);
 
 void adfs_object_fixup(struct adfs_dir *dir, struct object_info *obj)
 {
-	unsigned int i;
+	unsigned int dots, i;
 
 	/*
 	 * RISC OS allows the use of '/' in directory entry names, so we need
 	 * to fix these up.  '/' is typically used for FAT compatibility to
 	 * represent '.', so do the same conversion here.  In any case, '.'
 	 * will never be in a RISC OS name since it is used as the pathname
-	 * separator.
+	 * separator.  Handle the case where we may generate a '.' or '..'
+	 * name, replacing the first character with '^' (the RISC OS "parent
+	 * directory" character.)
 	 */
-	for (i = 0; i < obj->name_len; i++)
-		if (obj->name[i] == '/')
+	for (i = dots = 0; i < obj->name_len; i++)
+		if (obj->name[i] == '/') {
 			obj->name[i] = '.';
+			dots++;
+		}
+
+	if (obj->name_len <= 2 && dots == obj->name_len)
+		obj->name[0] = '^';
 
 	obj->filetype = -1;
 
-- 
2.7.4