linux-kernel - RE: [PATCH 06/10] mei: vsc: Event notifier fixes

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY5PR11MB6366D541BDE4C76FC24AE18CED7BA@CY5PR11MB6366.namprd11.prod.outlook.com>
Date: Wed, 25 Jun 2025 09:36:12 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: Hans de Goede <hansg@...nel.org>, Sakari Ailus
	<sakari.ailus@...ux.intel.com>, Stanislaw Gruszka
	<stanislaw.gruszka@...ux.intel.com>
CC: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 06/10] mei: vsc: Event notifier fixes

> Subject: Re: [PATCH 06/10] mei: vsc: Event notifier fixes
> 
> On 25-Jun-25 11:23 AM, Hans de Goede wrote:
> > Hi,
> >
> > On 25-Jun-25 11:12 AM, Usyskin, Alexander wrote:
> >>> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
> >>>
> >>> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
> >>> to protect against this.
> >>>
> >>> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> >>> Signed-off-by: Hans de Goede <hansg@...nel.org>
> >>> ---
> >>>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
> >>>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> >>> index 0feebffabdb3..76a6aa606a26 100644
> >>> --- a/drivers/misc/mei/vsc-tp.c
> >>> +++ b/drivers/misc/mei/vsc-tp.c
> >>> @@ -79,9 +79,8 @@ struct vsc_tp {
> >>>
> >>>  	vsc_tp_event_cb_t event_notify;
> >>>  	void *event_notify_context;
> >>> -
> >>> -	/* used to protect command download */
> >>> -	struct mutex mutex;
> >>> +	struct mutex event_notify_mutex;	/* protects event_notify +
> >>> context */
> >>> +	struct mutex mutex;			/* protects command
> >>> download */
> >>>  };
> >>>
> >>>  /* GPIO resources */
> >>> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> >>> *data)
> >>>  {
> >>>  	struct vsc_tp *tp = data;
> >>>
> >>
> >> The mutex overhead looks out of place here in the interrupt handler.
> >> Maybe it can be replaced with something lighter?
> >
> > Using mutexes in *threaded* isr handlers is quite normal, e.g.
> > both the SPI core (used as transport here) and the I2C cor will
> > take + release a mutex for each data transfer over the bus and
> > a threaded ISR handler may do more then 1 data transfer for a single
> > interrupt.
> >
> > As to using something lighter I could not come up with any lighter
> > solution then this.
> 
> p.s.
> 
> I forgot to mention that this interrupt also does not trigger
> that frequently that we really need to worry about overhead.
> 
> The VSC sits between the user-facing camera and the Intel
> CPU/SoC's CSI2 receiver. So we only need to talk to it
> (generating interrupts) on probe() and when starting/stopping
> streaming video from the camera. Each start/stop we'll get
> a bunch of interrupts but outside of that the interrupt never
> triggers. So overhead is not really a big worry here.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> > Also note that this is moved to a workqueue later in the series,
> > since the threaded ISR actually waits for the wake_up() call
> > done by the hard part of the ISR and an ISR waiting for
> > the interrupt to trigger a second/third/... time inside the ISR
> > handler is just plain wrong.
> >
> >> BTW is it possible to have interrupt before call to vsc_tp_register_event_c
> >
> > The interrupt gets triggered by a GPIO connected to the VSC,
> > so if the VSC is well behaved then the interrupt should not
> > trigger. But we cannot really count on that.
> >
> > Regards,
> >
> > Hans
> >

Look like better be safe then sorry here, let's add mutex.

Reviewed-by: Alexander Usyskin <alexander.usyskin@...el.com>

> >
> >
> >>> +	guard(mutex)(&tp->event_notify_mutex);
> >>> +
> >>>  	if (tp->event_notify)
> >>>  		tp->event_notify(tp->event_notify_context);
> >>>
> >>> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
> >>> "VSC_TP");
> >>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t
> event_cb,
> >>>  			    void *context)
> >>>  {
> >>> +	guard(mutex)(&tp->event_notify_mutex);
> >>> +
> >>>  	tp->event_notify = event_cb;
> >>>  	tp->event_notify_context = context;
> >>>
> >>> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> >>>  		return ret;
> >>>
> >>>  	mutex_init(&tp->mutex);
> >>> +	mutex_init(&tp->event_notify_mutex);
> >>>
> >>>  	/* only one child acpi device */
> >>>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
> >>> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
> >>>  err_destroy_lock:
> >>>  	free_irq(spi->irq, tp);
> >>>
> >>> +	mutex_destroy(&tp->event_notify_mutex);
> >>>  	mutex_destroy(&tp->mutex);
> >>>
> >>>  	return ret;
> >>> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
> >>>
> >>>  	free_irq(spi->irq, tp);
> >>>
> >>> +	mutex_destroy(&tp->event_notify_mutex);
> >>>  	mutex_destroy(&tp->mutex);
> >>>  }
> >>>
> >>> --
> >>> 2.49.0
> >>
> >