Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Yafang Shao <laoar.shao@gmail.com>]

On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:
> > Hi Yafang,
> >
> > On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
> > > We want to eliminate the use of __get_task_comm() for the following
> > > reasons:
> > >
> > > - The task_lock() is unnecessary
> > >   Quoted from Linus [0]:
> > >   : Since user space can randomly change their names anyway, using locking
> > >   : was always wrong for readers (for writers it probably does make sense
> > >   : to have some lock - although practically speaking nobody cares there
> > >   : either, but at least for a writer some kind of race could have
> > >   : long-term mixed results
> > >
> > > - The BUILD_BUG_ON() doesn't add any value
> > >   The only requirement is to ensure that the destination buffer is a valid
> > >   array.
> > >
> > > - Zeroing is not necessary in current use cases
> > >   To avoid confusion, we should remove it. Moreover, not zeroing could
> > >   potentially make it easier to uncover bugs. If the caller needs a
> > >   zero-padded task name, it should be explicitly handled at the call site.
> > >
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> > > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> > > Suggested-by: Alejandro Colomar <alx@kernel.org>
> > > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Eric Biederman <ebiederm@xmission.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > Cc: Matus Jokay <matus.jokay@stuba.sk>
> > > Cc: Alejandro Colomar <alx@kernel.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > ---
> > >  fs/exec.c             | 10 ----------
> > >  fs/proc/array.c       |  2 +-
> > >  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> > >  kernel/kthread.c      |  2 +-
> > >  4 files changed, 28 insertions(+), 18 deletions(-)
> > >
> >
> > [...]
> >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index f8d150343d42..c40b95a79d80 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> >
> > [...]
> >
> > > @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> > >     __set_task_comm(tsk, from, false);
> > >  }
> > >
> > > -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> > > +/*
> >
> > [...]
> >
> > > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
> > > + */
> > >  #define get_task_comm(buf, tsk) ({                 \
> > > -   BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
> > > -   __get_task_comm(buf, sizeof(buf), tsk);         \
> > > +   strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));     \
> >
> > I see that there's a two-argument macro
> >
> >       #define strscpy(dst, src)       sized_strscpy(dst, src, sizeof(dst))
> >
> > which is used in patch 2/8
> >
> >       diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >       index 6f0d6fb6523f..e4ef5e57dde9 100644
> >       --- a/kernel/auditsc.c
> >       +++ b/kernel/auditsc.c
> >       @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
> >               context->target_uid = task_uid(t);
> >               context->target_sessionid = audit_get_sessionid(t);
> >               security_task_getsecid_obj(t, &context->target_sid);
> >       -       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> >       +       strscpy(context->target_comm, t->comm);
> >        }
> >
> >        /**
>
> Ahh, the actual generic definition is in <include/linux/string.h>.
> You could do
>
>         diff --git i/include/linux/string.h w/include/linux/string.h
>         index 9edace076ddb..060504719904 100644
>         --- i/include/linux/string.h
>         +++ w/include/linux/string.h
>         @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>           * known size.
>           */
>          #define __strscpy0(dst, src, ...)      \
>         -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
>         +       sized_strscpy(dst, src, ARRAY_SIZE(dst))
>          #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
>
>          #define __strscpy_pad0(dst, src, ...)  \
>         -       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
>         +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
>          #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
>
>          /**

Thank you for your suggestion. How does the following commit log look
to you? Does it meet your expectations?

    string: Use ARRAY_SIZE() in strscpy()

    We can use ARRAY_SIZE() instead to clarify that they are regular characters.

    Co-developed-by: Alejandro Colomar <alx@kernel.org>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>
    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index bbab79c0c074..07216996e3a9 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -14,7 +14,7 @@
  * copying too much infrastructure for my taste, so userspace files
  * get less checking than kernel files.
  */
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))

 /* This is to get size_t and NULL */
 #ifndef __UM_HOST__
@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
const char *prefix_str,
 extern int in_aton(char *str);
 extern size_t strlcat(char *, const char *, size_t);
 extern size_t sized_strscpy(char *, const char *, size_t);
-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))

 /* Copied from linux/compiler-gcc.h since we can't include it directly */
 #define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/include/linux/string.h b/include/linux/string.h
index 9edace076ddb..060504719904 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
  * known size.
  */
 #define __strscpy0(dst, src, ...)      \
-       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
+       sized_strscpy(dst, src, ARRAY_SIZE(dst))
 #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)

 #define __strscpy_pad0(dst, src, ...)  \
-       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
+       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
 #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

 /**

--
Regards

Yafang