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

[Greg KH <gregkh@linuxfoundation.org>]

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