[LTP] [PATCH 1/1 1/1] Add ioctl_loop08 test for LOOP_GET_STATUS lock imbalance

[Richard Palethorpe]

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
>>


-- 
Thank you,
Richard.