lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <963931f5-88da-a3f0-a963-3291636fe624@windriver.com>
Date:   Tue, 13 Sep 2022 13:52:52 +0800
From:   Liwei Song <liwei.song@...driver.com>
To:     ChristophHellwig <hch@....de>
Cc:     MiquelRaynal <miquel.raynal@...tlin.com>,
        RichardWeinberger <richard@....at>,
        VigneshRaghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd_blkdevs: add mtd_table_mutex lock back to
 blktrans_{open, release} to avoid race condition



On 9/9/22 22:36, ChristophHellwig wrote:
> Can you try this patch (it'll need to be split up into a few if it
> works):

Hi Christoph,

Thanks for your help,
With this patch, the race condition issue I met can be fixed, but there will be
a deadlock issue as below when boot the board:

[   10.277872] ======================================================
[   10.282765] WARNING: possible circular locking dependency detected
[   10.287661] 5.18.0-yocto-standard+ #4 Not tainted
[   10.291078] ------------------------------------------------------
[   10.295967] systemd-udevd/173 is trying to acquire lock:
[   10.299994] ffff800009999150 (mtd_table_mutex){+.+.}-{3:3}, at: blktrans_open+0x60/0x128
[   10.306854] 
               but task is already holding lock:
[   10.310097] ffff000180832718 (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_get_by_dev+0xf8/0x2f8
[   10.317464] 
               which lock already depends on the new lock.

[   10.321749] 
               the existing dependency chain (in reverse order) is:
[   10.326659] 
               -> #1 (&disk->open_mutex){+.+.}-{3:3}:
[   10.330373]        __mutex_lock+0x90/0x8f8
[   10.333202]        mutex_lock_nested+0x54/0x70
[   10.336365]        bd_register_pending_holders+0x30/0x120
[   10.340489]        device_add_disk+0x1f4/0x370
[   10.343669]        add_mtd_blktrans_dev+0x304/0x468
[   10.343691]        mtdblock_add_mtd+0x7c/0xd0
[   10.343702]        blktrans_notify_add+0x50/0x70
[   10.343713]        add_mtd_device+0x40c/0x460
[   10.343724]        add_mtd_partitions+0xa0/0x1a8
[   10.343735]        parse_mtd_partitions+0x1b4/0x400
[   10.343746]        mtd_device_parse_register+0x94/0x2a0
[   10.343757]        spi_nor_probe+0x220/0x2e8
[   10.343767]        spi_mem_probe+0x74/0xb0
[   10.343777]        spi_probe+0x88/0xe0
[   10.343787]        really_probe+0xb0/0x268
[   10.343801]        __driver_probe_device+0x7c/0xe0
[   10.343811]        driver_probe_device+0x50/0x100
[   10.343823]        __device_attach_driver+0x98/0xd0
[   10.343833]        bus_for_each_drv+0x74/0xd8
[   10.343844]        __device_attach+0xf0/0x150
[   10.343854]        device_initial_probe+0x24/0x30
[   10.343865]        bus_probe_device+0xa4/0xb0
[   10.343875]        device_add+0x424/0x8b8
[   10.343884]        __spi_add_device+0x7c/0x120
[   10.343898]        spi_add_device+0x4c/0x80
[   10.343909]        of_register_spi_device+0x228/0x380
[   10.343920]        spi_register_controller+0x3c4/0x718
[   10.343931]        devm_spi_register_controller+0x28/0x80
[   10.343943]        cqspi_probe+0x714/0x9f8
[   10.343952]        platform_probe+0x6c/0xd8
[   10.343962]        really_probe+0xb0/0x268
[   10.343972]        __driver_probe_device+0x7c/0xe0
[   10.343982]        driver_probe_device+0x50/0x100
[   10.343994]        __driver_attach+0xa4/0x100
[   10.344004]        bus_for_each_dev+0x84/0xd8
[   10.344014]        driver_attach+0x30/0x40
[   10.344024]        bus_add_driver+0x160/0x208
[   10.344034]        driver_register+0x64/0x110
[   10.344045]        __platform_driver_register+0x34/0x40
[   10.344054]        cqspi_platform_driver_init+0x20/0x28
[   10.344068]        do_one_initcall+0xa4/0x440
[   10.344080]        kernel_init_freeable+0x320/0x378
[   10.344092]        kernel_init+0x2c/0x138
[   10.344104]        ret_from_fork+0x10/0x20
[   10.344115] 
               -> #0 (mtd_table_mutex){+.+.}-{3:3}:
[   10.344137]        __lock_acquire+0x1118/0x15d8
[   10.344148]        lock_acquire+0x2b8/0x3f8
[   10.344156]        __mutex_lock+0x90/0x8f8
[   10.344166]        mutex_lock_nested+0x54/0x70
[   10.344177]        blktrans_open+0x60/0x128
[   10.344189]        blkdev_get_whole+0x3c/0xb8
[   10.344203]        blkdev_get_by_dev+0x1ac/0x2f8
[   10.344211]        blkdev_open+0x64/0xb8
[   10.344219]        do_dentry_open+0x1b0/0x3b8
[   10.344234]        vfs_open+0x38/0x48
[   10.344243]        path_openat+0x758/0x938
[   10.344255]        do_filp_open+0x94/0x118
[   10.344267]        do_sys_openat2+0x234/0x308
[   10.344278]        do_sys_open+0x84/0xc0
[   10.344286]        __arm64_sys_openat+0x2c/0x38
[   10.344295]        invoke_syscall+0x64/0x138
[   10.344309]        el0_svc_common.constprop.4+0xf8/0x118
[   10.344320]        do_el0_svc+0x80/0xa0
[   10.344330]        el0_svc+0x68/0x1c8
[   10.344339]        el0t_64_sync_handler+0x88/0xb0
[   10.344349]        el0t_64_sync+0x15c/0x160
[   10.344358] 
               other info that might help us debug this:

[   10.344362]  Possible unsafe locking scenario:

[   10.344366]        CPU0                    CPU1
[   10.344370]        ----                    ----
[   10.344373]   lock(&disk->open_mutex);
[   10.344384]                                lock(mtd_table_mutex);
[   10.344395]                                lock(&disk->open_mutex);
[   10.344406]   lock(mtd_table_mutex);
[   10.344416] 
                *** DEADLOCK ***

[   10.344420] 1 lock held by systemd-udevd/173:
[   10.344427]  #0: ffff000180832718 (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_get_by_dev+0xf8/0x2f8
[   10.344457] 
               stack backtrace:
[   10.344465] CPU: 3 PID: 173 Comm: systemd-udevd Not tainted 5.18.0-yocto-standard+ #4
[   10.344476] Hardware name: SoCFPGA Stratix 10 SoCDK (DT)
[   10.344483] Call trace:
[   10.344488]  dump_backtrace.part.6+0xf4/0x100
[   10.344498]  show_stack+0x2c/0x48
[   10.344508]  dump_stack_lvl+0x9c/0xcc
[   10.344522]  dump_stack+0x14/0x2c
[   10.344532]  print_circular_bug.isra.49+0x1a8/0x200
[   10.344546]  check_noncircular+0x124/0x138
[   10.344557]  __lock_acquire+0x1118/0x15d8
[   10.344566]  lock_acquire+0x2b8/0x3f8
[   10.344574]  __mutex_lock+0x90/0x8f8
[   10.344585]  mutex_lock_nested+0x54/0x70
[   10.344595]  blktrans_open+0x60/0x128
[   10.344607]  blkdev_get_whole+0x3c/0xb8
[   10.344618]  blkdev_get_by_dev+0x1ac/0x2f8
[   10.344627]  blkdev_open+0x64/0xb8
[   10.344636]  do_dentry_open+0x1b0/0x3b8
[   10.344647]  vfs_open+0x38/0x48
[   10.344656]  path_openat+0x758/0x938
[   10.344666]  do_filp_open+0x94/0x118
[   10.344676]  do_sys_openat2+0x234/0x308
[   10.344687]  do_sys_open+0x84/0xc0
[   10.344695]  __arm64_sys_openat+0x2c/0x38
[   10.344704]  invoke_syscall+0x64/0x138
[   10.344715]  el0_svc_common.constprop.4+0xf8/0x118
[   10.344726]  do_el0_svc+0x80/0xa0
[   10.344736]  el0_svc+0x68/0x1c8
[   10.344745]  el0t_64_sync_handler+0x88/0xb0
[   10.344754]  el0t_64_sync+0x15c/0x160


Thanks,
Liwei.


> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 60b222799871e..9eda1dd98a406 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -24,24 +24,16 @@
>  
>  static LIST_HEAD(blktrans_majors);
>  
> -static void blktrans_dev_release(struct kref *kref)
> +static void blktrans_free_disk(struct gendisk *disk)
>  {
> -	struct mtd_blktrans_dev *dev =
> -		container_of(kref, struct mtd_blktrans_dev, ref);
> +	struct mtd_blktrans_dev *dev = disk->private_data;
>  
> -	put_disk(dev->disk);
>  	blk_mq_free_tag_set(dev->tag_set);
>  	kfree(dev->tag_set);
>  	list_del(&dev->list);
>  	kfree(dev);
>  }
>  
> -static void blktrans_dev_put(struct mtd_blktrans_dev *dev)
> -{
> -	kref_put(&dev->ref, blktrans_dev_release);
> -}
> -
> -
>  static blk_status_t do_blktrans_request(struct mtd_blktrans_ops *tr,
>  			       struct mtd_blktrans_dev *dev,
>  			       struct request *req)
> @@ -187,63 +179,58 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
>  	struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
>  	int ret = 0;
>  
> -	kref_get(&dev->ref);
> +	if (disk_openers(bdev->bd_disk) > 0)
> +		return 0;
>  
> -	mutex_lock(&dev->lock);
> -
> -	if (dev->open)
> -		goto unlock;
> +	mutex_lock(&mtd_table_mutex);
> +	if (!dev->mtd) {
> +		mutex_lock(&mtd_table_mutex);
> +		return -EINVAL;
> +	}
> +	ret = __get_mtd_device(dev->mtd);
> +	mutex_unlock(&mtd_table_mutex);
> +	if (ret)
> +		return ret;
>  
> +	mutex_lock(&dev->lock);
>  	__module_get(dev->tr->owner);
> -
> -	if (!dev->mtd)
> -		goto unlock;
> -
>  	if (dev->tr->open) {
>  		ret = dev->tr->open(dev);
>  		if (ret)
>  			goto error_put;
>  	}
> -
> -	ret = __get_mtd_device(dev->mtd);
> -	if (ret)
> -		goto error_release;
>  	dev->file_mode = mode;
> -
> -unlock:
>  	dev->open++;
>  	mutex_unlock(&dev->lock);
> -	return ret;
>  
> -error_release:
> -	if (dev->tr->release)
> -		dev->tr->release(dev);
> +	return 0;
> +
>  error_put:
>  	module_put(dev->tr->owner);
>  	mutex_unlock(&dev->lock);
> -	blktrans_dev_put(dev);
> +
> +	put_mtd_device(dev->mtd);
>  	return ret;
>  }
>  
>  static void blktrans_release(struct gendisk *disk, fmode_t mode)
>  {
>  	struct mtd_blktrans_dev *dev = disk->private_data;
> +	struct mtd_info *mtd = NULL;
>  
> -	mutex_lock(&dev->lock);
> -
> -	if (--dev->open)
> -		goto unlock;
> +	if (disk_openers(disk) > 0)
> +		return;
>  
> +	mutex_lock(&dev->lock);
> +	dev->open--;
>  	module_put(dev->tr->owner);
> -
> -	if (dev->mtd) {
> -		if (dev->tr->release)
> -			dev->tr->release(dev);
> -		__put_mtd_device(dev->mtd);
> -	}
> -unlock:
> +	mtd = dev->mtd;
> +	if (mtd && dev->tr->release)
> +		dev->tr->release(dev);
>  	mutex_unlock(&dev->lock);
> -	blktrans_dev_put(dev);
> +
> +	if (mtd)
> +		put_mtd_device(dev->mtd);
>  }
>  
>  static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> @@ -266,6 +253,7 @@ static const struct block_device_operations mtd_block_ops = {
>  	.owner		= THIS_MODULE,
>  	.open		= blktrans_open,
>  	.release	= blktrans_release,
> +	.free_disk	= blktrans_free_disk,
>  	.getgeo		= blktrans_getgeo,
>  };
>  
> @@ -318,7 +306,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>   added:
>  
>  	mutex_init(&new->lock);
> -	kref_init(&new->ref);
>  	if (!tr->writesect)
>  		new->readonly = 1;
>  
> @@ -410,6 +397,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  
>  int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  {
> +	struct mtd_info *old_mtd = NULL;
>  	unsigned long flags;
>  
>  	lockdep_assert_held(&mtd_table_mutex);
> @@ -438,13 +426,14 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  	if (old->open) {
>  		if (old->tr->release)
>  			old->tr->release(old);
> -		__put_mtd_device(old->mtd);
> +		old_mtd = old->mtd;
>  	}
> -
>  	old->mtd = NULL;
> -
>  	mutex_unlock(&old->lock);
> -	blktrans_dev_put(old);
> +	put_disk(old->disk);
> +
> +	if (old->mtd)
> +		put_mtd_device(old_mtd);
>  	return 0;
>  }
>  
> diff --git a/include/linux/mtd/blktrans.h b/include/linux/mtd/blktrans.h
> index 15cc9b95e32b5..41a81fc9f0462 100644
> --- a/include/linux/mtd/blktrans.h
> +++ b/include/linux/mtd/blktrans.h
> @@ -7,7 +7,6 @@
>  #define __MTD_TRANS_H__
>  
>  #include <linux/mutex.h>
> -#include <linux/kref.h>
>  #include <linux/sysfs.h>
>  
>  struct hd_geometry;
> @@ -26,7 +25,6 @@ struct mtd_blktrans_dev {
>  	unsigned long size;
>  	int readonly;
>  	int open;
> -	struct kref ref;
>  	struct gendisk *disk;
>  	struct attribute_group *disk_attributes;
>  	struct request_queue *rq;
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