diff mbox

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

Message ID AANLkTimB1DsCd8HuMDc9jgVOnOAE_PVd2ktssp3TIwxc@mail.gmail.com
State New, archived
Headers show

Commit Message

Kirill A. Shutemov May 17, 2010, 1:55 p.m. UTC 
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>
Acked-by: Jan Kara <jack@suse.cz>
---
Changelog:
 v4 -> v5:
 - Fix race pointed by David Woodhouse.
 v3 -> v4:
 - Use igrab() instead of __iget inside the inode_lock;
 - Add stable@ to CC list.
 v2 -> v3:
 - Rebase to mtd-2.6.git.
 v1 -> v2:
 - Fix error handling based on comments by Jan Kara.

---
 drivers/mtd/mtdchar.c   |   80 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/mtdcore.c   |    3 ++
 include/linux/mtd/mtd.h |    3 ++
 3 files changed, 78 insertions(+), 8 deletions(-)

 	int (*write) (struct mtd_info *mtd, loff_t to, size_t len, size_t
*retlen, const u_char *buf);

Comments

David Woodhouse May 17, 2010, 2:10 p.m. UTC | #1 
On Mon, 2010-05-17 at 16:55 +0300, Kirill A. Shutemov wrote:
> +       if (!mtd->inode) {
> +               struct inode *mtd_inode;
> +               mtd_inode = new_inode(mtd_inode_mnt->mnt_sb);
> +               if (!mtd_inode) {
> +                       put_mtd_device(mtd);
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +               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;
> +               }
> +
> +               if (unlikely(cmpxchg(&mtd->inode, NULL, mtd_inode))) {
> +                       /* Somebody has already initialized mtd->inode
> */
> +                       iput(mtd_inode);
> +               }
> +       }
> +
> +       igrab(mtd->inode); 

Now you're just reimplementing iget().

Can we just use iget_locked() to get the appropriate inode (using the
mtd device number as the inode number)? Then we don't need to bother
storing it in mtd->inode at all, and we can iput() the last refcount on
it when the chardevice is closed -- we don't need it to stick around
until the MTD device is destroyed.
David Woodhouse May 18, 2010, 11:27 a.m. UTC | #2 
On Mon, 2010-05-17 at 15:10 +0100, David Woodhouse wrote:
> Now you're just reimplementing iget().
> 
> Can we just use iget_locked() to get the appropriate inode (using the
> mtd device number as the inode number)? Then we don't need to bother
> storing it in mtd->inode at all, and we can iput() the last refcount on
> it when the chardevice is closed -- we don't need it to stick around
> until the MTD device is destroyed. 

... and we don't _want_ it to stick around until the MTD device is
destroyed, either. We may actually unload the mtdchar module and its
inodes will be destroyed when we unregister the fs type; we don't want
stale pointers to them hanging around to be reused if/when we reload the
mtdchar module later.

I've committed a version which keeps it entirely within the mtdchar
code: http://git.infradead.org/mtd-2.6.git/commitdiff/cd874237

Thanks.
Kirill A. Shutemov May 18, 2010, 12:18 p.m. UTC | #3 
On Tue, May 18, 2010 at 2:27 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Mon, 2010-05-17 at 15:10 +0100, David Woodhouse wrote:
>> Now you're just reimplementing iget().
>>
>> Can we just use iget_locked() to get the appropriate inode (using the
>> mtd device number as the inode number)? Then we don't need to bother
>> storing it in mtd->inode at all, and we can iput() the last refcount on
>> it when the chardevice is closed -- we don't need it to stick around
>> until the MTD device is destroyed.
>
> ... and we don't _want_ it to stick around until the MTD device is
> destroyed, either. We may actually unload the mtdchar module and its
> inodes will be destroyed when we unregister the fs type; we don't want
> stale pointers to them hanging around to be reused if/when we reload the
> mtdchar module later.
>
> I've committed a version which keeps it entirely within the mtdchar
> code: http://git.infradead.org/mtd-2.6.git/commitdiff/cd874237

Looks ok. Thank you.

What about stable@?

>
> Thanks.
>
> --
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>
>
David Woodhouse May 18, 2010, 12:45 p.m. UTC | #4 
On Tue, 2010-05-18 at 15:18 +0300, Kirill A. Shutemov wrote:
> > http://git.infradead.org/mtd-2.6.git/commitdiff/cd874237
>
> Looks ok. Thank you.
> 
> What about stable@? 

Ah, yes. For future reference, we should have included an explicit
'Cc: stable@kernel.org' in the commit itself, rather than just in the
email thread. As it is, we'll probably need to wait for it to hit Linus'
tree and then submit a backport to -stable. It'll need a slight massage
to apply to older kernels anyway.
diff mbox

Patch

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index c355491..eb672ec 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
@@ -85,11 +88,33 @@  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) {
+		struct inode *mtd_inode;
+		mtd_inode = new_inode(mtd_inode_mnt->mnt_sb);
+		if (!mtd_inode) {
+			put_mtd_device(mtd);
+			ret = -ENOMEM;
+			goto out;
+		}
+		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;
+		}
+
+		if (unlikely(cmpxchg(&mtd->inode, NULL, mtd_inode))) {
+			/* Somebody has already initialized mtd->inode */
+			iput(mtd_inode);
+		}
+	}
+
+	igrab(mtd->inode);
+	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)) {
+		iput(mtd->inode);
 		put_mtd_device(mtd);
 		ret = -EACCES;
 		goto out;
@@ -97,6 +122,7 @@  static int mtd_open(struct inode *inode, struct file *file)

 	mfi = kzalloc(sizeof(*mfi), GFP_KERNEL);
 	if (!mfi) {
+		iput(mtd->inode);
 		put_mtd_device(mtd);
 		ret = -ENOMEM;
 		goto out;
@@ -122,6 +148,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);
@@ -951,22 +979,58 @@  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, 0, 1 << MINORBITS,
+	ret = __register_chrdev(MTD_CHAR_MAJOR, 0, 1 << MINORBITS,
 				   "mtd", &mtd_fops);
-	if (status < 0) {
-		printk(KERN_NOTICE "Can't allocate major number %d for Memory
Technology Devices.\n",
-		       MTD_CHAR_MAJOR);
+	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;
 	}

-	return status;
+	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 ret;
+
+err_unregister_filesystem:
+	unregister_filesystem(&mtd_inodefs_type);
+err_unregister_chdev:
+	unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
+	return ret;
 }

 static void __exit cleanup_mtdchar(void)
 {
+	mntput(mtd_inode_mnt);
+	unregister_filesystem(&mtd_inodefs_type);
 	__unregister_chrdev(MTD_CHAR_MAJOR, 0, 1 << MINORBITS, "mtd");
 }

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 3ae06c8..54aab32 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -399,6 +399,9 @@  int del_mtd_device (struct mtd_info *mtd)
 		       mtd->index, mtd->name, mtd->usecount);
 		ret = -EBUSY;
 	} else {
+		if (mtd->inode)
+			iput(mtd->inode);
+
 		device_unregister(&mtd->dev);

 		idr_remove(&mtd_idr, mtd->index);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 5326435..d66409a 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>
@@ -174,6 +175,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);