Re: [PATCH] mm: dmapool: add/remove sysfs file outside of the pool lock

[Andrew Morton]

On Thu, 11 Sep 2014 21:31:16 +0200 Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:

> cat /sys/___/pools followed by removal the device leads to:
> 
> |======================================================
> |[ INFO: possible circular locking dependency detected ]
> |3.17.0-rc4+ #1498 Not tainted
> |-------------------------------------------------------
> |rmmod/2505 is trying to acquire lock:
> | (s_active#28){++++.+}, at: [<c017f754>] kernfs_remove_by_name_ns+0x3c/0x88
> |
> |but task is already holding lock:
> | (pools_lock){+.+.+.}, at: [<c011494c>] dma_pool_destroy+0x18/0x17c
> |
> |which lock already depends on the new lock.
> 
> The problem is the lock order of pools_lock and kernfs_mutex in
> dma_pool_destroy() vs show_pools().

Important details were omitted.  What's the call path whereby
show_pools() is called under kernfs_mutex?

> This patch breaks out the creation of the sysfs file outside of the
> pools_lock mutex.

I think the patch adds races.  They're improbable, but they're there.

> In theory we would have to create the link in the error path of
> device_create_file() in case the dev->dma_pools list is not empty. In
> reality I doubt that there will be a single device creating dma-pools in
> parallel where it would matter.

Maybe you're saying the same thing here, but the changelog lacks
sufficient detail for me to tell because it doesn't explain *why* "we
would have to create the link".

> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -132,6 +132,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  {
>  	struct dma_pool *retval;
>  	size_t allocation;
> +	bool empty = false;
>  
>  	if (align == 0) {
>  		align = 1;
> @@ -173,14 +174,22 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	INIT_LIST_HEAD(&retval->pools);
>  
>  	mutex_lock(&pools_lock);
> -	if (list_empty(&dev->dma_pools) &&
> -	    device_create_file(dev, &dev_attr_pools)) {
> -		kfree(retval);
> -		return NULL;
> -	} else
> -		list_add(&retval->pools, &dev->dma_pools);
> +	if (list_empty(&dev->dma_pools))
> +		empty = true;
> +	list_add(&retval->pools, &dev->dma_pools);
>  	mutex_unlock(&pools_lock);
> -
> +	if (empty) {
> +		int err;
> +
> +		err = device_create_file(dev, &dev_attr_pools);
> +		if (err) {
> +			mutex_lock(&pools_lock);
> +			list_del(&retval->pools);
> +			mutex_unlock(&pools_lock);
> +			kfree(retval);
> +			return NULL;
> +		}
> +	}
>  	return retval;
>  }
>  EXPORT_SYMBOL(dma_pool_create);
> @@ -251,11 +260,15 @@ static void pool_free_page(struct dma_pool *pool, struct dma_page *page)
>   */
>  void dma_pool_destroy(struct dma_pool *pool)
>  {
> +	bool empty = false;
> +
>  	mutex_lock(&pools_lock);
>  	list_del(&pool->pools);
>  	if (pool->dev && list_empty(&pool->dev->dma_pools))
> -		device_remove_file(pool->dev, &dev_attr_pools);
> +		empty = true;
>  	mutex_unlock(&pools_lock);

For example, if another process now runs dma_pool_create(), it will try
to create the sysfs file and will presumably fail because it's already
there.  Then when this process runs, the file gets removed again.  So
we'll get a nasty warning from device_create_file() (I assume) and the
dma_pool_create() call will fail.

There's probably a similar race in the destroy()-interrupts-create()
path but I'm lazy.

> +	if (empty)
> +		device_remove_file(pool->dev, &dev_attr_pools);
>  


This problem is pretty ugly.

It's a bit surprising that it hasn't happened elsewhere.  Perhaps this
is because dmapool went and broke the sysfs rules and has multiple
values in a single sysfs file.  This causes dmapool to walk a list
under kernfs_lock and that list walk requires a lock.

And it's too late to fix this by switching to one-value-per-file.  Ugh.
Maybe there's some wizardly hack we can use in dma_pool_create() and
dma_pool_destroy() to avoid the races.  Maybe use your patch as-is but
add yet another mutex to serialise dma_pool_create() against
dma_pool_destroy() so they can never run concurrently?  There may
already be higher-level locking which ensures this so perhaps we can
"fix" the races with suitable code comments.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/