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

[Konstantin Khorenko]

On 6/15/26 15:35, Pavel Tikhomirov wrote:
> 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.

Ok, i will put this into the TODO list on next rebase.

> 
> 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;
>>  
>