[1/1,1/1] Add ioctl_loop08 test for LOOP_GET_STATUS lock imbalance

From: xiaoshoukui <xiaoshoukui@ruijie.com.cn>

Signed-off-by: xiaoshoukui <xiaoshoukui@ruijie.com.cn>
---
 testcases/kernel/syscalls/ioctl/Makefile      |   1 +
 .../kernel/syscalls/ioctl/ioctl_loop08.c      | 132 ++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_loop08.c

Hi!
> +static void verify_ioctl_loop(void)
> +{
> +	struct tcase *tc = &tcases[0];
> +	pthread_t th1, th2;
> +
> +	pthread_create(&th1, NULL, ioctl_thread, tc);
> +	usleep(500000);
> +	pthread_create(&th2, NULL, ioctl_thread, tc + 1);
> +	usleep(500000);

This part looks strange, why do we run the code that calls the ioctl in
a separate thread if we are sleeping after the thread has been created
and quite likely the thread will finish before the main thread returns
from the sleep?

Why can we just call the ioctl_thread() sequentially here?

> +	if (!find_kmsg(lock_imbalance))
> +		tst_res(TFAIL, "Trigger lock imbalance");
> +	else
> +		tst_res(TPASS, "Nothing bad happened, probably");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_ioctl_loop,
> +	.needs_root = 1,
> +	.needs_kconfigs = (const char *[]) {
> +					    "CONFIG_LOCKDEP=y",
> +					    NULL },
> +	.tags = (const struct tst_tag[]) {
> +					  { "linux-git", "bdac616db9bb "},
> +					  {}
> +					   },
> +	.needs_drivers = (const char *const[]) {
> +						"loop",
> +						NULL }

The whitespaces here are all messed up.

> +};
> -- 
> 2.20.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Sincere thanks for your advice.
Based on my tests，the lockdep will block the ioctl request thread return to
userspace when it detect a lock imbalance. Place ioctl request in the main
thread, there is no chance to execute find_kmsg for determining what
exactly a lock problem happaned and printing the test result.

On Fri, Feb 10, 2023 at 6:47 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > +static void verify_ioctl_loop(void)
> > +{
> > +     struct tcase *tc = &tcases[0];
> > +     pthread_t th1, th2;
> > +
> > +     pthread_create(&th1, NULL, ioctl_thread, tc);
> > +     usleep(500000);
> > +     pthread_create(&th2, NULL, ioctl_thread, tc + 1);
> > +     usleep(500000);
>
> This part looks strange, why do we run the code that calls the ioctl in
> a separate thread if we are sleeping after the thread has been created
> and quite likely the thread will finish before the main thread returns
> from the sleep?
>
> Why can we just call the ioctl_thread() sequentially here?
>
> > +     if (!find_kmsg(lock_imbalance))
> > +             tst_res(TFAIL, "Trigger lock imbalance");
> > +     else
> > +             tst_res(TPASS, "Nothing bad happened, probably");
> > +}
> > +
> > +static struct tst_test test = {
> > +     .test_all = verify_ioctl_loop,
> > +     .needs_root = 1,
> > +     .needs_kconfigs = (const char *[]) {
> > +                                         "CONFIG_LOCKDEP=y",
> > +                                         NULL },
> > +     .tags = (const struct tst_tag[]) {
> > +                                       { "linux-git", "bdac616db9bb "},
> > +                                       {}
> > +                                        },
> > +     .needs_drivers = (const char *const[]) {
> > +                                             "loop",
> > +                                             NULL }
>
> The whitespaces here are all messed up.
>
> > +};
> > --
> > 2.20.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>

Hi!
> Sincere thanks for your advice.
> Based on my tests，the lockdep will block the ioctl request thread return to
> userspace when it detect a lock imbalance. Place ioctl request in the main
> thread, there is no chance to execute find_kmsg for determining what
> exactly a lock problem happaned and printing the test result.

Hmm, then maybe it would be easier and more reliable to run the ioctl()
in a child processes and fail the test when the parent detects the
child to lockup.

I suppose that the process that called the ioctl() ends up in the D
state, right? In that case the parent read the /proc/pid/stat a few
times with slight delays between them and if the process keeps hanging
in D state we declare it blocked forever.

good point.  It's more user friendly. change to fork child

