linux-kernel - Re: [PATCH 4/5] kernel: Prepare set_kthread_struct to be used for setup_thread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Feb 2023 11:54:23 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Mike Christie <michael.christie@...cle.com>
Cc:     brauner@...nel.org, ebiederm@...ssion.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] kernel: Prepare set_kthread_struct to be used for setup_thread_fn

On Sun, Feb 12, 2023 at 5:00 PM Mike Christie
<michael.christie@...cle.com> wrote:
>
> This preps set_kthread_struct to be used for a setup_thread_fn callback
> by having it set the task's comm and also returning an int instead of a
> bool.

Ok, so I like the concept, but this patch is just too ugly for words,
and very very confused.

Now, some of it is pre-exisging nasty code just moved around:

> +       mutex_lock(&create->name_mutex);
> +       if (!create->name_args) {
> +               mutex_unlock(&create->name_mutex);
> +               return -EINTR;
> +       }
> +
> +       va_copy(name_args, *create->name_args);
> +       len = vsnprintf(tsk->comm, TASK_COMM_LEN, create->name_fmt, name_args);
> +       va_end(name_args);
> +       if (len >= TASK_COMM_LEN) {
> +               /* leave it truncated when out of memory. */
> +               kthread->full_name = kvasprintf(GFP_KERNEL, create->name_fmt,
> +                                               *create->name_args);
> +       }
> +       mutex_unlock(&create->name_mutex);

The *whole* point of my suggestion was to stop having silly pointless
locking on the name, because this all should be local to that one
thread creation, so that "name_mutex" kind of makes this all
pointless,

But what the heck is this:

> +       mutex_init(&create->name_mutex);
> +       create->name_fmt = namefmt;
> +       va_copy(name_args, args);
> +       create->name_args = &name_args;

That's just crazy talk.

Please just create the name once.

And please don't think that just because it was using a "va_list", you
need to keep it in that format.

Just make it create the name in __kthread_create_on_node() and be done
with it. That code already does a

        struct kthread_create_info *create = kmalloc(sizeof(*create), ..

and you can just make a sufficiently large buffer there. Don't worry
about "kthread->fuil_name" being huge, it should just be bigger than
16. Make it be 32 or something. Nobody wants a larger "full name"
anyway.

No name_mutex, no va_list that is bigger than the buffer we'd need for
the name anyway. Just "create the name once".

IOW, this patch is just being much too complicated for no good reason.
The point was to make it _simpler_ to do thread setup, not more
complicated.

        Linus