linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Input: tests - miscellaneous fixes
@ 2023-05-02 10:17 Geert Uytterhoeven
  2023-05-02 10:17 ` [PATCH 1/2] Input: tests - fix use-after-free and refcount underflow in input_test_exit() Geert Uytterhoeven
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-02 10:17 UTC (permalink / raw)
  To: Dmitry Torokhov, Javier Martinez Canillas, Brendan Higgins,
	David Gow
  Cc: linux-input, linux-kselftest, kunit-dev, linux-kernel,
	Geert Uytterhoeven

	Hi all,

This patch series fixes a crash in the new input selftest, and makes the
test available when the KUnit framework is modular.

Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
and ARAnyM):

        KTAP version 1
        # Subtest: input_core
        1..3
    input: Test input device as /devices/virtual/input/input1
        ok 1 input_test_polling
    input: Test input device as /devices/virtual/input/input2
        ok 2 input_test_timestamp
    input: Test input device as /devices/virtual/input/input3
        # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
        Expected input_match_device_id(input_dev, &id) to be true, but is false
        not ok 3 input_test_match_device_id
    # input_core: pass:2 fail:1 skip:0 total:3
    # Totals: pass:2 fail:1 skip:0 total:3
    not ok 1 input_core

Thanks!

Geert Uytterhoeven (2):
  Input: tests - fix use-after-free and refcount underflow in
    input_test_exit()
  Input: tests - modular KUnit tests should not depend on KUNIT=y

 drivers/input/Kconfig            | 2 +-
 drivers/input/tests/input_test.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] Input: tests - fix use-after-free and refcount underflow in input_test_exit()
  2023-05-02 10:17 [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
@ 2023-05-02 10:17 ` Geert Uytterhoeven
  2023-05-02 10:17 ` [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y Geert Uytterhoeven
  2023-05-02 16:09 ` [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
  2 siblings, 0 replies; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-02 10:17 UTC (permalink / raw)
  To: Dmitry Torokhov, Javier Martinez Canillas, Brendan Higgins,
	David Gow
  Cc: linux-input, linux-kselftest, kunit-dev, linux-kernel,
	Geert Uytterhoeven

With CONFIG_DEBUG_SLAB=y:

        # Subtest: input_core
        1..3
    input: Test input device as /devices/virtual/input/input1
    8<--- cut here ---
    Unable to handle kernel paging request at virtual address 6b6b6dd7 when read
    ...
     __lock_acquire from lock_acquire+0x26c/0x300
     lock_acquire from _raw_spin_lock_irqsave+0x50/0x64
     _raw_spin_lock_irqsave from devres_remove+0x20/0x7c
     devres_remove from devres_destroy+0x8/0x24
     devres_destroy from input_free_device+0x2c/0x60
     input_free_device from kunit_try_run_case+0x70/0x94 [kunit]

Without CONFIG_DEBUG_SLAB=y:

	KTAP version 1
	# Subtest: input_core
	1..3
    input: Test input device as /devices/virtual/input/input1
    ------------[ cut here ]------------
    WARNING: CPU: 0 PID: 694 at lib/refcount.c:28 refcount_warn_saturate+0x54/0x100
    refcount_t: underflow; use-after-free.
    ...
    Call Trace: [<0037cad4>] dump_stack+0xc/0x10
     [<00377614>] __warn+0x7e/0xb4
     [<0037768c>] warn_slowpath_fmt+0x42/0x62
     [<001eee1c>] refcount_warn_saturate+0x54/0x100
     [<000b1d34>] kfree_const+0x0/0x20
     [<0036290a>] __kobject_del+0x0/0x6e
     [<001eee1c>] refcount_warn_saturate+0x54/0x100
     [<00362a1a>] kobject_put+0xa2/0xb6
     [<11965770>] kunit_generic_run_threadfn_adapter+0x0/0x1c [kunit]

As per the comments for input_allocate_device() and
input_register_device(), input_free_device() must be called only to free
devices that have not been registered.  input_unregister_device()
already calls input_put_device(), thus leading to a use-after-free.

Moreover, the kunit_suite.exit() method is called after every test case,
even on failures.  As the test itself already does cleanups in its
failure paths, this may lead to a second use-after-free.

Fix the first issue by dropping the call to input_allocate_device() from
input_test_exit().
Fix the second issue by making the cleanup code conditional on a
successful test.

Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/input/tests/input_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c
index e5a6c1ad2167c103..8b8ac3412a70d3b4 100644
--- a/drivers/input/tests/input_test.c
+++ b/drivers/input/tests/input_test.c
@@ -43,8 +43,8 @@ static void input_test_exit(struct kunit *test)
 {
 	struct input_dev *input_dev = test->priv;
 
-	input_unregister_device(input_dev);
-	input_free_device(input_dev);
+	if (input_dev)
+		input_unregister_device(input_dev);
 }
 
 static void input_test_poll(struct input_dev *input) { }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y
  2023-05-02 10:17 [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
  2023-05-02 10:17 ` [PATCH 1/2] Input: tests - fix use-after-free and refcount underflow in input_test_exit() Geert Uytterhoeven
@ 2023-05-02 10:17 ` Geert Uytterhoeven
  2023-05-02 11:19   ` Javier Martinez Canillas
  2023-05-04  6:29   ` David Gow
  2023-05-02 16:09 ` [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
  2 siblings, 2 replies; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-02 10:17 UTC (permalink / raw)
  To: Dmitry Torokhov, Javier Martinez Canillas, Brendan Higgins,
	David Gow
  Cc: linux-input, linux-kselftest, kunit-dev, linux-kernel,
	Geert Uytterhoeven

While KUnit tests that cannot be built as a loadable module must depend
on "KUNIT=y", this is not true for modular tests, where it adds an
unnecessary limitation.

Fix this by relaxing the dependency to "KUNIT".

Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/input/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index 735f90b74ee5ad44..3bdbd34314b3495a 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -168,7 +168,7 @@ config INPUT_EVBUG
 
 config INPUT_KUNIT_TEST
 	tristate "KUnit tests for Input" if !KUNIT_ALL_TESTS
-	depends on INPUT && KUNIT=y
+	depends on INPUT && KUNIT
 	default KUNIT_ALL_TESTS
 	help
 	  Say Y here if you want to build the KUnit tests for the input
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y
  2023-05-02 10:17 ` [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y Geert Uytterhoeven
@ 2023-05-02 11:19   ` Javier Martinez Canillas
  2023-05-02 11:22     ` Geert Uytterhoeven
  2023-05-04  6:29   ` David Gow
  1 sibling, 1 reply; 12+ messages in thread
From: Javier Martinez Canillas @ 2023-05-02 11:19 UTC (permalink / raw)
  To: Geert Uytterhoeven, Dmitry Torokhov, Brendan Higgins, David Gow
  Cc: linux-input, linux-kselftest, kunit-dev, linux-kernel,
	Geert Uytterhoeven


Hello Geert,

I've only been Cc'ed in patch #2.

Geert Uytterhoeven <geert+renesas@glider.be> writes:

> While KUnit tests that cannot be built as a loadable module must depend
> on "KUNIT=y", this is not true for modular tests, where it adds an
> unnecessary limitation.
>
> Fix this by relaxing the dependency to "KUNIT".
>
> Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y
  2023-05-02 11:19   ` Javier Martinez Canillas
@ 2023-05-02 11:22     ` Geert Uytterhoeven
  2023-05-02 11:34       ` Javier Martinez Canillas
  0 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-02 11:22 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Dmitry Torokhov, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

Hi Javier,

On Tue, May 2, 2023 at 1:19 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> I've only been Cc'ed in patch #2.

Not really, you're in the To-header on the full series?
https://lore.kernel.org/all/cover.1683022164.git.geert+renesas@glider.be

> Geert Uytterhoeven <geert+renesas@glider.be> writes:
> > While KUnit tests that cannot be built as a loadable module must depend
> > on "KUNIT=y", this is not true for modular tests, where it adds an
> > unnecessary limitation.
> >
> > Fix this by relaxing the dependency to "KUNIT".
> >
> > Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y
  2023-05-02 11:22     ` Geert Uytterhoeven
@ 2023-05-02 11:34       ` Javier Martinez Canillas
  0 siblings, 0 replies; 12+ messages in thread
From: Javier Martinez Canillas @ 2023-05-02 11:34 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Dmitry Torokhov, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,
>
> On Tue, May 2, 2023 at 1:19 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> I've only been Cc'ed in patch #2.
>
> Not really, you're in the To-header on the full series?
> https://lore.kernel.org/all/cover.1683022164.git.geert+renesas@glider.be
>

Strange... I only got patch #2, neither patch #1 nor the cover letter.

For patch #1 as well:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Sorry for missing that bug and thanks a lot for fixing it!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Input: tests - miscellaneous fixes
  2023-05-02 10:17 [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
  2023-05-02 10:17 ` [PATCH 1/2] Input: tests - fix use-after-free and refcount underflow in input_test_exit() Geert Uytterhoeven
  2023-05-02 10:17 ` [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y Geert Uytterhoeven
@ 2023-05-02 16:09 ` Geert Uytterhoeven
  2023-05-02 16:31   ` Javier Martinez Canillas
  2 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-02 16:09 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Dmitry Torokhov, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

Hi Javier,

On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> This patch series fixes a crash in the new input selftest, and makes the
> test available when the KUnit framework is modular.
>
> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
> and ARAnyM):
>
>         KTAP version 1
>         # Subtest: input_core
>         1..3
>     input: Test input device as /devices/virtual/input/input1
>         ok 1 input_test_polling
>     input: Test input device as /devices/virtual/input/input2
>         ok 2 input_test_timestamp
>     input: Test input device as /devices/virtual/input/input3
>         # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
>         Expected input_match_device_id(input_dev, &id) to be true, but is false
>         not ok 3 input_test_match_device_id
>     # input_core: pass:2 fail:1 skip:0 total:3
>     # Totals: pass:2 fail:1 skip:0 total:3
>     not ok 1 input_core

Adding more debug code shows that it's the test on evbit [1] in
input_match_device_id() that fails.
Looking at your input_test_match_device_id(), I think you expect
the checks for the various bitmaps to be gated by
"if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the
other checks?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Input: tests - miscellaneous fixes
  2023-05-02 16:09 ` [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
@ 2023-05-02 16:31   ` Javier Martinez Canillas
  2023-05-02 17:05     ` Dmitry Torokhov
  0 siblings, 1 reply; 12+ messages in thread
From: Javier Martinez Canillas @ 2023-05-02 16:31 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Dmitry Torokhov, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> This patch series fixes a crash in the new input selftest, and makes the
>> test available when the KUnit framework is modular.
>>
>> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
>> and ARAnyM):
>>
>>         KTAP version 1
>>         # Subtest: input_core
>>         1..3
>>     input: Test input device as /devices/virtual/input/input1
>>         ok 1 input_test_polling
>>     input: Test input device as /devices/virtual/input/input2
>>         ok 2 input_test_timestamp
>>     input: Test input device as /devices/virtual/input/input3
>>         # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
>>         Expected input_match_device_id(input_dev, &id) to be true, but is false
>>         not ok 3 input_test_match_device_id
>>     # input_core: pass:2 fail:1 skip:0 total:3
>>     # Totals: pass:2 fail:1 skip:0 total:3
>>     not ok 1 input_core
>
> Adding more debug code shows that it's the test on evbit [1] in
> input_match_device_id() that fails.
> Looking at your input_test_match_device_id(), I think you expect
> the checks for the various bitmaps to be gated by
> "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the
> other checks?
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021
>

That's correct. In input_test_init(), the input dev is marked as capable
of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to
check this.

That is, check if matches by the input dev capabilities in which case the
__set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..)
would make the condition false.

But maybe I misunderstood how the input_set_capability() and __set_bit()
functions work ?

I'll take a look to this tomorrow, thanks a lot for your report!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Input: tests - miscellaneous fixes
  2023-05-02 16:31   ` Javier Martinez Canillas
@ 2023-05-02 17:05     ` Dmitry Torokhov
  2023-05-03  6:53       ` Geert Uytterhoeven
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2023-05-02 17:05 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Geert Uytterhoeven, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> 
> Hello Geert,
> 
> > Hi Javier,
> >
> > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> >> This patch series fixes a crash in the new input selftest, and makes the
> >> test available when the KUnit framework is modular.
> >>
> >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
> >> and ARAnyM):
> >>
> >>         KTAP version 1
> >>         # Subtest: input_core
> >>         1..3
> >>     input: Test input device as /devices/virtual/input/input1
> >>         ok 1 input_test_polling
> >>     input: Test input device as /devices/virtual/input/input2
> >>         ok 2 input_test_timestamp
> >>     input: Test input device as /devices/virtual/input/input3
> >>         # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
> >>         Expected input_match_device_id(input_dev, &id) to be true, but is false
> >>         not ok 3 input_test_match_device_id
> >>     # input_core: pass:2 fail:1 skip:0 total:3
> >>     # Totals: pass:2 fail:1 skip:0 total:3
> >>     not ok 1 input_core
> >
> > Adding more debug code shows that it's the test on evbit [1] in
> > input_match_device_id() that fails.
> > Looking at your input_test_match_device_id(), I think you expect
> > the checks for the various bitmaps to be gated by
> > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the
> > other checks?
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021
> >
> 
> That's correct. In input_test_init(), the input dev is marked as capable
> of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to
> check this.
> 
> That is, check if matches by the input dev capabilities in which case the
> __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..)
> would make the condition false.
> 
> But maybe I misunderstood how the input_set_capability() and __set_bit()
> functions work ?
> 
> I'll take a look to this tomorrow, thanks a lot for your report!

Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect,
the kernel always used bitmaps when matching (with the assumption that
if one does not care about given bitmap they can simply pass empty one),
so I think what we need to change is:

diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c
index 8b8ac3412a70..0540225f0288 100644
--- a/drivers/input/tests/input_test.c
+++ b/drivers/input/tests/input_test.c
@@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test)
 static void input_test_match_device_id(struct kunit *test)
 {
 	struct input_dev *input_dev = test->priv;
-	struct input_device_id id;
+	struct input_device_id id = { 0 };
 
 	/*
 	 * Must match when the input device bus, vendor, product, version

to avoid having garbage in the match data.

I suppose we should remove INPUT_DEVICE_ID_MATCH_*BIT from
mod_devicetable.h to avoid this confusion.

Thanks.

-- 
Dmitry

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Input: tests - miscellaneous fixes
  2023-05-02 17:05     ` Dmitry Torokhov
@ 2023-05-03  6:53       ` Geert Uytterhoeven
  2023-05-03  7:05         ` Javier Martinez Canillas
  0 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-05-03  6:53 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Javier Martinez Canillas, Brendan Higgins, David Gow, linux-input,
	linux-kselftest, kunit-dev, linux-kernel

Hi Dmitry,

On Tue, May 2, 2023 at 7:05 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > >> This patch series fixes a crash in the new input selftest, and makes the
> > >> test available when the KUnit framework is modular.
> > >>
> > >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
> > >> and ARAnyM):
> > >>
> > >>         KTAP version 1
> > >>         # Subtest: input_core
> > >>         1..3
> > >>     input: Test input device as /devices/virtual/input/input1
> > >>         ok 1 input_test_polling
> > >>     input: Test input device as /devices/virtual/input/input2
> > >>         ok 2 input_test_timestamp
> > >>     input: Test input device as /devices/virtual/input/input3
> > >>         # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
> > >>         Expected input_match_device_id(input_dev, &id) to be true, but is false
> > >>         not ok 3 input_test_match_device_id
> > >>     # input_core: pass:2 fail:1 skip:0 total:3
> > >>     # Totals: pass:2 fail:1 skip:0 total:3
> > >>     not ok 1 input_core
> > >
> > > Adding more debug code shows that it's the test on evbit [1] in
> > > input_match_device_id() that fails.
> > > Looking at your input_test_match_device_id(), I think you expect
> > > the checks for the various bitmaps to be gated by
> > > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the
> > > other checks?
> > >
> > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021
> > >
> >
> > That's correct. In input_test_init(), the input dev is marked as capable
> > of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to
> > check this.
> >
> > That is, check if matches by the input dev capabilities in which case the
> > __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..)
> > would make the condition false.
> >
> > But maybe I misunderstood how the input_set_capability() and __set_bit()
> > functions work ?
> >
> > I'll take a look to this tomorrow, thanks a lot for your report!
>
> Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect,
> the kernel always used bitmaps when matching (with the assumption that
> if one does not care about given bitmap they can simply pass empty one),
> so I think what we need to change is:
>
> diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c
> index 8b8ac3412a70..0540225f0288 100644
> --- a/drivers/input/tests/input_test.c
> +++ b/drivers/input/tests/input_test.c
> @@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test)
>  static void input_test_match_device_id(struct kunit *test)
>  {
>         struct input_dev *input_dev = test->priv;
> -       struct input_device_id id;
> +       struct input_device_id id = { 0 };
>
>         /*
>          * Must match when the input device bus, vendor, product, version
>
> to avoid having garbage in the match data.

Thanks, that did the trick! 3/3 tests pass.


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Input: tests - miscellaneous fixes
  2023-05-03  6:53       ` Geert Uytterhoeven
@ 2023-05-03  7:05         ` Javier Martinez Canillas
  0 siblings, 0 replies; 12+ messages in thread
From: Javier Martinez Canillas @ 2023-05-03  7:05 UTC (permalink / raw)
  To: Geert Uytterhoeven, Dmitry Torokhov
  Cc: Brendan Higgins, David Gow, linux-input, linux-kselftest,
	kunit-dev, linux-kernel

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Dmitry,
>
> On Tue, May 2, 2023 at 7:05 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Tue, May 02, 2023 at 06:31:51PM +0200, Javier Martinez Canillas wrote:
>> > Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > > On Tue, May 2, 2023 at 12:17 PM Geert Uytterhoeven
>> > > <geert+renesas@glider.be> wrote:
>> > >> This patch series fixes a crash in the new input selftest, and makes the
>> > >> test available when the KUnit framework is modular.
>> > >>
>> > >> Unfortunately test 3 still fails for me (tested on Koelsch (R-Car M2-W)
>> > >> and ARAnyM):
>> > >>
>> > >>         KTAP version 1
>> > >>         # Subtest: input_core
>> > >>         1..3
>> > >>     input: Test input device as /devices/virtual/input/input1
>> > >>         ok 1 input_test_polling
>> > >>     input: Test input device as /devices/virtual/input/input2
>> > >>         ok 2 input_test_timestamp
>> > >>     input: Test input device as /devices/virtual/input/input3
>> > >>         # input_test_match_device_id: ASSERTION FAILED at # drivers/input/tests/input_test.c:99
>> > >>         Expected input_match_device_id(input_dev, &id) to be true, but is false
>> > >>         not ok 3 input_test_match_device_id
>> > >>     # input_core: pass:2 fail:1 skip:0 total:3
>> > >>     # Totals: pass:2 fail:1 skip:0 total:3
>> > >>     not ok 1 input_core
>> > >
>> > > Adding more debug code shows that it's the test on evbit [1] in
>> > > input_match_device_id() that fails.
>> > > Looking at your input_test_match_device_id(), I think you expect
>> > > the checks for the various bitmaps to be gated by
>> > > "if (id->flags & INPUT_DEVICE_ID_MATCH_EVBIT)", like is done for the
>> > > other checks?
>> > >
>> > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1021
>> > >
>> >
>> > That's correct. In input_test_init(), the input dev is marked as capable
>> > of emitting EV_KEY BTN_LEFT and BTN_RIGHT. The goal of that test was to
>> > check this.
>> >
>> > That is, check if matches by the input dev capabilities in which case the
>> > __set_bit(EV_KEY, ...) would make the match true and __set_bit(EV_ABS, ..)
>> > would make the condition false.
>> >
>> > But maybe I misunderstood how the input_set_capability() and __set_bit()
>> > functions work ?
>> >
>> > I'll take a look to this tomorrow, thanks a lot for your report!
>>
>> Unfortunately (?) INPUT_DEVICE_ID_MATCH_*BIT have never had any effect,
>> the kernel always used bitmaps when matching (with the assumption that
>> if one does not care about given bitmap they can simply pass empty one),
>> so I think what we need to change is:
>>
>> diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c
>> index 8b8ac3412a70..0540225f0288 100644
>> --- a/drivers/input/tests/input_test.c
>> +++ b/drivers/input/tests/input_test.c
>> @@ -87,7 +87,7 @@ static void input_test_timestamp(struct kunit *test)
>>  static void input_test_match_device_id(struct kunit *test)
>>  {
>>         struct input_dev *input_dev = test->priv;
>> -       struct input_device_id id;
>> +       struct input_device_id id = { 0 };
>>
>>         /*
>>          * Must match when the input device bus, vendor, product, version
>>
>> to avoid having garbage in the match data.
>
> Thanks, that did the trick! 3/3 tests pass.
>

Oh, silly me. Are you going to post that as a proper patch ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y
  2023-05-02 10:17 ` [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y Geert Uytterhoeven
  2023-05-02 11:19   ` Javier Martinez Canillas
@ 2023-05-04  6:29   ` David Gow
  1 sibling, 0 replies; 12+ messages in thread
From: David Gow @ 2023-05-04  6:29 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Dmitry Torokhov, Javier Martinez Canillas, Brendan Higgins,
	linux-input, linux-kselftest, kunit-dev, linux-kernel

On Tue, 2 May 2023 at 18:17, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>
> While KUnit tests that cannot be built as a loadable module must depend
> on "KUNIT=y", this is not true for modular tests, where it adds an
> unnecessary limitation.
>
> Fix this by relaxing the dependency to "KUNIT".
>
> Fixes: fdefcbdd6f361841 ("Input: Add KUnit tests for some of the input core helper functions")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---

Works here, thanks!

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  drivers/input/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
> index 735f90b74ee5ad44..3bdbd34314b3495a 100644
> --- a/drivers/input/Kconfig
> +++ b/drivers/input/Kconfig
> @@ -168,7 +168,7 @@ config INPUT_EVBUG
>
>  config INPUT_KUNIT_TEST
>         tristate "KUnit tests for Input" if !KUNIT_ALL_TESTS
> -       depends on INPUT && KUNIT=y
> +       depends on INPUT && KUNIT
>         default KUNIT_ALL_TESTS
>         help
>           Say Y here if you want to build the KUnit tests for the input
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-05-04  6:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-02 10:17 [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
2023-05-02 10:17 ` [PATCH 1/2] Input: tests - fix use-after-free and refcount underflow in input_test_exit() Geert Uytterhoeven
2023-05-02 10:17 ` [PATCH 2/2] Input: tests - modular KUnit tests should not depend on KUNIT=y Geert Uytterhoeven
2023-05-02 11:19   ` Javier Martinez Canillas
2023-05-02 11:22     ` Geert Uytterhoeven
2023-05-02 11:34       ` Javier Martinez Canillas
2023-05-04  6:29   ` David Gow
2023-05-02 16:09 ` [PATCH 0/2] Input: tests - miscellaneous fixes Geert Uytterhoeven
2023-05-02 16:31   ` Javier Martinez Canillas
2023-05-02 17:05     ` Dmitry Torokhov
2023-05-03  6:53       ` Geert Uytterhoeven
2023-05-03  7:05         ` Javier Martinez Canillas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).