[PATCH] mtd: Do not corrupt backing device for inode

[PATCH] mtd: Do not corrupt backing device for inode

[Kirill A. Shutemov]

We cannot modify file->f_mapping->backing_dev_info directly, because it
will corrupt backing device for inode, since inode->i_mapping ==
file->f_mapping (see __dentry_open() in fs/open.c).

So we have to copy f_mapping first.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 drivers/mtd/mtdchar.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 5b081cb..d85be4d 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -88,8 +88,23 @@ static int mtd_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 
-	if (mtd->backing_dev_info)
+	if (mtd->backing_dev_info) {
+		/*
+		 * We assume that file->f_mapping is equal to inode->i_mapping.
+		 */
+		BUG_ON(file->f_mapping != inode->i_mapping);
+
+		/*
+		 * We cannot modify file->f_mapping->backing_dev_info directly,
+		 * because it will corrupt backing device for inode, since
+		 * inode->i_mapping is equal to file->f_mapping. So we have to
+		 * copy f_mapping first.
+		 */
+		file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL);
+		memcpy(file->f_mapping, inode->i_mapping,
+				sizeof(*file->f_mapping));
 		file->f_mapping->backing_dev_info = mtd->backing_dev_info;
+	}
 
 	/* You can't open it RW if it's not a writeable device */
 	if ((file->f_mode & FMODE_WRITE) && !(mtd->flags & MTD_WRITEABLE)) {
@@ -129,6 +144,11 @@ static int mtd_close(struct inode *inode, struct file *file)
 	file->private_data = NULL;
 	kfree(mfi);
 
+	if (mtd->backing_dev_info) {
+		kfree(file->f_mapping);
+		file->f_mapping = inode->i_mapping;
+	}
+
 	return 0;
 } /* mtd_close */
 
-- 
1.7.0.4

Re: [PATCH] mtd: Do not corrupt backing device for inode

[David Woodhouse]

On Wed, 2010-04-21 at 18:21 +0300, Kirill A. Shutemov wrote:
> +		/*
> +		 * We cannot modify file->f_mapping->backing_dev_info directly,
> +		 * because it will corrupt backing device for inode, since
> +		 * inode->i_mapping is equal to file->f_mapping. So we have to
> +		 * copy f_mapping first.
> +		 */
> +		file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL);
> +		memcpy(file->f_mapping, inode->i_mapping,
> +				sizeof(*file->f_mapping));
>  		file->f_mapping->backing_dev_info = mtd->backing_dev_info;
> +	}

Ick. What about the rest of file->f_mapping? That'll still be inherited.

Jan pointed at drivers/char/raw.c as an example, but that doesn't do
anything as ugly as this -- that sets file->f_mapping to point at
bdev->bd_inode->i_mapping instead.

I suspect we should do something similar -- have an inode for the MTD
device, with a valid i_data of its own.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] mtd: Do not corrupt backing device for inode

[Jan Kara]

On Thu 22-04-10 12:08:55, David Woodhouse wrote:
> On Wed, 2010-04-21 at 18:21 +0300, Kirill A. Shutemov wrote:
> > +		/*
> > +		 * We cannot modify file->f_mapping->backing_dev_info directly,
> > +		 * because it will corrupt backing device for inode, since
> > +		 * inode->i_mapping is equal to file->f_mapping. So we have to
> > +		 * copy f_mapping first.
> > +		 */
> > +		file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL);
> > +		memcpy(file->f_mapping, inode->i_mapping,
> > +				sizeof(*file->f_mapping));
> >  		file->f_mapping->backing_dev_info = mtd->backing_dev_info;
> > +	}
> 
> Ick. What about the rest of file->f_mapping? That'll still be inherited.
> 
> Jan pointed at drivers/char/raw.c as an example, but that doesn't do
> anything as ugly as this -- that sets file->f_mapping to point at
> bdev->bd_inode->i_mapping instead.
> 
> I suspect we should do something similar -- have an inode for the MTD
> device, with a valid i_data of its own.
  Yes, that's what I meant by my suggestion... Sorry if I wasn't clear
enough.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH] mtd: Do not corrupt backing device of device node inode

