[PATCH v2] romfs: use different way to generate fsid for BLOCK or MTD

[PATCH v2] romfs: use different way to generate fsid for BLOCK or MTD

[Coly Li]

'Commit 8a59f5d25265 ("fs/romfs: return f_fsid for statfs(2)")' generates
a 64bit id from sb->s_bdev->bd_dev. This is only correct when romfs is
defined with CONFIG_ROMFS_ON_BLOCK. If romfs is defined with
CONFIG_ROMFS_ON_MTD, sb->s_bdev is NULL, referencing sb->s_bdev->bd_dev
will triger an oops.

If romfs is built on top of a MTD abstracted block device, this MTD
block device has a device ID generated by MTD_BLOCK_MAJOR and mtd index.
This patch uses the same ID to generate fsid for the romfs on top of the
MTD block device. Generally only one romfs can be built on single MTD
block device, this method is enough to identify multiple romfs instances
in a computer.

If romfs is built on top of a common block device, sb->s_bdev->bd_dev is
used to generate the 64bit id, as previous commit did.

Signed-off-by: Coly Li <colyli@suse.de>
Reported-by: Nong Li <nongli1031@gmail.com>
Tested-by: Nong Li <nongli1031@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/romfs/super.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/romfs/super.c b/fs/romfs/super.c
index d0f8a38..a38c07f 100644
--- a/fs/romfs/super.c
+++ b/fs/romfs/super.c
@@ -74,6 +74,7 @@
 #include <linux/highmem.h>
 #include <linux/pagemap.h>
 #include <linux/uaccess.h>
+#include <linux/major.h>
 #include "internal.h"
 
 static struct kmem_cache *romfs_inode_cachep;
@@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode)
 static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct super_block *sb = dentry->d_sb;
-	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
+	u64 id = 0;
 
+#ifdef CONFIG_ROMFS_ON_BLOCK
+	id = huge_encode_dev(sb->s_bdev->bd_dev);
+#endif
+#ifdef  CONFIG_ROMFS_ON_MTD
+	id = huge_encode_dev(sb->s_dev);
+#endif
 	buf->f_type = ROMFS_MAGIC;
 	buf->f_namelen = ROMFS_MAXFN;
 	buf->f_bsize = ROMBSIZE;
@@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_flags |= MS_RDONLY | MS_NOATIME;
 	sb->s_op = &romfs_super_ops;
 
+#ifdef CONFIG_ROMFS_ON_MTD
+	/* Use same dev ID from the underlying mtdblock device */
+	if (sb->s_mtd)
+		sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
+	else
+		sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0);
+#endif
 	/* read the image superblock and check it */
 	rsb = kmalloc(512, GFP_KERNEL);
 	if (!rsb)
-- 
2.6.6

Re: [PATCH v2] romfs: use different way to generate fsid for BLOCK or MTD

[Richard Weinberger]

CC'ing MTD

On Wed, Dec 28, 2016 at 9:53 AM, Coly Li <colyli@suse.de> wrote:
>  static struct kmem_cache *romfs_inode_cachep;
> @@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode)
>  static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  {
>         struct super_block *sb = dentry->d_sb;
> -       u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
> +       u64 id = 0;
>
> +#ifdef CONFIG_ROMFS_ON_BLOCK
> +       id = huge_encode_dev(sb->s_bdev->bd_dev);
> +#endif
> +#ifdef  CONFIG_ROMFS_ON_MTD
> +       id = huge_encode_dev(sb->s_dev);
> +#endif

How is this supposed to work with CONFIG_ROMFS_BACKED_BY_BOTH=y?

>         buf->f_type = ROMFS_MAGIC;
>         buf->f_namelen = ROMFS_MAXFN;
>         buf->f_bsize = ROMBSIZE;
> @@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
>         sb->s_flags |= MS_RDONLY | MS_NOATIME;
>         sb->s_op = &romfs_super_ops;
>
> +#ifdef CONFIG_ROMFS_ON_MTD
> +       /* Use same dev ID from the underlying mtdblock device */
> +       if (sb->s_mtd)
> +               sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
> +       else
> +               sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0);

Hmm, when there is no MTD, s_dev is still equal to mtd0, since mtd0 is
->index of
value 0. This seems fishy to me.

-- 
Thanks,
//richard

Re: [PATCH v2] romfs: use different way to generate fsid for BLOCK or MTD

[Coly Li]

On 2016/12/28 下午5:44, Richard Weinberger wrote:
> CC'ing MTD
> 
> On Wed, Dec 28, 2016 at 9:53 AM, Coly Li <colyli@suse.de> wrote:
>>  static struct kmem_cache *romfs_inode_cachep;
>> @@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode)
>>  static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>  {
>>         struct super_block *sb = dentry->d_sb;
>> -       u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
>> +       u64 id = 0;
>>
>> +#ifdef CONFIG_ROMFS_ON_BLOCK
>> +       id = huge_encode_dev(sb->s_bdev->bd_dev);
>> +#endif
>> +#ifdef  CONFIG_ROMFS_ON_MTD
>> +       id = huge_encode_dev(sb->s_dev);
>> +#endif
> 
> How is this supposed to work with CONFIG_ROMFS_BACKED_BY_BOTH=y?

Aha! Thanks for telling me this config option.
The purpose of f_fsid is to unify a file system volume, if
CONFIG_ROMFS_BACKED_BY_BOTH=y, it can firstly try sb->s_bdev->bd_dev. If
sb->s_bdev is NULL, then try sb->s_dev. If both failed, then f_fsid will
be 0.

> 
>>         buf->f_type = ROMFS_MAGIC;
>>         buf->f_namelen = ROMFS_MAXFN;
>>         buf->f_bsize = ROMBSIZE;
>> @@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
>>         sb->s_flags |= MS_RDONLY | MS_NOATIME;
>>         sb->s_op = &romfs_super_ops;
>>
>> +#ifdef CONFIG_ROMFS_ON_MTD
>> +       /* Use same dev ID from the underlying mtdblock device */
>> +       if (sb->s_mtd)
>> +               sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
>> +       else
>> +               sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0);
> 
> Hmm, when there is no MTD, s_dev is still equal to mtd0, since mtd0 is
> ->index of
> value 0. This seems fishy to me.

You are right, if both CONFIG_ROMFS_ON_BLOCK and CONFIG_ROMFS_ON_MTD are
defined, here is buggy.

Thanks for your review, I will send out another version.

Coly