Messages in this thread | | | Date | Fri, 8 Jul 2022 15:37:33 +0200 | From | Greg KH <> | Subject | Re: [PATCH] char: misc: make misc_open() and misc_register() killable |
| |
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
| |