Re: [PATCH] char: misc: make misc_open() and misc_register() killable
From: Greg KH
Date: Fri Jul 08 2022 - 09:38:15 EST
On Thu, Jul 07, 2022 at 02:06:38PM +0900, Tetsuo Handa wrote:
> Let's summarize current location:
> (1) Greg wants me to fix snapshot_open() not to sleep for too long, instead of
> making misc_open() killable.
> (2) I found snapshot_open() calls wait_for_device_probe() which might sleep
> long enough to consider as hung up due to:
> (a) One of existing probe request got stuck due to unresponsive hardware.
> (b) New probe requests come in before existing probe requests complete.
> (3) Because of (2), it is difficult to guarantee snapshot_open() not to sleep for
> too long.
> (4) Because of (3), calling file->f_op->open(inode, file) with misc_mtx held can
> block mutex_lock(&misc_mtx) too long. This is the phenomenon syzbot is reporting.
> Initial mitigation was to replace mutex_lock(&misc_mtx) with mutex_lock_killable(&misc_mtx)
> so that /dev/raw-gadget users can terminate upon SIGKILL and khungtaskd will not complain
> about misc_mtx.
> Next mitigation was not to call file->f_op->open() with misc_mtx held.
> Wedson worried that this approach breaks modules which call misc_deregister(), but
> I think we can use this approach for modules which do not need to call misc_deregister()
> given that this approach is opt-in basis.
> I also think that we can bring wait_for_device_probe() in snapshot_open() to before
> lock_system_sleep(), for system_transition_mutex will not be required for waiting for
> the image device to appear. If we can accept the "not to call file->f_op->open() with
> misc_mtx held" mitigation, wait_for_device_probe() in snapshot_open() can be called
> without locks.
> Finding universally safe timeout value is beyond what I can do for this report.
> Regarding this report, I think we can lower the risk of regression if we apply
> timeout for atomic_read(&probe_count) == 0 from only snapshot_open().
> Can we make below changes?
> drivers/base/dd.c | 14 ++++++++++++++
> drivers/char/misc.c | 4 ++++
> include/linux/device/driver.h | 1 +
> include/linux/miscdevice.h | 1 +
> kernel/power/user.c | 31 ++++++++++++++++++-------------
> 5 files changed, 38 insertions(+), 13 deletions(-)
Can you make this a patch series, it's hard to tease out what the
different things are attempting to do here :(