* [BUG] Rewriting backing_dev_info in MTD @ 2010-04-13 11:33 Kirill A. Shutemov 2010-04-15 17:23 ` Jan Kara 0 siblings, 1 reply; 7+ 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] 7+ messages in thread
* Re: [BUG] Rewriting backing_dev_info in MTD 2010-04-13 11:33 [BUG] Rewriting backing_dev_info in MTD Kirill A. Shutemov @ 2010-04-15 17:23 ` Jan Kara 2010-04-21 15:21 ` [PATCH] mtd: Do not corrupt backing device for inode Kirill A. Shutemov 0 siblings, 1 reply; 7+ 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] 7+ 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 2010-04-22 11:08 ` David Woodhouse 0 siblings, 1 reply; 7+ 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] 7+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device for inode 2010-04-21 15:21 ` [PATCH] mtd: Do not corrupt backing device for inode Kirill A. Shutemov @ 2010-04-22 11:08 ` David Woodhouse 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 0 siblings, 2 replies; 7+ 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] 7+ 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 2010-05-03 16:56 ` [PATCH] mtd: Do not corrupt backing device of device node inode Kirill A. Shutemov 1 sibling, 0 replies; 7+ 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] 7+ messages in thread
* [PATCH] mtd: Do not corrupt backing device of device node inode 2010-04-22 11:08 ` David Woodhouse 2010-04-22 15:20 ` Jan Kara @ 2010-05-03 16:56 ` Kirill A. Shutemov 2010-05-03 18:54 ` Jan Kara 1 sibling, 1 reply; 7+ 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] 7+ messages in thread
* Re: [PATCH] mtd: Do not corrupt backing device of device node inode 2010-05-03 16:56 ` [PATCH] mtd: Do not corrupt backing device of device node inode Kirill A. Shutemov @ 2010-05-03 18:54 ` Jan Kara 0 siblings, 0 replies; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2010-05-03 18:54 UTC | newest] Thread overview: 7+ 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-15 17:23 ` Jan Kara 2010-04-21 15:21 ` [PATCH] mtd: Do not corrupt backing device for inode Kirill A. Shutemov 2010-04-22 11:08 ` David Woodhouse 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 18:54 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).