Re: [PATCH v2] selftests/input: add test to cover len > maxlen in bits_to_user()

Hi Dana,

Thank you for the patch.

Missatge de Dana Elfassy delfassy@redhat.com del dia dc., 21 de juny 2023 a les 10:30:

In order to cover this case, setting 'maxlen = 0', with the following explanation: EVIOCGKEY is executed from evdev_do_ioctl(), which is called from evdev_ioctl_handler(). evdev_ioctl_handler() is called from 2 functions, where by code coverage, only the first one is in use. ‘compat’ is given the value ‘0’ [1]. Thus, the condition [2] is always false. This means ‘len’ always equals a positive number [3] ‘maxlen’ in evdev_handle_get_val [4] is defined locally in evdev_do_ioctl() [5], and is sent in the variable 'size' [6]

Like the comment in my previous patch I think this is understandable for someone that has some context, but I am not sure it is understood for people that don't have such context. So I'd try to rephrase and explain in a more plain way. I.e.

selftests/input: introduce a test for the EVIOCGKEY ioctl

This patch introduces a specific test case for the EVIOCGKEY ioctl. The test covers the case where len > maxlen in the EVIOCGKEY(sizeof(keystate)), keystate) ioctl.

[1] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1281 [2] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L705 [3] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L707 [4] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L886 [5] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1155 [6] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1141

Also, usually is not a good idea to add links in commit messages reference to links that can easily end as dead links.

Signed-off-by: Dana Elfassy dangel101@gmail.com

Changes in v2:

• Added following note about the patch's dependency

This patch depends on '[v3] selftests/input: Introduce basic tests for evdev ioctls' [1] sent to the ML. [1] https://patchwork.kernel.org/project/linux-input/patch/20230607153214.15933-...

tools/testing/selftests/input/evioc-test.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/input/evioc-test.c b/tools/testing/selftests/input/evioc-test.c index ad7b93fe39cf..b94de2ee5596 100644 --- a/tools/testing/selftests/input/evioc-test.c +++ b/tools/testing/selftests/input/evioc-test.c @@ -234,4 +234,23 @@ TEST(eviocsrep_set_repeat_settings) selftest_uinput_destroy(uidev); }

+TEST(eviocgkey_get_global_key_state) +{

•   struct selftest_uinput *uidev;
  

•   int rep_values[2];
  

nit: rep_values sounds like repeat values, I'd use a variable name more similar to what we really need to pass to the ioctl, like keystate. Also I think this can be simply

int keystate;

•   int rc;
  

•   memset(rep_values, 0, sizeof(rep_values));
  

and then, this memset is not really needed.

•   rc = selftest_uinput_create_device(&uidev);
  

This one without extra arguments need to be:

rc = selftest_uinput_create_device(&uidev, -1);

otherwise the vararg loop is going to keep the room warm for no reason.

•   ASSERT_EQ(0, rc);
  

•   ASSERT_NE(NULL, uidev);
  

•   /* ioctl to create the scenario where len > maxlen in bits_to_user() */
  

•   rc = ioctl(uidev->evdev_fd, EVIOCGKEY(0), rep_values);
  

•   ASSERT_EQ(0, rc);
  

while we are here, and as it should be easy, maybe we can also extend this to cover the normal workflow to get the global key status?

•   selftest_uinput_destroy(uidev);
  

+}

TEST_HARNESS_MAIN

2.41.0

Thanks, Enric