On Wed, 28 Nov 2012, Jens Axboe wrote:
> On 2012-11-28 04:57, Mikulas Patocka wrote: > > > > This patch is wrong because you must check if the device is mapped while > > holding bdev->bd_block_size_semaphore (because > > bdev->bd_block_size_semaphore prevents new mappings from being created) > > No it doesn't. If you read the patch, that was moved to i_mmap_mutex. Hmm, it was wrong before the patch and it is wrong after the patch too. The problem is that ->mmap method doesn't do the actual mapping, the caller of ->mmap (mmap_region) does it. So we must actually catch mmap_region and protect it with the lock, not ->mmap. Mikulas --- Introduce a method to catch mmap_region For block devices, we must prevent the device from being mapped while block size is being changed. This was implemented by catching the mmap method and taking bd_block_size_semaphore in it. The problem is that the generic_file_mmap method does almost nothing, it only sets vma->vm_ops = &generic_file_vm_ops, all the hard work is done by the caller, mmap_region. Consequently, locking the mmap method is insufficient, to prevent the race condition. The race condition can happen for example this way: process 1: Starts mapping a block device, it goes to mmap_region, calls file->f_op->mmap (blkdev_mmap - that acquires and releases bd_block_size_semaphore), and reschedule happens just after blkdev_mmap returns. process 2: Changes block device size, goes into set_blocksize, acquires percpu_down_write, calls mapping_mapped to test if the device is mapped (it still isn't). Then, it reschedules. process 1: Continues in mmap_region, successfully finishes the mapping. process 2: Continues in set_blocksize, changes the block size while the block device is mapped. (which is incorrect and may result in a crash or misbehavior). To fix this possible race condition, we need to catch the function that does real mapping - mmap_region. This patch adds a new method, file_operations->mmap_region. mmap_region calls the method file_operations->mmap_region if it is non-NULL, otherwise, it calls generic_mmap_region. Signed-off-by: Mikulas Patocka <mpato...@redhat.com> --- fs/block_dev.c | 12 ++++++++---- include/linux/fs.h | 4 ++++ include/linux/mm.h | 3 +++ mm/mmap.c | 10 ++++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) Index: linux-3.7-rc7/include/linux/fs.h =================================================================== --- linux-3.7-rc7.orig/include/linux/fs.h 2012-11-28 17:45:55.000000000 +0100 +++ linux-3.7-rc7/include/linux/fs.h 2012-11-28 18:02:45.000000000 +0100 @@ -27,6 +27,7 @@ #include <linux/lockdep.h> #include <linux/percpu-rwsem.h> #include <linux/blk_types.h> +#include <linux/mm_types.h> #include <asm/byteorder.h> #include <uapi/linux/fs.h> @@ -1528,6 +1529,9 @@ struct file_operations { unsigned int (*poll) (struct file *, struct poll_table_struct *); long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long); long (*compat_ioctl) (struct file *, unsigned int, unsigned long); + unsigned long (*mmap_region)(struct file *, unsigned long, + unsigned long, unsigned long, vm_flags_t, + unsigned long); int (*mmap) (struct file *, struct vm_area_struct *); int (*open) (struct inode *, struct file *); int (*flush) (struct file *, fl_owner_t id); Index: linux-3.7-rc7/include/linux/mm.h =================================================================== --- linux-3.7-rc7.orig/include/linux/mm.h 2012-11-28 17:45:55.000000000 +0100 +++ linux-3.7-rc7/include/linux/mm.h 2012-11-28 17:47:12.000000000 +0100 @@ -1444,6 +1444,9 @@ extern unsigned long get_unmapped_area(s extern unsigned long mmap_region(struct file *file, unsigned long addr, unsigned long len, unsigned long flags, vm_flags_t vm_flags, unsigned long pgoff); +extern unsigned long generic_mmap_region(struct file *file, unsigned long addr, + unsigned long len, unsigned long flags, + vm_flags_t vm_flags, unsigned long pgoff); extern unsigned long do_mmap_pgoff(struct file *, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long); Index: linux-3.7-rc7/mm/mmap.c =================================================================== --- linux-3.7-rc7.orig/mm/mmap.c 2012-11-28 17:45:55.000000000 +0100 +++ linux-3.7-rc7/mm/mmap.c 2012-11-28 17:57:40.000000000 +0100 @@ -1244,6 +1244,16 @@ unsigned long mmap_region(struct file *f unsigned long len, unsigned long flags, vm_flags_t vm_flags, unsigned long pgoff) { + if (file && file->f_op->mmap_region) + return file->f_op->mmap_region(file, addr, len, flags, vm_flags, + pgoff); + return generic_mmap_region(file, addr, len, flags, vm_flags, pgoff); +} + +unsigned long generic_mmap_region(struct file *file, unsigned long addr, + unsigned long len, unsigned long flags, + vm_flags_t vm_flags, unsigned long pgoff) +{ struct mm_struct *mm = current->mm; struct vm_area_struct *vma, *prev; int correct_wcount = 0; Index: linux-3.7-rc7/fs/block_dev.c =================================================================== --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 17:45:56.000000000 +0100 +++ linux-3.7-rc7/fs/block_dev.c 2012-11-28 17:47:12.000000000 +0100 @@ -1658,14 +1658,17 @@ ssize_t blkdev_aio_write(struct kiocb *i } EXPORT_SYMBOL_GPL(blkdev_aio_write); -static int blkdev_mmap(struct file *file, struct vm_area_struct *vma) +static unsigned long blkdev_mmap_region(struct file *file, unsigned long addr, + unsigned long len, unsigned long flags, + vm_flags_t vm_flags, + unsigned long pgoff) { - int ret; + unsigned long ret; struct block_device *bdev = I_BDEV(file->f_mapping->host); percpu_down_read(&bdev->bd_block_size_semaphore); - ret = generic_file_mmap(file, vma); + ret = generic_mmap_region(file, addr, len, flags, vm_flags, pgoff); percpu_up_read(&bdev->bd_block_size_semaphore); @@ -1737,7 +1740,8 @@ const struct file_operations def_blk_fop .write = do_sync_write, .aio_read = blkdev_aio_read, .aio_write = blkdev_aio_write, - .mmap = blkdev_mmap, + .mmap_region = blkdev_mmap_region, + .mmap = generic_file_mmap, .fsync = blkdev_fsync, .unlocked_ioctl = block_ioctl, #ifdef CONFIG_COMPAT -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/