Re: [PATCH 5.10.y] dm: fix NULL pointer dereference in __dm_suspend()

Zheng Qixing
2025-10-22T09:14:08Z

Hi,

在 2025/10/22 17:10, Li Lingfeng 写道:
>
> 在 2025/10/22 16:45, Greg KH 写道:
>> On Wed, Oct 22, 2025 at 04:21:33PM +0800, Li Lingfeng wrote:
>>> Hi,
>>>
>>> 在 2025/10/22 16:10, Greg KH 写道:
>>>> On Wed, Oct 22, 2025 at 04:01:04PM +0800, Li Lingfeng wrote:
>>>>> Hi,
>>>>>
>>>>> 在 2025/10/14 11:03, Sasha Levin 写道:
>>>>>> From: Zheng Qixing <zhengqixing@huawei.com>
>>>>>>
>>>>>> [ Upstream commit 8d33a030c566e1f105cd5bf27f37940b6367f3be ]
>>>>>>
>>>>>> There is a race condition between dm device suspend and table
>>>>>> load that
>>>>>> can lead to null pointer dereference. The issue occurs when
>>>>>> suspend is
>>>>>> invoked before table load completes:
>>>>>>
>>>>>> BUG: kernel NULL pointer dereference, address: 0000000000000054
>>>>>> Oops: 0000 [#1] PREEMPT SMP PTI
>>>>>> CPU: 6 PID: 6798 Comm: dmsetup Not tainted 6.6.0-g7e52f5f0ca9b #62
>>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>>>>>> 1.16.1-2.fc37 04/01/2014
>>>>>> RIP: 0010:blk_mq_wait_quiesce_done+0x0/0x50
>>>>>> Call Trace:
>>>>>>      <TASK>
>>>>>>      blk_mq_quiesce_queue+0x2c/0x50
>>>>>>      dm_stop_queue+0xd/0x20
>>>>>>      __dm_suspend+0x130/0x330
>>>>>>      dm_suspend+0x11a/0x180
>>>>>>      dev_suspend+0x27e/0x560
>>>>>>      ctl_ioctl+0x4cf/0x850
>>>>>>      dm_ctl_ioctl+0xd/0x20
>>>>>>      vfs_ioctl+0x1d/0x50
>>>>>>      __se_sys_ioctl+0x9b/0xc0
>>>>>>      __x64_sys_ioctl+0x19/0x30
>>>>>>      x64_sys_call+0x2c4a/0x4620
>>>>>>      do_syscall_64+0x9e/0x1b0
>>>>>>
>>>>>> The issue can be triggered as below:
>>>>>>
>>>>>> T1                         T2
>>>>>> dm_suspend                    table_load
>>>>>> __dm_suspend                    dm_setup_md_queue
>>>>>>                         dm_mq_init_request_queue
>>>>>>                         blk_mq_init_allocated_queue
>>>>>>                         => q->mq_ops = set->ops; (1)
>>>>>> dm_stop_queue / dm_wait_for_completion
>>>>>> => q->tag_set NULL pointer!    (2)
>>>>>>                         => q->tag_set = set; (3)
>>>>>>
>>>>>> Fix this by checking if a valid table (map) exists before performing
>>>>>> request-based suspend and waiting for target I/O. When map is NULL,
>>>>>> skip these table-dependent suspend steps.
>>>>>>
>>>>>> Even when map is NULL, no I/O can reach any target because there is
>>>>>> no table loaded; I/O submitted in this state will fail early in the
>>>>>> DM layer. Skipping the table-dependent suspend logic in this case
>>>>>> is safe and avoids NULL pointer dereferences.
>>>>>>
>>>>>> Fixes: c4576aed8d85 ("dm: fix request-based dm's use of
>>>>>> dm_wait_for_completion")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
>>>>>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>>>>>> [ omitted DMF_QUEUE_STOPPED flag setting and braces absent in 5.15 ]
>>>>>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>>>>>> ---
>>>>>>     drivers/md/dm.c | 7 ++++---
>>>>>>     1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>>>>>> index 0868358a7a8d2..b51281ce15d4c 100644
>>>>>> --- a/drivers/md/dm.c
>>>>>> +++ b/drivers/md/dm.c
>>>>>> @@ -2457,7 +2457,7 @@ static int __dm_suspend(struct
>>>>>> mapped_device *md, struct dm_table *map,
>>>>>>     {
>>>>>>         bool do_lockfs = suspend_flags & DM_SUSPEND_LOCKFS_FLAG;
>>>>>>         bool noflush = suspend_flags & DM_SUSPEND_NOFLUSH_FLAG;
>>>>>> -    int r;
>>>>>> +    int r = 0;
>>>>>>         lockdep_assert_held(&md->suspend_lock);
>>>>>> @@ -2509,7 +2509,7 @@ static int __dm_suspend(struct
>>>>>> mapped_device *md, struct dm_table *map,
>>>>>>          * Stop md->queue before flushing md->wq in case
>>>>>> request-based
>>>>>>          * dm defers requests to md->wq from md->queue.
>>>>>>          */
>>>>>> -    if (dm_request_based(md))
>>>>>> +    if (map && dm_request_based(md))
>>>>>>             dm_stop_queue(md->queue);
>>>>> It seems that before commit 80bd4a7aab4c ("blk-mq: move the
>>>>> srcu_struct
>>>>> used for quiescing to the tagset") was merged,
>>>>> blk_mq_wait_quiesce_done()
>>>>> would not attempt to access q->tag_set, so in dm_stop_queue() this
>>>>> kind
>>>>> of race condition would not cause a null pointer dereference. The
>>>>> change
>>>>> may not be necessary for 5.10, but adding it doesn’t appear to
>>>>> cause any
>>>>> issues either.
>>>>>> flush_workqueue(md->wq);
>>>>>> @@ -2519,7 +2519,8 @@ static int __dm_suspend(struct
>>>>>> mapped_device *md, struct dm_table *map,
>>>>>>          * We call dm_wait_for_completion to wait for all
>>>>>> existing requests
>>>>>>          * to finish.
>>>>>>          */
>>>>>> -    r = dm_wait_for_completion(md, task_state);
>>>>>> +    if (map)
>>>>>> +        r = dm_wait_for_completion(md, task_state);
>>>>> The fix tag c4576aed8d85 ("dm: fix request-based dm's use of
>>>>> dm_wait_for_completion") seems to correspond to the modification of
>>>>> dm_wait_for_completion().
>>>>>>         if (!r)
>>>>>>             set_bit(dmf_suspended_flag, &md->flags);
>>>>> Perhaps adding another fix tag would be more appropriate?
>>>> Those tags come directly from the original commit.
>>> Thanks for the clarification. Since the patch has already been
>>> merged into
>>> the mainline tree, we can't update the commit there. I was just
>>> wondering
>>> if it would be acceptable to add an additional “Fixes” tag when
>>> applying
>>> it to the stable branch, before merging.
>> I can, if needed, can you please provide that line?
>
> Fixes: 80bd4a7aab4c ("blk-mq: move the srcu_struct used for quiescing
> to the tagset")
>
> Thanks,
> Lingfeng
>

Thank you for adding a fix tag.

Qixing

>>
>> thanks,
>>
>> greg k-h
>>