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 <[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;
>
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
