Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 4 Jul 2022 09:29:45 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable
Message-ID: <YsKW6VvWqvcMRBSl@kroah.com>
References: <000000000000d9ff3a05bb37069e@google.com>
 <72e74af9-f1b6-e383-a2c3-6ee8a0aea5e0@I-love.SAKURA.ne.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <72e74af9-f1b6-e383-a2c3-6ee8a0aea5e0@I-love.SAKURA.ne.jp>
Precedence: bulk

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@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  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