Patchwork mtd: Do not corrupt backing device for inode

login
register
mail settings
Submitter Kirill A. Shutemov
Date April 21, 2010, 3:21 p.m.
Message ID <1271863269-5423-1-git-send-email-kirill@shutemov.name>
Download mbox | patch
Permalink /patch/50661/
State New
Headers show

Comments

Kirill A. Shutemov - April 21, 2010, 3:21 p.m.
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(-)
David Woodhouse - April 22, 2010, 11:08 a.m.
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.
Jan Kara - April 22, 2010, 3:20 p.m.
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

Patch

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 */