| 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
| ||
|
Date: Tue, 11 Feb 2020 15:20:49 +0000
From: Jérôme Pouiller <Jerome.Pouiller@...abs.com>
To: Michał Mirosław <mirq-linux@...e.qmqm.pl>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race
On Tuesday 11 February 2020 13:49:22 CET Michał Mirosław wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> > On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> > > Current code races in init/exit with interrupt handlers. This is noticed
> > > by the warning below. Fix it by using devres for ordering allocations and
> > > IRQ de/registration.
> > >
> > > WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> > > race condition in driver init/deinit
> [...]
> > > --- a/drivers/staging/wfx/bus_spi.c
> > > +++ b/drivers/staging/wfx/bus_spi.c
> > > @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> > > wfx_bh_request_rx(bus->core);
> > > }
> > >
> > > +static void wfx_flush_irq_work(void *w)
> > > +{
> > > + flush_work(w);
> > > +}
> > > +
> > > static size_t wfx_spi_align_size(void *priv, size_t size)
> > > {
> > > // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> > > @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> > > udelay(2000);
> > > }
> > >
> > > - ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > - IRQF_TRIGGER_RISING, "wfx", bus);
> > > - if (ret)
> > > - return ret;
> > > -
> > > INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> > > bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> > > &wfx_spi_hwbus_ops, bus);
> > > if (!bus->core)
> > > return -EIO;
> > >
> > > - ret = wfx_probe(bus->core);
> > > + ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> > > + &bus->request_rx);
> > > if (ret)
> > > - wfx_free_common(bus->core);
> > > + return ret;
> > >
> > > - return ret;
> > > + ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > + IRQF_TRIGGER_RISING, "wfx", bus);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return wfx_probe(bus->core);
> > > }
> > >
> > > static int wfx_spi_remove(struct spi_device *func)
> > > @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> > > struct wfx_spi_priv *bus = spi_get_drvdata(func);
> > >
> > > wfx_release(bus->core);
> > > - wfx_free_common(bus->core);
> > > - // A few IRQ will be sent during device release. Hopefully, no IRQ
> > > - // should happen after wdev/wvif are released.
> > > - devm_free_irq(&func->dev, func->irq, bus);
> > > - flush_work(&bus->request_rx);
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > index 84adad64fc30..76b2ff7fc7fe 100644
> > > --- a/drivers/staging/wfx/main.c
> > > +++ b/drivers/staging/wfx/main.c
> > > @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > > return ret;
> > > }
> > >
> > > +static void wfx_free_common(void *data)
> > > +{
> > > + struct wfx_dev *wdev = data;
> > > +
> > > + mutex_destroy(&wdev->rx_stats_lock);
> > > + mutex_destroy(&wdev->conf_mutex);
> > > + wfx_tx_queues_deinit(wdev);
> > > + ieee80211_free_hw(wdev->hw);
> > > +}
> > > +
> > > struct wfx_dev *wfx_init_common(struct device *dev,
> > > const struct wfx_platform_data *pdata,
> > > const struct hwbus_ops *hwbus_ops,
> > > @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > > wfx_init_hif_cmd(&wdev->hif_cmd);
> > > wfx_tx_queues_init(wdev);
> > >
> > > + if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> > > + return NULL;
> > > +
> > > return wdev;
> > > }
> > >
> > > -void wfx_free_common(struct wfx_dev *wdev)
> > > -{
> > > - mutex_destroy(&wdev->rx_stats_lock);
> > > - mutex_destroy(&wdev->conf_mutex);
> > > - wfx_tx_queues_deinit(wdev);
> > > - ieee80211_free_hw(wdev->hw);
> > > -}
> > > -
> > > int wfx_probe(struct wfx_dev *wdev)
> > > {
> > > int i;
> > > diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> > > index 875f8c227803..9c9410072def 100644
> > > --- a/drivers/staging/wfx/main.h
> > > +++ b/drivers/staging/wfx/main.h
> > > @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > > const struct wfx_platform_data *pdata,
> > > const struct hwbus_ops *hwbus_ops,
> > > void *hwbus_priv);
> > > -void wfx_free_common(struct wfx_dev *wdev);
> > >
> > > int wfx_probe(struct wfx_dev *wdev);
> > > void wfx_release(struct wfx_dev *wdev);
> > > --
> > > 2.20.1
> > >
> >
> > Are you sure you can completely drop wfx_free_common()? If you want to
> > use devres, I think that wfx_flush_irq_work() should call
> > wfx_free_common(), no?
>
> wfx_free_common() will be called by devres in the right order. That's ensured
> by devm_add_action_or_reset() at the end of wfx_init_common().
Indeed.
Reviewed-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
--
Jérôme Pouiller
Powered by blists - more mailing lists