Re: [PATCH v2 00/33] HID: logitech-hidpp: G920 force-feedback fixes, rework, and cleanups

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Gideon,

On Thu, Nov 12, 2026 at 02:30:39AM +0100, Gideon Heinz wrote:
> Hi Dmitry,
>
> I tested v2 of the series on my G920 (046d:c262). Everything on my
> list works except one scenario that takes the whole machine down, with
> and without your series (last item).
...
>  - rmmod with fftest holding the device open: rmmod returned after
>    2.99 s with "Unloading HID++ force feedback" and nothing else in
>    dmesg. When I then exited fftest (entering -1), the machine went
>    down: the first time with a garbled panic screen that I couldn't
>    read, and on later attempts as a total hang with no output. The
>    same sequence on stock Arch 7.2.11-arch1-1 with the unpatched
>    driver also hangs the whole system, so this predates the series.
>
>    To reproduce: with the module loaded and the G920 plugged in, start
>    fftest on its event node and leave it at the effect prompt, run
>    "rmmod hid_logitech_hidpp" from another terminal, then enter -1 in
>    fftest.
>
>    I could not recover a trace yet and will send one if I manage to.
>
>    My guess, from reading the code and not verified by a trace:
>    hidpp_ff_create() stores hidpp_ff_destroy() and the other FF
>    callbacks in the ff_device as pointers into the module. With fftest
>    holding the evdev open, the input_dev outlives the driver, so when
>    fftest closes, the final input_dev release calls ff->destroy in
>    module code that rmmod has already freed.

Thank you very much for the thorough testing!

Your diagnosis is spot on. Opening /dev/input/eventX pins the evdev cdev
(and through it input_dev), but does not hold a module reference on the
underlying HID driver. When rmmod unloads hid_logitech_hidpp while fftest
still has the file descriptor open, __input_unregister_device() disconnects
evdev (flushing effects, closing the device, and marking evdev dead so all
subsequent ioctls and writes return -ENODEV) and runs ff->stop(), after
which rmmod frees the module text. When fftest finally exits and closes the
file descriptor, input_dev_release() calls input_ff_destroy(), which
invokes ff->destroy() pointing into the already-unmapped module text.

Rather than working around this in individual drivers with a separate
ff->stop() callback, input core should call input_ff_destroy() directly in
__input_unregister_device() once all handlers are disconnected and the
device is removed from sysfs. At that point nothing touches dev->ff
anymore, and calling input_ff_destroy() there guarantees ff->destroy()
always runs while the driver and its module are still loaded (while the
second input_ff_destroy() call in input_dev_release() becomes a no-op once
dev->ff is NULL).

Could you try the input core patch below (with patch 22/33 "HID:
logitech-hidpp: shut down G920 force feedback on input unregister" dropped
from the hid-logitech-hidpp series, since ff->stop is removed and
hidpp_ff_destroy() will now run on input unregistration)?

Alternatively, if you prefer to test with your out-of-tree module build
without rebuilding the kernel, you can test the equivalent effect by
setting:

	ff->destroy = NULL;

inside hidpp_ff_stop() in hid-logitech-hidpp.c.

Thanks.

---
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: [PATCH] Input: destroy force-feedback device on input unregistration

When an input device is unregistered while userspace holds an open file
descriptor to its event node, input_dev_release() is deferred until the
last file descriptor is closed. Because input_ff_destroy() was only
called from input_dev_release(), force-feedback teardown (ff->destroy
and freeing ff->private) could run after the parent driver had already
unbound, freed its device state, or even been unloaded via rmmod,
causing use-after-free errors or page faults in unmapped module text.

