From: Greg KH <gregkh@linuxfoundation.org>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Sean Young <sean@mess.org>,
Alan Stern <stern@rowland.harvard.edu>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Jarod Wilson <jarod@redhat.com>,
syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>,
andreyknvl@google.com, linux-media@vger.kernel.org,
linux-usb@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH v2 (resend)] media: imon: reorganize serialization
Date: Mon, 2 May 2022 14:53:22 +0200 [thread overview]
Message-ID: <Ym/UQpEV46W6frqH@kroah.com> (raw)
In-Reply-To: <21ffa07a-1bc1-cb1f-eef4-6c3a73953061@I-love.SAKURA.ne.jp>
On Mon, May 02, 2022 at 12:49:04PM +0900, Tetsuo Handa wrote:
> Since usb_register_dev() from imon_init_display() from imon_probe() holds
> minor_rwsem while display_open() which holds driver_lock and ictx->lock is
> called with minor_rwsem held from usb_open(), holding driver_lock or
> ictx->lock when calling usb_register_dev() causes circular locking
> dependency problem.
>
> Since usb_deregister_dev() from imon_disconnect() holds minor_rwsem while
> display_open() which holds driver_lock is called with minor_rwsem held,
> holding driver_lock when calling usb_deregister_dev() also causes circular
> locking dependency problem.
>
> Sean Young explained that the problem is there are imon devices which have
> two usb interfaces, even though it is one device. The probe and disconnect
> function of both usb interfaces can run concurrently.
>
> Alan Stern responded that the driver and USB cores guarantee that when an
> interface is probed, both the interface and its USB device are locked.
> Ditto for when the disconnect callback gets run. So concurrent probing/
> disconnection of multiple interfaces on the same device is not possible.
>
> Therefore, we don't need locks for handling race between imon_probe() and
> imon_disconnect(). But we still need to handle race between display_open()
> /vfd_write()/lcd_write()/display_close() and imon_disconnect(), for
> disconnect event can happen while file descriptors are in use.
>
> Since "struct file"->private_data is set by display_open(), vfd_write()/
> lcd_write()/display_close() can assume that "struct file"->private_data
> is not NULL even after usb_set_intfdata(interface, NULL) was called.
>
> Replace insufficiently held driver_lock with refcount_t based management.
> Add a boolean flag for recording whether imon_disconnect() was already
> called. Use RCU for accessing this boolean flag and refcount_t.
>
> Since the boolean flag for imon_disconnect() is shared, disconnect event
> on either intf0 or intf1 affects both interfaces. But I assume that this
> change does not matter, for usually disconnect event would not happen
> while interfaces are in use.
>
> Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497
> Reported-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+c558267ad910fc494497@syzkaller.appspotmail.com>
> Cc: Sean Young <sean@mess.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
> Changes in v2:
> Defer free_imon_context() using refcount till display_close() is called.
>
> drivers/media/rc/imon.c | 99 +++++++++++++++++++----------------------
> 1 file changed, 47 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 54da6f60079b..9a4f24e294bc 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -153,6 +153,24 @@ struct imon_context {
> const struct imon_usb_dev_descr *dev_descr;
> /* device description with key */
> /* table for front panels */
> + /*
> + * Fields for deferring free_imon_context().
> + *
> + * Since reference to "struct imon_context" is stored into
> + * "struct file_operations"->private_data, we need to remember
> + * how many file descriptors might access this "struct imon_context".
> + */
> + refcount_t users;
Are you sure this is going to work properly?
How do you handle userspace passing around file descriptors to other
processes?
You really should not ever have to count this.
> + /*
> + * Use a flag for telling display_open()/vfd_write()/lcd_write() that
> + * imon_disconnect() was already called.
> + */
> + bool disconnected;
> + /*
> + * We need to wait for RCU grace period in order to allow
> + * display_open() to safely check ->disconnected and increment ->users.
> + */
> + struct rcu_head rcu;
> };
>
> #define TOUCH_TIMEOUT (HZ/30)
> @@ -160,18 +178,18 @@ struct imon_context {
> /* vfd character device file operations */
> static const struct file_operations vfd_fops = {
> .owner = THIS_MODULE,
> - .open = &display_open,
> - .write = &vfd_write,
> - .release = &display_close,
> + .open = display_open,
> + .write = vfd_write,
> + .release = display_close,
> .llseek = noop_llseek,
> };
>
> /* lcd character device file operations */
> static const struct file_operations lcd_fops = {
> .owner = THIS_MODULE,
> - .open = &display_open,
> - .write = &lcd_write,
> - .release = &display_close,
> + .open = display_open,
> + .write = lcd_write,
> + .release = display_close,
> .llseek = noop_llseek,
> };
>
> @@ -439,9 +457,6 @@ static struct usb_driver imon_driver = {
> .id_table = imon_usb_id_table,
> };
>
> -/* to prevent races between open() and disconnect(), probing, etc */
> -static DEFINE_MUTEX(driver_lock);
> -
> /* Module bookkeeping bits */
> MODULE_AUTHOR(MOD_AUTHOR);
> MODULE_DESCRIPTION(MOD_DESC);
> @@ -481,9 +496,11 @@ static void free_imon_context(struct imon_context *ictx)
> struct device *dev = ictx->dev;
>
> usb_free_urb(ictx->tx_urb);
> + WARN_ON(ictx->dev_present_intf0);
> usb_free_urb(ictx->rx_urb_intf0);
> + WARN_ON(ictx->dev_present_intf1);
> usb_free_urb(ictx->rx_urb_intf1);
> - kfree(ictx);
> + kfree_rcu(ictx, rcu);
>
> dev_dbg(dev, "%s: iMON context freed\n", __func__);
> }
> @@ -499,9 +516,6 @@ static int display_open(struct inode *inode, struct file *file)
> int subminor;
> int retval = 0;
>
> - /* prevent races with disconnect */
> - mutex_lock(&driver_lock);
> -
> subminor = iminor(inode);
> interface = usb_find_interface(&imon_driver, subminor);
> if (!interface) {
> @@ -509,13 +523,16 @@ static int display_open(struct inode *inode, struct file *file)
> retval = -ENODEV;
> goto exit;
> }
> - ictx = usb_get_intfdata(interface);
>
> - if (!ictx) {
> + rcu_read_lock();
> + ictx = usb_get_intfdata(interface);
> + if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
> + rcu_read_unlock();
> pr_err("no context found for minor %d\n", subminor);
> retval = -ENODEV;
> goto exit;
> }
> + rcu_read_unlock();
>
> mutex_lock(&ictx->lock);
>
> @@ -533,8 +550,10 @@ static int display_open(struct inode *inode, struct file *file)
>
> mutex_unlock(&ictx->lock);
>
> + if (retval && refcount_dec_and_test(&ictx->users))
> + free_imon_context(ictx);
> +
> exit:
> - mutex_unlock(&driver_lock);
> return retval;
> }
>
> @@ -544,16 +563,9 @@ static int display_open(struct inode *inode, struct file *file)
> */
> static int display_close(struct inode *inode, struct file *file)
> {
> - struct imon_context *ictx = NULL;
> + struct imon_context *ictx = file->private_data;
> int retval = 0;
>
> - ictx = file->private_data;
> -
> - if (!ictx) {
> - pr_err("no context for device\n");
> - return -ENODEV;
> - }
> -
> mutex_lock(&ictx->lock);
>
> if (!ictx->display_supported) {
> @@ -568,6 +580,8 @@ static int display_close(struct inode *inode, struct file *file)
> }
>
> mutex_unlock(&ictx->lock);
> + if (refcount_dec_and_test(&ictx->users))
> + free_imon_context(ictx);
Why not just put a kref into your larger structure?
I think trying to count users of open/close is never going to work, just
allow the normal open/close logic to work properly and track your data
structure based on reference counts like it should be doing already.
thanks,
greg k-h
next prev parent reply other threads:[~2022-05-02 12:53 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-09 13:18 possible deadlock in display_open syzbot
2022-04-25 5:29 ` Tetsuo Handa
2022-04-25 9:20 ` Sean Young
2022-04-25 11:14 ` Tetsuo Handa
2022-04-25 11:56 ` Sean Young
2022-04-25 16:01 ` Alan Stern
2022-04-25 16:21 ` Sean Young
2022-04-26 0:54 ` [PATCH] media: imon: remove redundant serialization Tetsuo Handa
2022-04-26 7:57 ` Sean Young
2022-04-26 10:32 ` [PATCH v2] media: imon: reorganize serialization Tetsuo Handa
2022-05-02 3:49 ` [PATCH v2 (resend)] " Tetsuo Handa
2022-05-02 9:39 ` Sean Young
2022-05-02 10:26 ` Tetsuo Handa
2022-05-02 10:40 ` Tetsuo Handa
2022-05-02 10:46 ` Oliver Neukum
2022-05-02 11:05 ` Tetsuo Handa
2022-05-03 7:41 ` Oliver Neukum
2022-05-02 12:53 ` Greg KH [this message]
2022-05-02 13:04 ` Tetsuo Handa
2022-05-02 13:06 ` Greg KH
2022-05-02 13:40 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ym/UQpEV46W6frqH@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=andreyknvl@google.com \
--cc=jarod@redhat.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=sean@mess.org \
--cc=stern@rowland.harvard.edu \
--cc=syzbot+c558267ad910fc494497@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).