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 <[email protected]>
>
>>
>> Fixes: 85731dd819a7 ("block/blk-cbt: add BLKCBTLIST ioctl")
>> https://virtuozzo.atlassian.net/browse/VSTOR-132310
>> Signed-off-by: Konstantin Khorenko <[email protected]>
>> ---
>> 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;
>>
>
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