Commit ef166ce08801 ("Input: stop force-feedback timer when
unregistering input devices") added a separate ff->stop callback for
ff-memless, but left ff->destroy invoked from input_dev_release().

Once __input_unregister_device() has disconnected all input handlers
(which flushes effects, closes the device, and marks evdev dead) and
deleted the device from sysfs, nothing accesses dev->ff anymore. Call
input_ff_destroy() directly in __input_unregister_device() so that
ff->destroy() always runs synchronously while the driver and its module
are still active, and remove the now-redundant ff->stop callback. Keep
input_ff_destroy() in input_dev_release() (where it is a no-op once
dev->ff is NULL) to handle input_free_device() on devices that were
never registered.

Fixes: 509ca1a93836 ("Input: implement new force feedback interface")
Cc: stable@vger.kernel.org
Reported-by: Gideon Heinz <gideonheinz02@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/ff-core.c    |  2 +-
 drivers/input/ff-memless.c | 23 -----------------------
 drivers/input/input.c      |  5 ++---
 include/linux/input.h      |  3 ---
 4 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 0e5d1d1ceb46..133b4622dc49 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(input_ff_create);
  *
  * This function is only needed in error path as input core will
  * automatically free force feedback structures when device is
- * destroyed.
+ * unregistered or destroyed.
  */
 void input_ff_destroy(struct input_dev *dev)
 {
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index d1fefd1dfc0d..3069436148ec 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -483,32 +483,10 @@ static void ml_ff_destroy(struct ff_device *ff)
 {
 	struct ml_device *ml = ff->private;
 
-	/*
-	 * The timer is normally shut down in ml_ff_stop() when the device
-	 * is unregistered. However, we still shut it down here as a safety
-	 * net and for cases where the device was never registered (e.g.
-	 * error paths during probe).
-	 */
 	timer_shutdown_sync(&ml->timer);
-
 	kfree(ml->private);
 }
 
-static void ml_ff_stop(struct ff_device *ff)
-{
-	struct ml_device *ml = ff->private;
-
-	/*
-	 * Even though we stop all playing effects when tearing down an
-	 * input device (by the way of evdev calling input_flush_device()
-	 * that calls into input_ff_flush() that stops and erases all
-	 * effects), we do not actually shutdown the timer, and therefore
-	 * we should do it here to prevent it firing after the input
-	 * device is unregistered and its associated resources are freed.
-	 */
-	timer_shutdown_sync(&ml->timer);
-}
-
 /**
  * input_ff_create_memless() - create memoryless force-feedback device
  * @dev: input device supporting force-feedback
@@ -543,7 +521,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
 	ff->playback = ml_ff_playback;
 	ff->set_gain = ml_ff_set_gain;
 	ff->destroy = ml_ff_destroy;
-	ff->stop = ml_ff_stop;
 
 	/* we can emulate periodic effects with RUMBLE */
 	if (test_bit(FF_RUMBLE, ff->ffbit)) {
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 210635ef147e..7be69deed8eb 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2286,10 +2286,9 @@ static void __input_unregister_device(struct input_dev *dev)
 		input_wakeup_procfs_readers();
 	}
 
-	if (dev->ff && dev->ff->stop)
-		dev->ff->stop(dev->ff);
-
 	device_del(&dev->dev);
+
+	input_ff_destroy(dev);
 }
 
 static void devm_input_device_unregister(struct device *dev, void *res)
diff --git a/include/linux/input.h b/include/linux/input.h
index 3381608127f7..02047cc45d04 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -549,8 +549,6 @@ extern const struct class input_class;
  * @set_autocenter: Called to auto-center device
  * @destroy: called by input core when parent input device is being
  *	destroyed
- * @stop: called by input core when parent input device is being
- *	unregistered
  * @private: driver-specific data, will be freed automatically
  * @ffbit: bitmap of force feedback capabilities truly supported by
  *	device (not emulated like ones in input_dev->ffbit)
@@ -579,7 +577,6 @@ struct ff_device {
 	void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
 
 	void (*destroy)(struct ff_device *);
-	void (*stop)(struct ff_device *);
 
 	void *private;
 

-- 
Dmitry