[Devel] [PATCH vz10] block/blk-cbt: don't copy_to_user() under RCU in cbt_ioc_list()

[Pavel Tikhomirov]

On 6/4/26 11:35, Konstantin Khorenko wrote:
> cbt_ioc_list() walked q->cbt_list under rcu_read_lock() and called
> copy_to_user() for each entry inside that section. copy_to_user() may
> fault and sleep, which is illegal in an RCU read-side critical section
> ("BUG: scheduling while atomic" / sleeping-in-atomic). In addition the
> copy_to_user() failure path did "return -EFAULT" without
> rcu_read_unlock(), leaking the RCU read lock.
> 
> q->cbt_list is added to / removed from under cbt_mutex (see
> blk_cbt_add() and the BLKCBTGET show path which already walks the list
> with plain list_for_each_entry() under cbt_mutex). Take cbt_mutex here
> too: it stabilises the list, allows the sleeping copy_to_user(), and the
> error path now drops the mutex.

Let's add a comment here that it's not a fast path, so taking mutex is
a viable solution (fast path is in blk_cbt_bio_queue() and it remains rcu
based). And though it is technically possible to make this code path
lockless (as it's a readonly list operation) by adding an intermediate
page-size buffer before the loop, we probably don't really need it unless
list-opp is often called concurrently with other cbt operations.

Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>

> 
> Fixes: 85731dd819a7 ("block/blk-cbt: add BLKCBTLIST ioctl")
> https://virtuozzo.atlassian.net/browse/VSTOR-132310
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  block/blk-cbt.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
> index 8d243562161f..90219f58f1ae 100644
> --- a/block/blk-cbt.c
> +++ b/block/blk-cbt.c
> @@ -1001,16 +1001,18 @@ static int cbt_ioc_list(struct block_device *bdev, void __user *arg)
>  		       lst.count * CBT_NAME_LENGTH))
>  		return -EFAULT;
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(cbt, &q->cbt_list, list) {
> +	mutex_lock(&cbt_mutex);
> +	list_for_each_entry(cbt, &q->cbt_list, list) {
>  		if (total < lst.count) {
> -			if (copy_to_user(name_ptr, cbt->name, CBT_NAME_LENGTH))
> +			if (copy_to_user(name_ptr, cbt->name, CBT_NAME_LENGTH)) {
> +				mutex_unlock(&cbt_mutex);
>  				return -EFAULT;
> +			}
>  			name_ptr = (void *)((size_t)name_ptr + CBT_NAME_LENGTH);
>  		}
>  		total++;
>  	}
> -	rcu_read_unlock();
> +	mutex_unlock(&cbt_mutex);
>  
>  	lst.count = total;
>  

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.