linux-kernel - Re: [PATCH] Revert "module, async: async_synchronize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Feb 2022 07:43:27 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Igor Pylypiv <ipylypiv@...gle.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
        Changyuan Lyu <changyuanl@...gle.com>
Subject: Re: [PATCH] Revert "module, async: async_synchronize_full() on
 module init iff async is used"

On Thu, Jan 27, 2022 at 03:39:53PM -0800, Igor Pylypiv wrote:
> This reverts commit 774a1221e862b343388347bac9b318767336b20b.
> 
> We need to finish all async code before the module init sequence is done.
> In the reverted commit the PF_USED_ASYNC flag was added to mark a thread
> that called async_schedule(). Then the PF_USED_ASYNC flag was used to
> determine whether or not async_synchronize_full() needs to be invoked.
> This works when modprobe thread is calling async_schedule(), but it
> does not work if module dispatches init code to a worker thread which
> then calls async_schedule().
> 
> For example, PCI driver probing is invoked from a worker thread based on
> a node where device is attached:
> 
> 	if (cpu < nr_cpu_ids)
> 		error = work_on_cpu(cpu, local_pci_probe, &ddi);
> 	else
> 		error = local_pci_probe(&ddi);
> 
> We end up in a situation where a worker thread gets the PF_USED_ASYNC flag
> set instead of the modprobe thread. As a result, async_synchronize_full()
> is not invoked and modprobe completes without waiting for the async code
> to finish.
> 
> The issue was discovered while loading the pm80xx driver:
> (scsi_mod.scan=async)
> 
> modprobe pm80xx                      worker
> ...
>   do_init_module()
>   ...
>     pci_call_probe()
>       work_on_cpu(local_pci_probe)
>                                      local_pci_probe()
>                                        pm8001_pci_probe()
>                                          scsi_scan_host()
>                                            async_schedule()
>                                            worker->flags |= PF_USED_ASYNC;
>                                      ...
>       < return from worker >
>   ...
>   if (current->flags & PF_USED_ASYNC) <--- false
>   	async_synchronize_full();
> 
> Commit 21c3c5d28007 ("block: don't request module during elevator init")
> fixed the deadlock issue which the reverted commit 774a1221e862 ("module,
> async: async_synchronize_full() on module init iff async is used") tried
> to fix.
> 
> Since commit 0fdff3ec6d87 ("async, kmod: warn on synchronous
> request_module() from async workers") synchronous module loading
> from async is not allowed.
> 
> Given that the original deadlock issue is fixed and it is no longer allowed
> to call synchronous request_module() from async we can remove PF_USED_ASYNC
> flag to make module init consistently invoke async_synchronize_full()
> unless async module probe is requested.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@...gle.com>
> Reviewed-by: Changyuan Lyu <changyuanl@...gle.com>

That's quite a walk down the memory lane and I agree with your analysis. The
PF_USED_ASYNC is redundant for correctness with the removal of synchrnous
loading from iosched path and the WARN_ON guarantees that nothing in kernel
is creating a similar situation.

Acked-by: Tejun Heo <tj@...nel.org>

Thanks.

-- 
tejun