---
 .../kernel/syscalls/ioctl/ioctl_loop08.c      | 147 ++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_loop08.c

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
b/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
new file mode 100644
index 000000000..047273576
--- /dev/null
+++ b/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 xiaoshoukui <xiaoshoukui@ruijie.com.cn>
+ */
+
+/*\
+ * [Description]
+ *
+ * This is a basic ioctl test about loopdevice LOOP_GET_STATUS
+ * and LOOP_GET_STATUS64.
+ * Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
+ * returning, but the loop_get_status_old(), loop_get_status64(), and
+ * loop_get_status_compat() wrappers don't call loop_get_status() if the
+ * passed argument is NULL. The callers expect that the lock is dropped, so
+ * make sure we drop it in that case, too.
+ *
+ * Fixed by commit:
+ *
+ *  commit bdac616db9bbadb90b7d6a406144571015e138f7
+ *  Author: Omar Sandoval <osandov@fb.com>
+ *  Date:   Fri Apr 06 09:57:03 2018 -0700
+ *
+ *    loop: fix LOOP_GET_STATUS lock imbalance
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <stdlib.h>
+#include "lapi/loop.h"
+#include "tst_test.h"
+
+#define MAX_MSGSIZE 4096
+
+static const char lock_imbalance[] = "lock held when returning to user
space";
+
+static struct tcase {
+       int ioctl_flag;
+       char *message;
+} tcases[] = {
+       { LOOP_GET_STATUS,
+        "Testing LOOP_GET_STATUS lock imbalance" },
+
+       { LOOP_GET_STATUS64,
+        "Testing LOOP_GET_STATUS64 lock imbalance" },
+};
+
+static int find_kmsg(const char *text_to_find)
+{
+       int f, msg_found = 0;
+       char msg[MAX_MSGSIZE + 1];
+
+       f = SAFE_OPEN("/dev/kmsg", O_RDONLY | O_NONBLOCK);
+
+       while (1) {
+               TEST(read(f, msg, MAX_MSGSIZE));
+               if (TST_RET < 0) {
+                       if (TST_ERR == EAGAIN)
+                               /* there are no more messages */
+                               break;
+                       else if (TST_ERR == EPIPE)
+                               /* current message was overwritten */
+                               continue;
+                       else
+                               tst_brk(TBROK | TTERRNO,
+                                       "err reading /dev/kmsg");
+               } else {
+                       /* lines from kmsg are not NULL terminated */
+                       msg[TST_RET] = '\0';
+                       if (strstr(msg, text_to_find) != NULL) {
+                               msg_found = 1;
+                               break;
+                       }
+               }
+       }
+       SAFE_CLOSE(f);
+
+       if (msg_found)
+               return 0;
+       else
+               return -1;
+}
+
+static void do_child(void)
+{
+       char dev_path[1024];
+       int dev_num, dev_fd;
+       unsigned int i;
+
+       for (i = 0; i < ARRAY_SIZE(tcases); i++) {
+               tst_res(TINFO, "%s", tcases[i].message);
+               dev_num = tst_find_free_loopdev(dev_path, sizeof(dev_path));
+
+               if (dev_num < 0)
+                       tst_brk(TBROK, "Failed to find free loop device");
+
+               dev_fd = SAFE_OPEN(dev_path, O_RDWR);
+
+               if (tcases[i].ioctl_flag == LOOP_GET_STATUS)
+                       ioctl(dev_fd, LOOP_GET_STATUS, NULL);
+               else
+                       ioctl(dev_fd, LOOP_GET_STATUS64, NULL);
+
+               if (dev_fd > 0)
+                       SAFE_CLOSE(dev_fd);
+
+       }
+
+       exit(0);
+
+}
+
+static void verify_ioctl_loop(void)
+{
+       int ret, pid;
+
+       pid = SAFE_FORK();
+       if (!pid)
+               do_child();
+
+       ret = TST_PROCESS_STATE_WAIT(pid, 'D', 5000);
+
+       if (!ret && !find_kmsg(lock_imbalance))
+               tst_res(TFAIL, "Trigger ioctl loop lock imbalance");
+       else
+               tst_res(TPASS, "Nothing bad happened, probably");
+
+}
+
+static struct tst_test test = {
+       .test_all = verify_ioctl_loop,
+       .needs_root = 1,
+       .needs_kconfigs = (const char *[]) {
+               "CONFIG_LOCKDEP=y",
+               NULL
+       },
+       .tags = (const struct tst_tag[]) {
+               {"linux-git", "bdac616db9bb "},
+               {}
+       },
+       .needs_drivers = (const char *const[]) {
+               "loop",
+               NULL
+       },
+       .forks_child = 1,
+       .timeout = 60,
+};

Hello,

xiao shoukui <xiaoshoukui@gmail.com> writes:

> good point.  It's more user friendly. change to fork child

Please submit a V2 patch with these changes.

You can CC Cyril on the new patch and add a In-reply-to if you want it
to be part of this thread. Most importantly though is that it is marked
as V2.

