Re: [REGRESSION] disabling and re-enabling magic sysrq fails after kernel 6.11

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

Hi everyone,

On Sun, Oct 27, 2024 at 10:02:24AM -0700, Ned T. Crigler wrote:
> Hi Peter, Christian,
> 
> On Sun, Oct 27, 2024 at 04:37:44PM +0100, Peter Seiderer wrote:
> > Hello Ned, Christian, *,
> > 
> > On Sun, 27 Oct 2024 15:06:09 +0100, Christian Heusel <christian@heusel.eu> wrote:
> > 
> > > On 24/10/26 07:15PM, Ned T. Crigler wrote:
> > > > Hi,
> > >
> > > Hey Ned,
> > >
> > > > It looks like starting with kernel 6.11, disabling and re-enabling
> > > > magic
> > > > sysrq fails with these errors in dmesg:
> > > >
> > > > kernel: input: input_handler_check_methods: only one event processing
> > > > method can be defined (sysrq)
> > > > kernel: sysrq: Failed to register input handler, error -22
> > > >
> > > > after doing:
> > > >
> > > > # echo 0 > /proc/sys/kernel/sysrq
> > > > # echo 438 > /proc/sys/kernel/sysrq
> > > > # echo 0 > /proc/sys/kernel/sysrq
> > > > # echo 438 > /proc/sys/kernel/sysrq
> > > > # echo 0 > /proc/sys/kernel/sysrq
> > > > # echo 438 > /proc/sys/kernel/sysrq
> > >
> > > I have found that this issue is also present in the latest mainline
> > > release and bisected it to the following commit:
> > >
> > >     d469647bafd9 ("Input: simplify event handling logic")
> > >
> > 
> > After the mentioned commit a call sysrq_register_handler() -->
> > input_register_handler(&sysrq_handler) with sysrq_handler.filter set
> > will result in sysrq_handler.events set to input_handler_events_filter,
> > see drivers/input/input.c (line 2607 to 2608):
> > 
> > 2596 int input_register_handler(struct input_handler *handler)
> > 2597 {
> > 2598         struct input_dev *dev;
> > 2599         int error;
> > 2600
> > 2601         error = input_handler_check_methods(handler);
> > 2602         if (error)
> > 2603                 return error;
> > 2604
> > 2605         INIT_LIST_HEAD(&handler->h_list);
> > 2606
> > 2607         if (handler->filter)
> > 2608                 handler->events = input_handler_events_filter;
> > 2609         else if (handler->event)
> > 2610                 handler->events = input_handler_events_default;
> > 2611         else if (!handler->events)
> > 2612                 handler->events = input_handler_events_null;
> > 
> > So the second call will fail at the check 'input_handler_check_methods(handler)'
> > which only allows one method to be set, see drivers/input/input.c:
> > 
> > 2517 static int input_handler_check_methods(const struct input_handler *handler)
> > 2518 {
> > 2519         int count = 0;
> > 2520
> > 2521         if (handler->filter)
> > 2522                 count++;
> > 2523         if (handler->events)
> > 2524                 count++;
> > 2525         if (handler->event)
> > 2526                 count++;
> > 2527
> > 2528         if (count > 1) {
> > 2529                 pr_err("%s: only one event processing method can be defined      (%s)\n",
> > 2530                        __func__, handler->name);
> > 2531                 return -EINVAL;
> > 2532         }
> > 2533
> > 2534         return 0;
> > 2535 }

Yes, I did not consider that we might want to re-register the same input
handler, thank you for alerting me about the regression.

> > 
> > 
> > A quick fix/hack for the sysrq case:
> > 
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -1045,7 +1045,7 @@ static inline void sysrq_register_handler(void)
> >         int error;
> > 
> >         sysrq_of_get_keyreset_config();
> > -
> > +       sysrq_handler.events = NULL;
> >         error = input_register_handler(&sysrq_handler);
> >         if (error)
> >                 pr_err("Failed to register input handler, error %d", error);
> > lines 1-13/13 (END)
> > 
> > Regards,
> > Peter
> > 
> 
> Thanks for tracking this down. It seems messy that the mentioned commit
> changes input_register_handler to overwrite ->events for an internal purpose,
> and callers may expect it to be unchanged, as sysrq does here by reusing
> sysrq_handler.

Yes, indeed. I wonder if we can solve this by moving the derived event
handler method into input_handle structure, like the patch below. 

Thanks.

-- 
Dmitry


diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3c321671793f..3d2cc13e1f32 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -119,12 +119,12 @@ static void input_pass_values(struct input_dev *dev,
 
 	handle = rcu_dereference(dev->grab);
 	if (handle) {
-		count = handle->handler->events(handle, vals, count);
+		count = handle->handle_events(handle, vals, count);
 	} else {
 		list_for_each_entry_rcu(handle, &dev->h_list, d_node)
 			if (handle->open) {
-				count = handle->handler->events(handle, vals,
-								count);
+				count = handle->handle_events(handle, vals,
+							      count);
 				if (!count)
 					break;
 			}
@@ -2537,57 +2537,6 @@ static int input_handler_check_methods(const struct input_handler *handler)
 	return 0;
 }
 
-/*
- * An implementation of input_handler's events() method that simply
- * invokes handler->event() method for each event one by one.
- */
-static unsigned int input_handler_events_default(struct input_handle *handle,
-						 struct input_value *vals,
-						 unsigned int count)
-{
-	struct input_handler *handler = handle->handler;
-	struct input_value *v;
-
-	for (v = vals; v != vals + count; v++)
-		handler->event(handle, v->type, v->code, v->value);
-
-	return count;
-}
-
-/*
- * An implementation of input_handler's events() method that invokes
- * handler->filter() method for each event one by one and removes events
- * that were filtered out from the "vals" array.
- */
-static unsigned int input_handler_events_filter(struct input_handle *handle,
-						struct input_value *vals,
-						unsigned int count)
-{
-	struct input_handler *handler = handle->handler;
-	struct input_value *end = vals;
-	struct input_value *v;
-
-	for (v = vals; v != vals + count; v++) {
-		if (handler->filter(handle, v->type, v->code, v->value))
-			continue;
-		if (end != v)
-			*end = *v;
-		end++;
-	}
-
-	return end - vals;
-}
-
-/*
- * An implementation of input_handler's events() method that does nothing.
- */
-static unsigned int input_handler_events_null(struct input_handle *handle,
-					      struct input_value *vals,
-					      unsigned int count)
-{
-	return count;
-}
-
 /**
  * input_register_handler - register a new input handler
  * @handler: handler to be registered
@@ -2607,13 +2556,6 @@ int input_register_handler(struct input_handler *handler)
 
 	INIT_LIST_HEAD(&handler->h_list);
 
-	if (handler->filter)
-		handler->events = input_handler_events_filter;
-	else if (handler->event)
-		handler->events = input_handler_events_default;
-	else if (!handler->events)
-		handler->events = input_handler_events_null;
-
 	error = mutex_lock_interruptible(&input_mutex);
 	if (error)
 		return error;
@@ -2687,6 +2629,75 @@ int input_handler_for_each_handle(struct input_handler *handler, void *data,
 }
 EXPORT_SYMBOL(input_handler_for_each_handle);
 
+/*
+ * An implementation of input_handle's handle_events() method that simply
+ * invokes handler->event() method for each event one by one.
+ */
+static unsigned int input_handle_events_default(struct input_handle *handle,
+						struct input_value *vals,
+						unsigned int count)
+{
+	struct input_handler *handler = handle->handler;
+	struct input_value *v;
+
+	for (v = vals; v != vals + count; v++)
+		handler->event(handle, v->type, v->code, v->value);
+
+	return count;
+}
+
+/*
+ * An implementation of input_handle's handle_events() method that invokes
+ * handler->filter() method for each event one by one and removes events
+ * that were filtered out from the "vals" array.
+ */
+static unsigned int input_handle_events_filter(struct input_handle *handle,
+					       struct input_value *vals,
+					       unsigned int count)
+{
+	struct input_handler *handler = handle->handler;
+	struct input_value *end = vals;
+	struct input_value *v;
+
+	for (v = vals; v != vals + count; v++) {
+		if (handler->filter(handle, v->type, v->code, v->value))
+			continue;
+		if (end != v)
+			*end = *v;
+		end++;
+	}
+
+	return end - vals;
+}
+
+/*
+ * An implementation of input_handle's handle_events() method that does nothing.
+ */
+static unsigned int input_handle_events_null(struct input_handle *handle,
+					     struct input_value *vals,
+					     unsigned int count)
+{
+	return count;
+}
+
+/*
+ * Sets up appropriate handle->event_handler based on the input_handler
+ * associated with the handle.
+ */
+static void input_handle_setup_event_handler(struct input_handle *handle)
+{
+	struct input_handler *handler = handle->handler;
+
+	if (handler->filter)
+		handle->handle_events = input_handle_events_filter;
+	else if (handler->event)
+		handle->handle_events = input_handle_events_default;
+	else if (handler->events)
+		handle->handle_events = handler->events;
+	else
+		handle->handle_events = input_handle_events_null;
+}
+
 /**
  * input_register_handle - register a new input handle
  * @handle: handle to register
@@ -2704,6 +2715,7 @@ int input_register_handle(struct input_handle *handle)
 	struct input_dev *dev = handle->dev;
 	int error;
 
+	input_handle_setup_event_handler(handle);
 	/*
 	 * We take dev->mutex here to prevent race with
 	 * input_release_device().
diff --git a/include/linux/input.h b/include/linux/input.h
index 89a0be6ee0e2..cd866b020a01 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -339,12 +339,16 @@ struct input_handler {
  * @name: name given to the handle by handler that created it
  * @dev: input device the handle is attached to
  * @handler: handler that works with the device through this handle
+ * @handle_events: event sequence handler. It is set up by the input core
+ *	according to event handling method specified in the @handler. See
+ *	input_handle_setup_event_handler().
+ *	This method is being called by the input core with interrupts disabled
+ *	and dev->event_lock spinlock held and so it may not sleep.
  * @d_node: used to put the handle on device's list of attached handles
  * @h_node: used to put the handle on handler's list of handles from which
  *	it gets events
  */
 struct input_handle {
-
 	void *private;
 
 	int open;
@@ -353,6 +357,10 @@ struct input_handle {
 	struct input_dev *dev;
 	struct input_handler *handler;
 
+	unsigned int (*handle_events)(struct input_handle *handle,
+				      struct input_value *vals,
+				      unsigned int count);
+
 	struct list_head	d_node;
 	struct list_head	h_node;
 };