* [BUG] Rewriting backing_dev_info in MTD
@ 2010-04-13 11:33 ` Kirill A. Shutemov
0 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-04-13 11:33 UTC (permalink / raw)
To: Alexander Viro, David Woodhouse
Cc: linux-fsdevel, linux-mtd, David Howells, Bernd Schmidt,
Alexander Shishkin
I've got NULL-pointer dereference in __mark_inode_dirty() on chmod()
for MTD device node. wb->bdi was NULL in this case.
During investigation I've found that MTD subsystem rewrites
file->f_mapping->backing_dev_info on openning to get mmap() work on
MMU-less systems. But in fact it rewrites
inode->i_mapping->backing_dev_info too, since inode->i_mapping ==
file->f_mapping (see __dentry_open() in fs/open.c). It breaks
writeback of inode changes.
I guess the right way to fix this is changing of __dentry_open() to
create _copy_ of i_mapping to assign to f_mapping since in common case
f_mapping != i_mapping. But I'm not sure were the copy should be
freed.
What do you think?
^ permalink raw reply [flat|nested] 14+ messages in thread
* [BUG] Rewriting backing_dev_info in MTD
@ 2010-04-13 11:33 ` Kirill A. Shutemov
0 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-04-13 11:33 UTC (permalink / raw)
To: Alexander Viro, David Woodhouse
Cc: linux-fsdevel, David Howells, Bernd Schmidt, linux-mtd,
Alexander Shishkin
I've got NULL-pointer dereference in __mark_inode_dirty() on chmod()
for MTD device node. wb->bdi was NULL in this case.
During investigation I've found that MTD subsystem rewrites
file->f_mapping->backing_dev_info on openning to get mmap() work on
MMU-less systems. But in fact it rewrites
inode->i_mapping->backing_dev_info too, since inode->i_mapping ==
file->f_mapping (see __dentry_open() in fs/open.c). It breaks
writeback of inode changes.
I guess the right way to fix this is changing of __dentry_open() to
create _copy_ of i_mapping to assign to f_mapping since in common case
f_mapping != i_mapping. But I'm not sure were the copy should be
freed.
What do you think?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [BUG] Rewriting backing_dev_info in MTD
2010-04-13 11:33 ` Kirill A. Shutemov
@ 2010-04-15 17:23 ` Jan Kara
-1 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-04-15 17:23 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Alexander Viro, David Woodhouse, linux-fsdevel, linux-mtd,
David Howells, Bernd Schmidt, Alexander Shishkin
On Tue 13-04-10 14:33:01, Kirill A. Shutemov wrote:
> I've got NULL-pointer dereference in __mark_inode_dirty() on chmod()
> for MTD device node. wb->bdi was NULL in this case.
>
> During investigation I've found that MTD subsystem rewrites
> file->f_mapping->backing_dev_info on openning to get mmap() work on
> MMU-less systems. But in fact it rewrites
> inode->i_mapping->backing_dev_info too, since inode->i_mapping ==
> file->f_mapping (see __dentry_open() in fs/open.c). It breaks
> writeback of inode changes.
I think the right trick is to not overwrite
file->f_mapping->backing_dev_info but rather change already
file->f_mapping. For example drivers/char/raw.c does this. Then you'll stop
having problems with writeback code going wild.
> I guess the right way to fix this is changing of __dentry_open() to
> create _copy_ of i_mapping to assign to f_mapping since in common case
> f_mapping != i_mapping. But I'm not sure were the copy should be
> freed.
No, in most cases we will leave f_mapping == i_mapping so copying
i_mapping would be an overkill.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [BUG] Rewriting backing_dev_info in MTD
@ 2010-04-15 17:23 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-04-15 17:23 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Bernd Schmidt, David Howells, Alexander Shishkin, linux-mtd,
Alexander Viro, linux-fsdevel, David Woodhouse
On Tue 13-04-10 14:33:01, Kirill A. Shutemov wrote:
> I've got NULL-pointer dereference in __mark_inode_dirty() on chmod()
> for MTD device node. wb->bdi was NULL in this case.
>
> During investigation I've found that MTD subsystem rewrites
> file->f_mapping->backing_dev_info on openning to get mmap() work on
> MMU-less systems. But in fact it rewrites
> inode->i_mapping->backing_dev_info too, since inode->i_mapping ==
> file->f_mapping (see __dentry_open() in fs/open.c). It breaks
> writeback of inode changes.
I think the right trick is to not overwrite
file->f_mapping->backing_dev_info but rather change already
file->f_mapping. For example drivers/char/raw.c does this. Then you'll stop
having problems with writeback code going wild.
> I guess the right way to fix this is changing of __dentry_open() to
> create _copy_ of i_mapping to assign to f_mapping since in common case
> f_mapping != i_mapping. But I'm not sure were the copy should be
> freed.
No, in most cases we will leave f_mapping == i_mapping so copying
i_mapping would be an overkill.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] mtd: Do not corrupt backing device for inode
2010-04-15 17:23 ` Jan Kara
@ 2010-04-21 15:21 ` Kirill A. Shutemov
-1 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-04-21 15:21 UTC (permalink / raw)
To: David Woodhouse
Cc: Jan Kara, Alexander Viro, David Howells, Alexander Shishkin,
linux-mtd, linux-fsdevel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] mtd: Do not corrupt backing device for inode
@ 2010-04-21 15:21 ` Kirill A. Shutemov
0 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-04-21 15:21 UTC (permalink / raw)
To: David Woodhouse
Cc: Jan Kara, linux-kernel, David Howells, Alexander Shishkin,
linux-mtd, Alexander Viro, linux-fsdevel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device for inode
2010-04-21 15:21 ` Kirill A. Shutemov
@ 2010-04-22 11:08 ` David Woodhouse
-1 siblings, 0 replies; 14+ messages in thread
From: David Woodhouse @ 2010-04-22 11:08 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Jan Kara, Alexander Viro, David Howells, Alexander Shishkin,
linux-mtd, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device for inode
@ 2010-04-22 11:08 ` David Woodhouse
0 siblings, 0 replies; 14+ messages in thread
From: David Woodhouse @ 2010-04-22 11:08 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Jan Kara, linux-kernel, David Howells, Alexander Shishkin,
linux-mtd, Alexander Viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device for inode
2010-04-22 11:08 ` David Woodhouse
@ 2010-04-22 15:20 ` Jan Kara
-1 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-04-22 15:20 UTC (permalink / raw)
To: David Woodhouse
Cc: Kirill A. Shutemov, Jan Kara, Alexander Viro, David Howells,
Alexander Shishkin, linux-mtd, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device for inode
@ 2010-04-22 15:20 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-04-22 15:20 UTC (permalink / raw)
To: David Woodhouse
Cc: Jan Kara, linux-kernel, David Howells, Alexander Shishkin,
linux-mtd, Alexander Viro, linux-fsdevel, Kirill A. Shutemov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] mtd: Do not corrupt backing device of device node inode
2010-04-22 11:08 ` David Woodhouse
@ 2010-05-03 16:56 ` Kirill A. Shutemov
-1 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-05-03 16:56 UTC (permalink / raw)
To: David Woodhouse
Cc: Jan Kara, Alexander Viro, David Howells, Alexander Shishkin,
Artem Bityutskiy, linux-mtd, linux-fsdevel, linux-kernel,
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] mtd: Do not corrupt backing device of device node inode
@ 2010-05-03 16:56 ` Kirill A. Shutemov
0 siblings, 0 replies; 14+ messages in thread
From: Kirill A. Shutemov @ 2010-05-03 16:56 UTC (permalink / raw)
To: David Woodhouse
Cc: Jan Kara, Artem Bityutskiy, linux-kernel, David Howells,
Alexander Shishkin, linux-mtd, Alexander Viro, linux-fsdevel,
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device of device node inode
2010-05-03 16:56 ` Kirill A. Shutemov
@ 2010-05-03 18:54 ` Jan Kara
-1 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-05-03 18:54 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: David Woodhouse, Jan Kara, Alexander Viro, David Howells,
Alexander Shishkin, Artem Bityutskiy, linux-mtd, linux-fsdevel,
linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device of device node inode
@ 2010-05-03 18:54 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2010-05-03 18:54 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Jan Kara, Artem Bityutskiy, linux-kernel, David Howells,
Alexander Shishkin, linux-mtd, Alexander Viro, linux-fsdevel,
David Woodhouse
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-05-03 18:54 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-13 11:33 [BUG] Rewriting backing_dev_info in MTD Kirill A. Shutemov
2010-04-13 11:33 ` Kirill A. Shutemov
2010-04-15 17:23 ` Jan Kara
2010-04-15 17:23 ` Jan Kara
2010-04-21 15:21 ` [PATCH] mtd: Do not corrupt backing device for inode Kirill A. Shutemov
2010-04-21 15:21 ` Kirill A. Shutemov
2010-04-22 11:08 ` David Woodhouse
2010-04-22 11:08 ` David Woodhouse
2010-04-22 15:20 ` Jan Kara
2010-04-22 15:20 ` Jan Kara
2010-05-03 16:56 ` [PATCH] mtd: Do not corrupt backing device of device node inode Kirill A. Shutemov
2010-05-03 16:56 ` Kirill A. Shutemov
2010-05-03 18:54 ` Jan Kara
2010-05-03 18:54 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.