Re: [PATCH v2] HID: hid-lenovo-go: cancel cfg_setup work in hid_go_cfg_remove()

["Günther Noack" <gnoack@google.com>]

Hello Derek and Mark!

May I please draw your attention to this issue in the hid-lenovo drivers,
which was pointed out by Sashiko:


On Fri, May 15, 2026 at 06:23:46PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [Critical] Hardware state corruption and crashes due to use of a static global struct for device data.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


> > diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> > index d4d26c7833563..b9d8cde531365 100644
> > --- a/drivers/hid/hid-lenovo-go.c
> > +++ b/drivers/hid/hid-lenovo-go.c
> > @@ -2408,6 +2408,15 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
> >  
> >  static void hid_go_cfg_remove(struct hid_device *hdev)
> >  {
> > +	/*
> > +	 * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay
> > +	 * and dereferences drvdata.hdev.  Drain it here before tearing
> > +	 * down so the workqueue cannot run after hid_destroy_device()'s
> > +	 * put_device() has released the underlying hdev and dereference
> > +	 * a stale drvdata.hdev pointer.
> > +	 */
> > +	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
> 
> This isn't a bug introduced by this patch, but does the driver use a global
> static variable (static struct hid_go_cfg drvdata) to store device state?
> 
> A second device probe would unconditionally overwrite drvdata.hdev and
> re-initialize both drvdata.cfg_mutex and drvdata.go_cfg_setup.
> 
> Re-initializing an active mutex destroys its wait lists, and re-initializing
> an active delayed work overwrites its linked list pointers, leading to
> immediate memory corruption and kernel panics.
> 
> Should the device state be dynamically allocated per-instance instead?


This is the same issue which was also introduced in hid-oxp, which I
pointed out in [1].  As the review-bot points out above, while the data in
your static drvdata variable is maybe only necessary once (but that
also seems questionable?), I think the mutex and workqueue shouldn't
just get re-initialized in the middle of being used, when a second device is
plugged in and the probe() hook runs again.

This pattern is used in both the drivers hid-lenovo-go.c, hid-lenovo-go-s.c,
as well as in the newer hid-oxp discussed in [1].

Shouldn't the drvdata just be allocated with one of the devm_*alloc() variants
as in the other drivers?

—Günther


[1] https://lore.kernel.org/all/akfGTjId84EjV141@google.com/