Re: [PATCH v4] block/loop: Serialize ioctl operations.

[Tetsuo Handa]

On 2018/09/24 21:31, Jan Kara wrote:
> On Mon 24-09-18 19:29:10, Tetsuo Handa wrote:
>> On 2018/09/24 7:03, Ming Lei wrote:
>>> On Sat, Sep 22, 2018 at 09:39:02PM +0900, Tetsuo Handa wrote:
>>>> Hello, Ming Lei.
>>>>
>>>> I'd like to hear your comment on this patch regarding the ordering of
>>>> stopping kernel thread.
>>>>
>>>>   > In order to enforce this strategy, this patch inversed
>>>>   > loop_reread_partitions() and loop_unprepare_queue() in loop_clr_fd().
>>>>   > I don't know whether it breaks something, but I don't have testcases.
>>>>
>>>> Until 3.19, kthread_stop(lo->lo_thread) was called before
>>>> ioctl_by_bdev(bdev, BLKRRPART, 0) is called.
>>>> During 4.0 to 4.3, the loop module was using "kloopd" workqueue.
>>>> But since 4.4, loop_reread_partitions(lo, bdev) is called before
>>>> loop_unprepare_queue(lo) is called. And this patch is trying to change to
>>>> call loop_unprepare_queue() before loop_reread_partitions() is called.
>>>> Is there some reason we need to preserve current ordering?
>>>
>>> IMO, both the two orders are fine, and what matters is that 'lo->lo_state'
>>> is updated before loop_reread_partitions(), then any IO from loop_reread_partitions
>>> will be failed, so it shouldn't be a big deal wrt. the order between
>>> loop_reread_partitions() and loop_unprepare_queue().
>>
>> OK. Thank you. Here is v4 patch (only changelog was updated).
>> Andrew, can we test this patch in the -mm tree?
>>
>> From 2278250ac8c5b912f7eb7af55e36ed40e2f7116b Mon Sep 17 00:00:00 2001
>> From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
>> Date: Mon, 24 Sep 2018 18:58:37 +0900
>> Subject: [PATCH v4] block/loop: Serialize ioctl operations.
>>
>> syzbot is reporting NULL pointer dereference [1] which is caused by
>> race condition between ioctl(loop_fd, LOOP_CLR_FD, 0) versus
>> ioctl(other_loop_fd, LOOP_SET_FD, loop_fd) due to traversing other
>> loop devices without holding corresponding locks.
>>
>> syzbot is also reporting circular locking dependency between bdev->bd_mutex
>> and lo->lo_ctl_mutex [2] which is caused by calling blkdev_reread_part()
>> with lock held.
> 
> Thanks for looking into the loop crashes Tetsuo. I was looking into the
> loop code and trying to understand how your patch fixes them but I've
> failed. Can you please elaborate a bit on how exactly LOOP_CLR_FD and
> LOOP_SET_FD race to hit NULL pointer dereference? I don't really see the
> code traversing other loop devices as you mention in your changelog so I'm
> probably missing something. Thanks.
> 
> 								Honza
> 

That is explained in a discussion for [1] at
https://groups.google.com/forum/#!msg/syzkaller-bugs/c8KUcTAzTvA/3o_7g6-tAwAJ .
In the current code, the location of dangerous traversal is in loop_validate_file().