>
> ---
>  .../kernel/syscalls/ioctl/ioctl_loop08.c      | 147 ++++++++++++++++++
>  1 file changed, 147 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_loop08.c
>
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
> b/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
> new file mode 100644
> index 000000000..047273576
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop08.c
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 xiaoshoukui <xiaoshoukui@ruijie.com.cn>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This is a basic ioctl test about loopdevice LOOP_GET_STATUS
> + * and LOOP_GET_STATUS64.
> + * Commit 2d1d4c1e591f made loop_get_status() drop lo_ctx_mutex before
> + * returning, but the loop_get_status_old(), loop_get_status64(), and
> + * loop_get_status_compat() wrappers don't call loop_get_status() if the
> + * passed argument is NULL. The callers expect that the lock is dropped, so
> + * make sure we drop it in that case, too.
> + *
> + * Fixed by commit:
> + *
> + *  commit bdac616db9bbadb90b7d6a406144571015e138f7
> + *  Author: Omar Sandoval <osandov@fb.com>
> + *  Date:   Fri Apr 06 09:57:03 2018 -0700
> + *
> + *    loop: fix LOOP_GET_STATUS lock imbalance
> + */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <stdlib.h>
> +#include "lapi/loop.h"
> +#include "tst_test.h"
> +
> +#define MAX_MSGSIZE 4096
> +
> +static const char lock_imbalance[] = "lock held when returning to user
> space";
> +
> +static struct tcase {
> +       int ioctl_flag;
> +       char *message;
> +} tcases[] = {
> +       { LOOP_GET_STATUS,
> +        "Testing LOOP_GET_STATUS lock imbalance" },
> +
> +       { LOOP_GET_STATUS64,
> +        "Testing LOOP_GET_STATUS64 lock imbalance" },
> +};
> +
> +static int find_kmsg(const char *text_to_find)
> +{
> +       int f, msg_found = 0;
> +       char msg[MAX_MSGSIZE + 1];
> +
> +       f = SAFE_OPEN("/dev/kmsg", O_RDONLY | O_NONBLOCK);
> +
> +       while (1) {
> +               TEST(read(f, msg, MAX_MSGSIZE));
> +               if (TST_RET < 0) {
> +                       if (TST_ERR == EAGAIN)
> +                               /* there are no more messages */
> +                               break;
> +                       else if (TST_ERR == EPIPE)
> +                               /* current message was overwritten */
> +                               continue;
> +                       else
> +                               tst_brk(TBROK | TTERRNO,
> +                                       "err reading /dev/kmsg");
> +               } else {
> +                       /* lines from kmsg are not NULL terminated */
> +                       msg[TST_RET] = '\0';
> +                       if (strstr(msg, text_to_find) != NULL) {
> +                               msg_found = 1;
> +                               break;
> +                       }
> +               }
> +       }
> +       SAFE_CLOSE(f);
> +
> +       if (msg_found)
> +               return 0;
> +       else
> +               return -1;
> +}
> +
> +static void do_child(void)
> +{
> +       char dev_path[1024];
> +       int dev_num, dev_fd;
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(tcases); i++) {
> +               tst_res(TINFO, "%s", tcases[i].message);
> +               dev_num = tst_find_free_loopdev(dev_path, sizeof(dev_path));
> +
> +               if (dev_num < 0)
> +                       tst_brk(TBROK, "Failed to find free loop device");
> +
> +               dev_fd = SAFE_OPEN(dev_path, O_RDWR);
> +
> +               if (tcases[i].ioctl_flag == LOOP_GET_STATUS)
> +                       ioctl(dev_fd, LOOP_GET_STATUS, NULL);
> +               else
> +                       ioctl(dev_fd, LOOP_GET_STATUS64, NULL);
> +
> +               if (dev_fd > 0)
> +                       SAFE_CLOSE(dev_fd);
> +
> +       }
> +
> +       exit(0);
> +
> +}
> +
> +static void verify_ioctl_loop(void)
> +{
> +       int ret, pid;
> +
> +       pid = SAFE_FORK();
> +       if (!pid)
> +               do_child();
> +
> +       ret = TST_PROCESS_STATE_WAIT(pid, 'D', 5000);
> +
> +       if (!ret && !find_kmsg(lock_imbalance))
> +               tst_res(TFAIL, "Trigger ioctl loop lock imbalance");
> +       else
> +               tst_res(TPASS, "Nothing bad happened, probably");
> +
> +}
> +
> +static struct tst_test test = {
> +       .test_all = verify_ioctl_loop,
> +       .needs_root = 1,
> +       .needs_kconfigs = (const char *[]) {
> +               "CONFIG_LOCKDEP=y",
> +               NULL
> +       },
> +       .tags = (const struct tst_tag[]) {
> +               {"linux-git", "bdac616db9bb "},
> +               {}
> +       },
> +       .needs_drivers = (const char *const[]) {
> +               "loop",
> +               NULL
> +       },
> +       .forks_child = 1,
> +       .timeout = 60,
> +};
> -- 
> 2.20.1
>
>
> On Fri, Feb 10, 2023 at 8:12 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
>> Hi!
>> > Sincere thanks for your advice.
>> > Based on my tests，the lockdep will block the ioctl request thread return
>> to
>> > userspace when it detect a lock imbalance. Place ioctl request in the
>> main
>> > thread, there is no chance to execute find_kmsg for determining what
>> > exactly a lock problem happaned and printing the test result.
>>
>> Hmm, then maybe it would be easier and more reliable to run the ioctl()
>> in a child processes and fail the test when the parent detects the
>> child to lockup.
>>
>> I suppose that the process that called the ioctl() ends up in the D
>> state, right? In that case the parent read the /proc/pid/stat a few
>> times with slight delays between them and if the process keeps hanging
>> in D state we declare it blocked forever.
>>
>> --
>> Cyril Hrubis
>> chrubis@suse.cz
>>