[Kirill A. Shutemov]

We cannot modify file->f_mapping->backing_dev_info, because it will corrupt
backing device of device node inode, since file->f_mapping is equal to
inode->i_mapping (see __dentry_open() in fs/open.c).

Let's introduce separate inode for MTD device with appropriate backing
device.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 drivers/mtd/mtdchar.c   |   70 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/mtdcore.c   |    3 ++
 include/linux/mtd/mtd.h |    3 ++
 3 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 5b081cb..24ea34f 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -15,12 +15,15 @@
 #include <linux/smp_lock.h>
 #include <linux/backing-dev.h>
 #include <linux/compat.h>
+#include <linux/mount.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/compatmac.h>
 
 #include <asm/uaccess.h>
 
+#define MTD_INODE_FS_MAGIC 0x11307854
+static struct vfsmount *mtd_inode_mnt __read_mostly;
 
 /*
  * Data structure to hold the pointer to the mtd device as well
@@ -88,8 +91,21 @@ static int mtd_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 
-	if (mtd->backing_dev_info)
-		file->f_mapping->backing_dev_info = mtd->backing_dev_info;
+	if (!mtd->inode) {
+		mtd->inode = new_inode(mtd_inode_mnt->mnt_sb);
+		mtd->inode->i_mode = S_IFCHR;
+		mtd->inode->i_rdev = inode->i_rdev;
+		if (mtd->backing_dev_info) {
+			mtd->inode->i_data.backing_dev_info =
+				mtd->backing_dev_info;
+		}
+	}
+
+	spin_lock(&inode_lock);
+	__iget(mtd->inode);
+	spin_unlock(&inode_lock);
+
+	file->f_mapping = mtd->inode->i_mapping;
 
 	/* You can't open it RW if it's not a writeable device */
 	if ((file->f_mode & FMODE_WRITE) && !(mtd->flags & MTD_WRITEABLE)) {
@@ -125,6 +141,8 @@ static int mtd_close(struct inode *inode, struct file *file)
 	if ((file->f_mode & FMODE_WRITE) && mtd->sync)
 		mtd->sync(mtd);
 
+	iput(mtd->inode);
+
 	put_mtd_device(mtd);
 	file->private_data = NULL;
 	kfree(mfi);
@@ -954,21 +972,57 @@ static const struct file_operations mtd_fops = {
 #endif
 };
 
+static int mtd_inodefs_get_sb(struct file_system_type *fs_type, int flags,
+                               const char *dev_name, void *data,
+                               struct vfsmount *mnt)
+{
+        return get_sb_pseudo(fs_type, "mtd_inode:", NULL, MTD_INODE_FS_MAGIC,
+                             mnt);
+}
+
+static struct file_system_type mtd_inodefs_type = {
+       .name = "mtd_inodefs",
+       .get_sb = mtd_inodefs_get_sb,
+       .kill_sb = kill_anon_super,
+};
+
 static int __init init_mtdchar(void)
 {
-	int status;
+	int ret;
 
-	status = register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops);
-	if (status < 0) {
-		printk(KERN_NOTICE "Can't allocate major number %d for Memory Technology Devices.\n",
-		       MTD_CHAR_MAJOR);
+	ret = register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops);
+	if (ret < 0) {
+		pr_notice("Can't allocate major number %d for "
+				"Memory Technology Devices.\n", MTD_CHAR_MAJOR);
+		return ret;
+	}
+
+	ret = register_filesystem(&mtd_inodefs_type);
+	if (ret) {
+		pr_notice("Can't register mtd_inodefs filesystem: %d\n", ret);
+		goto err_unregister_chdev;
+	}
+
+	mtd_inode_mnt = kern_mount(&mtd_inodefs_type);
+	if (IS_ERR(mtd_inode_mnt)) {
+		ret = PTR_ERR(mtd_inode_mnt);
+		pr_notice("Error mounting mtd_inodefs filesystem: %d\n", ret);
+		goto err_unregister_filesystem;
 	}
 
-	return status;
+	return ret;
+
+err_unregister_chdev:
+	unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
+err_unregister_filesystem:
+	unregister_filesystem(&mtd_inodefs_type);
+	return ret;
 }
 
 static void __exit cleanup_mtdchar(void)
 {
+	mntput(mtd_inode_mnt);
+	unregister_filesystem(&mtd_inodefs_type);
 	unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
 }
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index b177e75..980919e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -383,6 +383,9 @@ int del_mtd_device (struct mtd_info *mtd)
 	} else {
 		struct mtd_notifier *not;
 
+		if (mtd->inode)
+			iput(mtd->inode);
+
 		device_unregister(&mtd->dev);
 
 		/* No need to get a refcount on the module containing
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 0f32a9b..0589632 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -12,6 +12,7 @@
 #include <linux/uio.h>
 #include <linux/notifier.h>
 #include <linux/device.h>
+#include <linux/fs.h>
 
 #include <linux/mtd/compatmac.h>
 #include <mtd/mtd-abi.h>
@@ -177,6 +178,8 @@ struct mtd_info {
 	 */
 	struct backing_dev_info *backing_dev_info;
 
+	/* inode for mtd device */
+	struct inode *inode;
 
 	int (*read) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf);
 	int (*write) (struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf);
