Re: [PATCH] char: misc: make misc_open() and misc_register() killable

Greg KH
2022-07-08T13:38:15Z

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 :(

thanks,

greg k-h