| Message ID | 20171214132522.20346-3-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Benjamin Tissoires, on jeu. 14 déc. 2017 14:25:22 +0100, wrote: > Before unregistering the led classes, we have to be sure there is no > more events in the input pipeline. > Closing the input node before removing the led classes flushes the > pipeline and this prevents segfaults. > > Found with https://github.com/whot/fuzzydevice > Link: https://bugzilla.kernel.org/show_bug.cgi?id=197679 > > Cc: stable@vger.kernel.org > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> input_close_device does run synchronize_rcu() which we seem to have to process before freeing the rest indeed. Thus, Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org> (though AFAIK it doesn't apply on the mainline tree) > --- > drivers/input/input-leds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c > index c86eb3d648bf..8aefcc186a02 100644 > --- a/drivers/input/input-leds.c > +++ b/drivers/input/input-leds.c > @@ -211,6 +211,7 @@ static void input_leds_disconnect(struct input_handle *handle) > int i; > > cancel_delayed_work_sync(&leds->init_work); > + input_close_device(handle); > > for (i = 0; i < leds->num_leds; i++) { > struct input_led *led = &leds->leds[i]; > @@ -219,7 +220,6 @@ static void input_leds_disconnect(struct input_handle *handle) > kfree(led->cdev.name); > } > > - input_close_device(handle); > input_unregister_handle(handle); > > kfree(leds); > -- > 2.14.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote: > Before unregistering the led classes, we have to be sure there is no > more events in the input pipeline. > Closing the input node before removing the led classes flushes the > pipeline and this prevents segfaults. I do not think this actually the issue. input_leds_event() is an empty stub, it does not really do anything with events it receives form input core, and it does not reference the led structures. The way input leds work is that input driver owns the hardware led and is responsible for communicating with it. The LED subsystem is a secondary interface, requests coming from /sys/class/led/... are being forwarded to the input core and then to the input hardware driver by the way of: input_leds_brightness_set() -> input_inject_event() -> input_handle_event()-> disposition & INPUT_PASS_TO_DEVICE dev->event() (in atkbd or hid-input) actual control of LED I do not see how stopping event flow from input core to input-leds would help here. Thanks.
On Sat, Dec 16, 2017 at 1:48 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Benjamin, > > On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote: >> Before unregistering the led classes, we have to be sure there is no >> more events in the input pipeline. >> Closing the input node before removing the led classes flushes the >> pipeline and this prevents segfaults. > > I do not think this actually the issue. input_leds_event() is an empty > stub, it does not really do anything with events it receives form input > core, and it does not reference the led structures. The way input leds Right. Actually, we could simply drop the stub as input_to_handler() checks for the validity of .event(). > work is that input driver owns the hardware led and is responsible for > communicating with it. The LED subsystem is a secondary interface, > requests coming from /sys/class/led/... are being forwarded to the input > core and then to the input hardware driver by the way of: > > input_leds_brightness_set() -> > input_inject_event() -> > input_handle_event()-> > disposition & INPUT_PASS_TO_DEVICE > dev->event() (in atkbd or hid-input) > actual control of LED > > I do not see how stopping event flow from input core to input-leds would > help here. I wonder if this solution in this patch is not just masking the race condition by forcing the sync. After further researches, I realized that the patch is actually not needed. In the upstream repo of Peter, the code inject events and closes the device ASAP, triggering races with udev. Adding the proper udev stubs seem to solve the issues. I still have other random crashes, but I can't seem to reproduce the error of https://bugzilla.kernel.org/show_bug.cgi?id=197679 now. Anyway, I can't explain the backtrace of the kernel bug, it is as if we are calling the unregister function without having properly registered the device. But this can not happen because of the mutexes in place. The race seems to be udev and probably the led class accesses... BTW, if the handler doesn't use the events coming in from the input subsystem, is there any benefits of opening the device from the handler? I tried without, and it seemed to be working fine, but I wonder if there is no hidden dependency I haven't see. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 20, 2017 at 9:11 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Sat, Dec 16, 2017 at 1:48 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> Hi Benjamin, >> >> On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote: >>> Before unregistering the led classes, we have to be sure there is no >>> more events in the input pipeline. >>> Closing the input node before removing the led classes flushes the >>> pipeline and this prevents segfaults. >> >> I do not think this actually the issue. input_leds_event() is an empty >> stub, it does not really do anything with events it receives form input >> core, and it does not reference the led structures. The way input leds > > Right. Actually, we could simply drop the stub as input_to_handler() > checks for the validity of .event(). > >> work is that input driver owns the hardware led and is responsible for >> communicating with it. The LED subsystem is a secondary interface, >> requests coming from /sys/class/led/... are being forwarded to the input >> core and then to the input hardware driver by the way of: >> >> input_leds_brightness_set() -> >> input_inject_event() -> >> input_handle_event()-> >> disposition & INPUT_PASS_TO_DEVICE >> dev->event() (in atkbd or hid-input) >> actual control of LED >> >> I do not see how stopping event flow from input core to input-leds would >> help here. > > I wonder if this solution in this patch is not just masking the race > condition by forcing the sync. > > After further researches, I realized that the patch is actually not > needed. In the upstream repo of Peter, the code inject events and > closes the device ASAP, triggering races with udev. > Adding the proper udev stubs seem to solve the issues. > I still have other random crashes, but I can't seem to reproduce the > error of https://bugzilla.kernel.org/show_bug.cgi?id=197679 now. > > Anyway, I can't explain the backtrace of the kernel bug, it is as if > we are calling the unregister function without having properly > registered the device. But this can not happen because of the mutexes > in place. > The race seems to be udev and probably the led class accesses... I wonder if the crash was observed only with your first patch adding the delay in initializing the leds? > > BTW, if the handler doesn't use the events coming in from the input > subsystem, is there any benefits of opening the device from the > handler? > I tried without, and it seemed to be working fine, but I wonder if > there is no hidden dependency I haven't see. You want to open the device as it is what essentially powers it up, so that it can react to input_inject_event() sent via LED subsystem. In case of atkbd input_open_device() will result in call to serio_open() which enables the KBD port of i8042. You probably do not see any difference because the VT subsystem already attached the keyboard handler to the device and it already "opened" the device in question. Thanks.
On Wed, Dec 20, 2017 at 7:20 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Dec 20, 2017 at 9:11 AM, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: >> On Sat, Dec 16, 2017 at 1:48 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >>> Hi Benjamin, >>> >>> On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote: >>>> Before unregistering the led classes, we have to be sure there is no >>>> more events in the input pipeline. >>>> Closing the input node before removing the led classes flushes the >>>> pipeline and this prevents segfaults. >>> >>> I do not think this actually the issue. input_leds_event() is an empty >>> stub, it does not really do anything with events it receives form input >>> core, and it does not reference the led structures. The way input leds >> >> Right. Actually, we could simply drop the stub as input_to_handler() >> checks for the validity of .event(). >> >>> work is that input driver owns the hardware led and is responsible for >>> communicating with it. The LED subsystem is a secondary interface, >>> requests coming from /sys/class/led/... are being forwarded to the input >>> core and then to the input hardware driver by the way of: >>> >>> input_leds_brightness_set() -> >>> input_inject_event() -> >>> input_handle_event()-> >>> disposition & INPUT_PASS_TO_DEVICE >>> dev->event() (in atkbd or hid-input) >>> actual control of LED >>> >>> I do not see how stopping event flow from input core to input-leds would >>> help here. >> >> I wonder if this solution in this patch is not just masking the race >> condition by forcing the sync. >> >> After further researches, I realized that the patch is actually not >> needed. In the upstream repo of Peter, the code inject events and >> closes the device ASAP, triggering races with udev. >> Adding the proper udev stubs seem to solve the issues. >> I still have other random crashes, but I can't seem to reproduce the >> error of https://bugzilla.kernel.org/show_bug.cgi?id=197679 now. >> >> Anyway, I can't explain the backtrace of the kernel bug, it is as if >> we are calling the unregister function without having properly >> registered the device. But this can not happen because of the mutexes >> in place. >> The race seems to be udev and probably the led class accesses... > > I wonder if the crash was observed only with your first patch adding > the delay in initializing the leds? Nah, the bug was initially found by Peter on a plain Fedora system without my crap :) > >> >> BTW, if the handler doesn't use the events coming in from the input >> subsystem, is there any benefits of opening the device from the >> handler? >> I tried without, and it seemed to be working fine, but I wonder if >> there is no hidden dependency I haven't see. > > You want to open the device as it is what essentially powers it up, so > that it can react to input_inject_event() sent via LED subsystem. In > case of atkbd input_open_device() will result in call to serio_open() > which enables the KBD port of i8042. You probably do not see any > difference because the VT subsystem already attached the keyboard > handler to the device and it already "opened" the device in question. > Right. So I guess there is not much to do then :) Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c index c86eb3d648bf..8aefcc186a02 100644 --- a/drivers/input/input-leds.c +++ b/drivers/input/input-leds.c @@ -211,6 +211,7 @@ static void input_leds_disconnect(struct input_handle *handle) int i; cancel_delayed_work_sync(&leds->init_work); + input_close_device(handle); for (i = 0; i < leds->num_leds; i++) { struct input_led *led = &leds->leds[i]; @@ -219,7 +220,6 @@ static void input_leds_disconnect(struct input_handle *handle) kfree(led->cdev.name); } - input_close_device(handle); input_unregister_handle(handle); kfree(leds);
Before unregistering the led classes, we have to be sure there is no more events in the input pipeline. Closing the input node before removing the led classes flushes the pipeline and this prevents segfaults. Found with https://github.com/whot/fuzzydevice Link: https://bugzilla.kernel.org/show_bug.cgi?id=197679 Cc: stable@vger.kernel.org Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/input/input-leds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)