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

[Greg KH]

On Mon, Jul 04, 2022 at 03:44:07PM +0900, Tetsuo Handa wrote:
> syzbot is reporting hung task at misc_open() [1], for snapshot_open() from
> misc_open() might sleep for long with misc_mtx held whereas userspace can
> flood with concurrent misc_open() requests. Mitigate this problem by making
> misc_open() and misc_register() killable.

I do not understand, why not just fix snapshot_open()?  Why add this
complexity to the misc core for a foolish individual misc device?  Why
not add the fix there where it is spinning instead?

> Link: https://syzkaller.appspot.com/bug?extid=358c9ab4c93da7b7238c [1]
> Reported-by: syzbot <syzbot+358c9ab4c93da7b7238c@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/char/misc.c | 57 +++++++++++++++++++--------------------------
>  1 file changed, 24 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index cba19bfdc44d..b9a494bc4228 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -100,49 +100,39 @@ static const struct seq_operations misc_seq_ops = {
>  static int misc_open(struct inode *inode, struct file *file)
>  {
>  	int minor = iminor(inode);
> -	struct miscdevice *c = NULL, *iter;
> +	struct miscdevice *iter;
>  	int err = -ENODEV;
>  	const struct file_operations *new_fops = NULL;
> +	bool retried = false;
>  
> -	mutex_lock(&misc_mtx);
> -
> + retry:
> +	if (mutex_lock_killable(&misc_mtx))
> +		return -EINTR;

I really don't want to add this type of thing to the misc core if at all
possible.  Again, please just fix up the misbehaving misc driver
instead, don't penalize the core with thie complexity.

thanks,

greg k-h