linux-kernel - Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YrBGFZwLENLigWMV@hovoldconsulting.com>
Date:   Mon, 20 Jun 2022 12:04:05 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Frank Zago <frank@...o.net>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Wolfram Sang <wsa@...nel.org>, linux-usb@...r.kernel.org,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the
 CH341

On Wed, Jun 15, 2022 at 08:29:31PM -0500, Frank Zago wrote:
> On 5/23/22 11:16, Johan Hovold wrote:

> >> +static void ch341_gpio_irq_enable(struct irq_data *data)
> >> +{
> >> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> >> +	int rc;
> >> +
> >> +	/*
> >> +	 * The URB might have just been unlinked in
> >> +	 * ch341_gpio_irq_disable, but the completion handler hasn't
> >> +	 * been called yet.
> >> +	 */
> >> +	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
> >> +		usb_kill_anchored_urbs(&dev->irq_urb_out);
> >> +
> >> +	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
> >> +	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> >> +	if (rc)
> >> +		usb_unanchor_urb(dev->irq_urb);
> > 
> > This looks confused and broken.
> > 
> > usb_kill_anchored_urbs() can sleep so either calling it is broken or
> > using GFP_ATOMIC is unnecessary.
> 
> Right, that function can sleep. I changed GFP_ATOMIC to GFP_KERNEL.

These callbacks can be called in atomic context so that's not an option,
I'm afraid.

> > And isn't this function called multiple times when enabling more than
> > one irq?!
> 
> There's only one IRQ, so only one URB will be posted at a time. It
> is reposted as soon as it comes back unless the IRQ is disabled or
> the device stops.

AFAICT you have up to 16 (CH341_GPIO_NUM_PINS) interrupts, not one. So I
still say this is broken.

> >> +}
> >> +
> >> +static void ch341_gpio_irq_disable(struct irq_data *data)
> >> +{
> >> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> >> +
> >> +	usb_unlink_urb(dev->irq_urb);
> > 
> > Same here...

> >> +}
> >> +
> >> +static int ch341_gpio_remove(struct platform_device *pdev)
> >> +{
> >> +	struct ch341_gpio *dev = platform_get_drvdata(pdev);
> >> +
> >> +	usb_kill_anchored_urbs(&dev->irq_urb_out);
> > 
> > You only have one URB...
> > 
> > And what prevents it from being resubmitted here?
> 
> I don't see what would resubmit it here. The gpio is being released.

Your implementation needs to handle racing requests. The gpio chip is
still registered here.

Johan