Received-SPF: pass (google.com: domain of linux-kernel+bounces-187538-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <o89373n4-3oq5-25qr-op7n-55p9657r96o8@vanv.qr> <CAHk-=wjxdtkFMB8BPYpU3JedjAsva3XXuzwxtzKoMwQ2e8zRzw@mail.gmail.com>
 <ZkvO-h7AsWnj4gaZ@slm.duckdns.org> <CALOAHbCYpV1ubO3Z3hjMWCQnSmGd9-KYARY29p9OnZxMhXKs4g@mail.gmail.com>
 <CAHk-=wj9gFa31JiMhwN6aw7gtwpkbAJ76fYvT5wLL_tMfRF77g@mail.gmail.com>
 <CALOAHbAmHTGxTLVuR5N+apSOA29k08hky5KH9zZDY8yg2SAG8Q@mail.gmail.com> <CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com>
In-Reply-To: <CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com>
From: Yafang Shao <laoar.shao@gmail.com>
Date: Thu, 23 May 2024 21:03:34 +0800
Message-ID: <CALOAHbAAAU9MTQFc56GYoYWR3TsLbkncp5QrrwHMbqJ9SECivw@mail.gmail.com>
Subject: Re: [PATCH workqueue/for-6.10-fixes] workqueue: Refactor worker ID
 formatting and make wq_worker_comm() use full ID string
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: bpf <bpf@vger.kernel.org>, Tejun Heo <tj@kernel.org>, Jan Engelhardt <jengelh@inai.de>, 
	Craig Small <csmall@enc.com.au>, linux-kernel@vger.kernel.org, 
	Lai Jiangshan <jiangshanlai@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 23, 2024 at 12:32=E2=80=AFPM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, 22 May 2024 at 19:38, Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Indeed, the 16-byte limit is hard-coded in certain BPF code:
>
> It's worse than that.
>
> We have code like this:
>
>     memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
>
> and it looks like this code not only has a fixed-size target buffer of
> TASK_COMM_LEN, it also just uses "memcpy()" instead of "strscpy()",
> knowing that the source has the NUL byte in it.
>
> If it wasn't for that memcpy() pattern, I think this trivial patch
> would "JustWork(tm)"
>
>   diff --git a/fs/exec.c b/fs/exec.c
>   index 2d7dd0e39034..5829912a2fa0 100644
>   --- a/fs/exec.c
>   +++ b/fs/exec.c
>   @@ -1239,7 +1239,7 @@ char *__get_task_comm(char *buf, size_t
> buf_size, struct task_struct *tsk)
>    {
>         task_lock(tsk);
>         /* Always NUL terminated and zero-padded */
>   -     strscpy_pad(buf, tsk->comm, buf_size);
>   +     strscpy_pad(buf, tsk->real_comm, buf_size);
>         task_unlock(tsk);
>         return buf;
>    }
>   @@ -1254,7 +1254,7 @@ void __set_task_comm(struct task_struct *tsk,
> const char *buf, bool exec)
>    {
>         task_lock(tsk);
>         trace_task_rename(tsk, buf);
>   -     strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
>   +     strscpy_pad(tsk->real_comm, buf, sizeof(tsk->real_comm));
>         task_unlock(tsk);
>         perf_event_comm(tsk, exec);
>    }
>   diff --git a/include/linux/sched.h b/include/linux/sched.h
>   index 61591ac6eab6..948220958548 100644
>   --- a/include/linux/sched.h
>   +++ b/include/linux/sched.h
>   @@ -299,6 +299,7 @@ struct user_event_mm;
>     */
>    enum {
>         TASK_COMM_LEN =3D 16,
>   +     REAL_TASK_COMM_LEN =3D 24,
>    };
>
>    extern void sched_tick(void);
>   @@ -1090,7 +1091,10 @@ struct task_struct {
>          * - access it with [gs]et_task_comm()
>          * - lock it with task_lock()
>          */
>   -     char                            comm[TASK_COMM_LEN];
>   +     union {
>   +             char    comm[TASK_COMM_LEN];
>   +             char    real_comm[REAL_TASK_COMM_LEN];
>   +     };
>
>         struct nameidata                *nameidata;
>
> and the old common pattern of just printing with '%s' and tsk->comm
> would just continue to work:
>
>         pr_alert("BUG: Bad page state in process %s  pfn:%05lx\n",
>                 current->comm, page_to_pfn(page));
>
> but will get a longer max string.
>
> Of course, we have code like this in security/selinux/selinuxfs.c that
> is literally written so that it won't work:
>
>         if (new_value) {
>                 char comm[sizeof(current->comm)];
>
>                 memcpy(comm, current->comm, sizeof(comm));
>                 pr_err("SELinux: %s (%d) set checkreqprot to 1. This
> is no longer supported.\n",
>                        comm, current->pid);
>         }
>
> which copies to a temporary buffer (which now does *NOT* have a
> closing NUL character), and then prints from that. The intent is to at
> least have a stable buffer, but it basically relies on the source of
> the memcpy() being stable enough anyway.
>
> That said, a simple grep like this:
>
>     git grep 'memcpy.*->comm\>'
>
> more than likely finds all relevant cases. Not *that* many, and just
> changing the 'memcpy()' to 'copy_comm()' should fix them all.
>
> The "copy_comm()" would trivially look something like this:
>
>    memcpy(dst, src, TASK_COMM_LEN);
>    dst[TASK_COMM_LEN-1] =3D 0;
>
> and the people who want that old TASK_COMM_LEN behavior will get it,
> and the people who just print out ->comm as a string will magically
> get the longer new "real comm".
>
> And people who do "sizeof(->comm)" will continue to get the old value
> because of the hacky union. FWIW.
>
> Anybody want to polish up the above turd? It doesn't look all that
> hard unless I'm missing something, but needs some testing and care.

If it's not urgent and no one else will handle it, I'll take care of
it. However, I might not be able to complete it quickly.

--=20
Regards
Yafang