-- 
1.7.0.4

Re: [PATCH] mtd: Do not corrupt backing device of device node inode

[Jan Kara]

On Mon 03-05-10 19:56:07, Kirill A. Shutemov wrote:
> We cannot modify file->f_mapping->backing_dev_info, because it will corrupt
> backing device of device node inode, since file->f_mapping is equal to
> inode->i_mapping (see __dentry_open() in fs/open.c).
> 
> Let's introduce separate inode for MTD device with appropriate backing
> device.
  Now the patch looks much cleaner. Thanks! Having a separate fstype for a
single inode seems like a bit of an overkill but I agree there doesn't seem
to be a suitable filesystem where an inode could live, so it's probably OK.
Two minor comments are below.

> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> ---
>  drivers/mtd/mtdchar.c   |   70 +++++++++++++++++++++++++++++++++++++++++-----
>  drivers/mtd/mtdcore.c   |    3 ++
>  include/linux/mtd/mtd.h |    3 ++
>  3 files changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 5b081cb..24ea34f 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -88,8 +91,21 @@ static int mtd_open(struct inode *inode, struct file *file)
>  		goto out;
>  	}
>  
> -	if (mtd->backing_dev_info)
> -		file->f_mapping->backing_dev_info = mtd->backing_dev_info;
> +	if (!mtd->inode) {
> +		mtd->inode = new_inode(mtd_inode_mnt->mnt_sb);
  This can fail so you should check whether mtd->inode is != NULL.

...
>  static int __init init_mtdchar(void)
>  {
> -	int status;
> +	int ret;
>  
> -	status = register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops);
> -	if (status < 0) {
> -		printk(KERN_NOTICE "Can't allocate major number %d for Memory Technology Devices.\n",
> -		       MTD_CHAR_MAJOR);
> +	ret = register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops);
> +	if (ret < 0) {
> +		pr_notice("Can't allocate major number %d for "
> +				"Memory Technology Devices.\n", MTD_CHAR_MAJOR);
> +		return ret;
> +	}
> +
> +	ret = register_filesystem(&mtd_inodefs_type);
> +	if (ret) {
> +		pr_notice("Can't register mtd_inodefs filesystem: %d\n", ret);
> +		goto err_unregister_chdev;
> +	}
> +
> +	mtd_inode_mnt = kern_mount(&mtd_inodefs_type);
> +	if (IS_ERR(mtd_inode_mnt)) {
> +		ret = PTR_ERR(mtd_inode_mnt);
> +		pr_notice("Error mounting mtd_inodefs filesystem: %d\n", ret);
> +		goto err_unregister_filesystem;
>  	}
>  
> -	return status;
> +	return ret;
> +
> +err_unregister_chdev:
> +	unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
> +err_unregister_filesystem:
> +	unregister_filesystem(&mtd_inodefs_type);
> +	return ret;
  I think you should swap unregister_chrdev and unregister_filesystem
blocks...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR