[GIT,PULL] pid: use flex array

  Hey Linus,

/* Summary */
This contains Kees' work to make struct upid in struct pid a proper
flexible array and thus gets rid of a bunch of syzbot UBSAN warnings.

---

Sorry for sending this one later than the others.
It's pretty minimal though.

/* Testing */
clang: Ubuntu clang version 15.0.7
gcc: (Ubuntu 12.2.0-3ubuntu1) 12.2.0

All patches are based on v6.4-rc2 and have been sitting in linux-next.
No build failures or warnings were observed. All old and new tests in
selftests, and LTP pass without regressions.

/* Conflicts */
At the time of creating this PR no merge conflicts were reported from
linux-next and no merge conflicts showed up doing a test-merge with
current mainline.

The following changes since commit f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6:

  Linux 6.4-rc2 (2023-05-14 12:51:40 -0700)

are available in the Git repository at:

  git@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux tags/v6.5/kernel.pid

for you to fetch changes up to 757777eef55b48b310603d0a1f6591f2a138691b:

  pid: Replace struct pid 1-element array with flex-array (2023-05-30 17:46:48 +0200)

Please consider pulling these changes from the signed v6.5/kernel.pid tag.

Thanks!
Christian

----------------------------------------------------------------
v6.5/kernel.pid

----------------------------------------------------------------
Kees Cook (1):
      pid: Replace struct pid 1-element array with flex-array

 include/linux/pid.h    |  2 +-
 kernel/pid.c           | 13 ++++++++-----
 kernel/pid_namespace.c |  2 +-
 3 files changed, 10 insertions(+), 7 deletions(-)
  

On Wed, 28 Jun 2023 at 03:37, Christian Brauner <brauner@kernel.org> wrote:
>
> This contains Kees' work to make struct upid in struct pid a proper
> flexible array and thus gets rid of a bunch of syzbot UBSAN warnings.

Hmm. Of this, about half were replacing "array + index" with "&array[index]".

Honestly, it makes no difference, but the reverse is also true: the
"array + index" is *very* traditional, and if people have problems
with that simple syntax I really don't know what to say. It's kind of
core C. It's *literally* how arrays work, and what the '[]' operator
means.

And of the remaining half, half again is using a truly disgusting

    struct_size((struct pid *)0, numbers, X)

thing. That is *GARBAGE*. It's garbage for too many reasons for me to
actually pull this sh*t, but let me just name them:

 - 0 isn't a pointer. Stop doing that.

 - dammit, we have 'struct_size_t' that does the above disgusting cast
without getting that simple thing wrong.

In other words, this pull request contained half pointless and
unrelated churn, and 25% actual garbage.

In other words, I'm not pulling this to just get the remaining 25%.

              Linus
  

On Thu, Jun 29, 2023 at 04:52:43PM -0700, Linus Torvalds wrote:
> On Wed, 28 Jun 2023 at 03:37, Christian Brauner <brauner@kernel.org> wrote:
> >
> > This contains Kees' work to make struct upid in struct pid a proper
> > flexible array and thus gets rid of a bunch of syzbot UBSAN warnings.
> 
> Hmm. Of this, about half were replacing "array + index" with "&array[index]".
> 
> Honestly, it makes no difference, but the reverse is also true: the
> "array + index" is *very* traditional, and if people have problems
> with that simple syntax I really don't know what to say. It's kind of
> core C. It's *literally* how arrays work, and what the '[]' operator
> means.

I have no preference for either syntax. Both work. But this is probably
more an objection to this being mixed in with the flex array change in
the first place.

> 
> And of the remaining half, half again is using a truly disgusting
> 
>     struct_size((struct pid *)0, numbers, X)

I did react to that in the original review here:
https://lore.kernel.org/all/20230518-zuneigen-brombeeren-0a57cd32b1a7@brauner
but then I grepped for it and saw it done in a few other places already
which is why I didn't ask for it to be changed. See commits
48658213202c ("scsi: megaraid_sas: Use struct_size() in code related to struct MR_PD_CFG_SEQ_NUM_SYNC") 
5b12a568cc6f ("scsi: hptiop: Use struct_size() helper in code related to struct hpt_iop_request_scsi_command"
as examples.

> 
> thing. That is *GARBAGE*. It's garbage for too many reasons for me to
> actually pull this sh*t, but let me just name them:
> 
>  - 0 isn't a pointer. Stop doing that.
> 
>  - dammit, we have 'struct_size_t' that does the above disgusting cast
> without getting that simple thing wrong.
> 
> In other words, this pull request contained half pointless and
> unrelated churn, and 25% actual garbage.
> 
> In other words, I'm not pulling this to just get the remaining 25%.

Sure. @Kees, I'd appreciate it if you could change the patch according
to the comments here.
  

On Thu, 29 Jun 2023 at 23:51, Christian Brauner <brauner@kernel.org> wrote:
>
> I have no preference for either syntax. Both work. But this is probably
> more an objection to this being mixed in with the flex array change in
> the first place.

Yes. I looked at it, and tried to figure out if it was related
somehow, and decided that no, it can't possibly be, and must be just
an unrelated change.

> I did react to that in the original review here:
> https://lore.kernel.org/all/20230518-zuneigen-brombeeren-0a57cd32b1a7@brauner
> but then I grepped for it and saw it done in a few other places already

Yeah, we do end up growing new uses of 'use 0 as a pointer' almost as
quickly as we get rid of them.

We got rid of a couple just recently in commit dadeeffbe525 ("fbdev:
hitfb: Use NULL for pointers"), but yes, a quick

    git grep '\*)0\>'

shows many more.

And some of them are even ok. I don't think it's always wrong,
particularly if you then abstract it out.

So doing something like that

   #define PCI_IOBASE ((void __iomem *)0)

makes perfect sense. It's literally abstracting out something real (in
this case yes, it looks like a NULL pointer, but it's actually a
pointer with a strict type that just happens to have the value zero.

So that "NULL pointer with a type" concept makes sense, but it really
should be abstracted out, not be in the middle of some random code.

And that's *particularly* true when we already have the exact
abstraction for the situation that the code then uses (ie in this case
that "struct_size_t()" thing). Writing it out - in an ugly form -
using the disgusting traditional "constant integer zero can be cast to
any pointer" thing - just makes me go "Eugghhh!".

I mean, if this ugly part was just a small detail in a l;arger patch,
I probably would just have let it slide. It works. It is what it is.

But when three quarters of the patch was stuff I found questionable, I
just couldn't stomach it.

I'm looking at some of the grep hits, and I'm going "ok, that's a C++
programmer". I think there's a very real reason why so many of them
are in bpf code and in the bpf tests.

C++ made a *huge* fundamental design mistake early on wrt NULL, and a
generation of C++ programmers were poisoned by it and you had people
who swore until they were blue that 0 and NULL had to be the same
thing.

They were horribly and utterly wrong. But sometimes when it takes you
three decades to admit that you were wrong all that time, it's just
too painful to admit.

There are literally people who still can't admit to their mistake, and
refuse to use NULL, and use either 0 or 'nullptr'.

                  Linus
  

On Fri, Jun 30, 2023 at 12:12:22AM -0700, Linus Torvalds wrote:
> On Thu, 29 Jun 2023 at 23:51, Christian Brauner <brauner@kernel.org> wrote:
> >
> > I have no preference for either syntax. Both work. But this is probably
> > more an objection to this being mixed in with the flex array change in
> > the first place.
> 
> Yes. I looked at it, and tried to figure out if it was related
> somehow, and decided that no, it can't possibly be, and must be just
> an unrelated change.

Yeah, I admit that I would've paid more attention to this detail if it
would've been in a fs/ codepath. So that's fully on me.

> 
> > I did react to that in the original review here:
> > https://lore.kernel.org/all/20230518-zuneigen-brombeeren-0a57cd32b1a7@brauner
> > but then I grepped for it and saw it done in a few other places already
> 
> Yeah, we do end up growing new uses of 'use 0 as a pointer' almost as
> quickly as we get rid of them.
> 
> We got rid of a couple just recently in commit dadeeffbe525 ("fbdev:
> hitfb: Use NULL for pointers"), but yes, a quick
> 
>     git grep '\*)0\>'
> 
> shows many more.
> 
> And some of them are even ok. I don't think it's always wrong,
> particularly if you then abstract it out.
> 
> So doing something like that
> 
>    #define PCI_IOBASE ((void __iomem *)0)
> 
> makes perfect sense. It's literally abstracting out something real (in
> this case yes, it looks like a NULL pointer, but it's actually a
> pointer with a strict type that just happens to have the value zero.
> 
> So that "NULL pointer with a type" concept makes sense, but it really
> should be abstracted out, not be in the middle of some random code.
> 
> And that's *particularly* true when we already have the exact
> abstraction for the situation that the code then uses (ie in this case
> that "struct_size_t()" thing). Writing it out - in an ugly form -
> using the disgusting traditional "constant integer zero can be cast to
> any pointer" thing - just makes me go "Eugghhh!".
> 
> I mean, if this ugly part was just a small detail in a l;arger patch,
> I probably would just have let it slide. It works. It is what it is.
> 
> But when three quarters of the patch was stuff I found questionable, I
> just couldn't stomach it.
> 
> I'm looking at some of the grep hits, and I'm going "ok, that's a C++
> programmer". I think there's a very real reason why so many of them
> are in bpf code and in the bpf tests.
> 
> C++ made a *huge* fundamental design mistake early on wrt NULL, and a
> generation of C++ programmers were poisoned by it and you had people
> who swore until they were blue that 0 and NULL had to be the same
> thing.
> 
> They were horribly and utterly wrong. But sometimes when it takes you
> three decades to admit that you were wrong all that time, it's just
> too painful to admit.
> 
> There are literally people who still can't admit to their mistake, and
> refuse to use NULL, and use either 0 or 'nullptr'.

I have to admit that I've never touched C++ in my life in any meaningful
way. It was C, Go, and then Rust...

I've grepped around a bit and I saw that the
struct_size((struct bla *)NULL, ...)
pattern seems to be used in most places that have similar needs. Not
sure if there's something nicer.

I gave this thing a stab myself since I have a few minutes and so Kees
doesn't have to do it. Authorship retained and dropped the ack. Is the
following more acceptable? 

From ee7cb2546ff47d02e284fd8461ae1c57fab8ca49 Mon Sep 17 00:00:00 2001
From: Kees Cook <keescook@chromium.org>
Date: Fri, 30 Jun 2023 09:46:17 +0200
Subject: [PATCH] pid: Replace struct pid 1-element array with flex-array

For pid namespaces, struct pid uses a dynamically sized array member,
"numbers". This was implemented using the ancient 1-element fake flexible
array, which has been deprecated for decades. Replace it with a C99
flexible array, refactor the array size calculations to use struct_size(),
and address elements via indexes. Note that the static initializer (which
defines a single element) works as-is, and requires no special handling.

Without this, CONFIG_UBSAN_BOUNDS (and potentially CONFIG_FORTIFY_SOURCE)
will trigger bounds checks:
https://lore.kernel.org/lkml/20230517-bushaltestelle-super-e223978c1ba6@brauner

Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Jeff Xu <jeffxu@google.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Daniel Verkamp <dverkamp@chromium.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Jeff Xu <jeffxu@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Reported-by: syzbot+ac3b41786a2d0565b6d5@syzkaller.appspotmail.com
[brauner: dropped unrelated changes and remove 0 with NULL cast]
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/pid.h    | 2 +-
 kernel/pid.c           | 7 +++++--
 kernel/pid_namespace.c | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index b75de288a8c2..653a527574c4 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -67,7 +67,7 @@ struct pid
 	/* wait queue for pidfd notifications */
 	wait_queue_head_t wait_pidfd;
 	struct rcu_head rcu;
-	struct upid numbers[1];
+	struct upid numbers[];
 };
 
 extern struct pid init_struct_pid;
diff --git a/kernel/pid.c b/kernel/pid.c
index f93954a0384d..8bce3aebc949 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -656,8 +656,11 @@ void __init pid_idr_init(void)
 
 	idr_init(&init_pid_ns.idr);
 
-	init_pid_ns.pid_cachep = KMEM_CACHE(pid,
-			SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
+	init_pid_ns.pid_cachep = kmem_cache_create("pid",
+			struct_size((struct pid *)NULL, numbers, 1),
+			__alignof__(struct pid),
+			SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT,
+			NULL);
 }
 
 static struct file *__pidfd_fget(struct task_struct *task, int fd)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index b43eee07b00c..70a929784a5d 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -48,7 +48,7 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
 		return kc;
 
 	snprintf(name, sizeof(name), "pid_%u", level + 1);
-	len = sizeof(struct pid) + level * sizeof(struct upid);
+	len = struct_size((struct pid *)NULL, numbers, level + 1);
 	mutex_lock(&pid_caches_mutex);
 	/* Name collision forces to do allocation under mutex. */
 	if (!*pkc)
  

From: Christian Brauner
> Sent: 30 June 2023 09:04
...
> > And some of them are even ok. I don't think it's always wrong,
> > particularly if you then abstract it out.
> >
> > So doing something like that
> >
> >    #define PCI_IOBASE ((void __iomem *)0)
> >
> > makes perfect sense. It's literally abstracting out something real (in
> > this case yes, it looks like a NULL pointer, but it's actually a
> > pointer with a strict type that just happens to have the value zero.

You can't do that, it doesn't work.
A NULL pointer is any constant integer expression that evaluates
to zero implicitly or explicitly converted to a pointer type.

The bit pattern used for a NULL pointer is implementation defined.
It is almost always 0, but the C language allows any invalid
value to be used - eg the 'all ones' pattern.

clang warns for 'PCI_IOBASE + 4'.
Probably because it only has the expected value if NULL is the
zero bit pattern - so it isn't portable.

If has to be said that I doubt gcc or clang support NULL being
other than the zero bit pattern.
Any C code that uses memset() to set a pointer to NULL is making
that assumption.

I have used a system where the native 'invalid pointer' was ~0
but the C port used zero.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Fri, 30 Jun 2023 at 04:21, David Laight <David.Laight@aculab.com> wrote:
>
> The bit pattern used for a NULL pointer is implementation defined.

Oh, pretty much everything we do in the kernel is implementation defined.

And yes, we do indeed basically rely on the bit pattern for NULL being
all zeroes, and that example I gave with PCI_IOBASE is just one of
many.

And yes, compilers will sometimes complain about the things we do.

On s390, for example, the low memory range is special (and the kernel
mapping is special), so s390 uses all these pointers that are
*literally* small constants, and we've had to turn off some compiler
warnings because gcc would think that we're doing small offsets off
NULL.

But "implementation defined" is actually the good case.

The problem case is "undefined", when we sometimes want to do exactly
those kinds of things. We try to generally avoid it, but we sometimes
end up using compiler switches to say "turn off that part of your
logic" (strict overflow comes to mind).

             Linus
  

On Fri, Jun 30, 2023 at 10:04:14AM +0200, Christian Brauner wrote:
> On Fri, Jun 30, 2023 at 12:12:22AM -0700, Linus Torvalds wrote:
> > On Thu, 29 Jun 2023 at 23:51, Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > I have no preference for either syntax. Both work. But this is probably
> > > more an objection to this being mixed in with the flex array change in
> > > the first place.
> > 
> > Yes. I looked at it, and tried to figure out if it was related
> > somehow, and decided that no, it can't possibly be, and must be just
> > an unrelated change.

Yes, those changes were style changes because I was annoyed that a grep
for 'numbers[' didn't turn anything up. :P Since it's an array I think
it's just good form to use [] when accessing an element. But yes, it's
conceptually the same.

> > > I did react to that in the original review here:
> > > https://lore.kernel.org/all/20230518-zuneigen-brombeeren-0a57cd32b1a7@brauner
> > > but then I grepped for it and saw it done in a few other places already
> > 
> > Yeah, we do end up growing new uses of 'use 0 as a pointer' almost as
> > quickly as we get rid of them.

Apologies on this -- this patch was just before the addition of
struct_size_t(), so I missed it in the cleanup I did for that:
https://git.kernel.org/linus/d67790ddf0219aa0ad3e13b53ae0a7619b3425a2

> I've grepped around a bit and I saw that the
> struct_size((struct bla *)NULL, ...)
> pattern seems to be used in most places that have similar needs. Not
> sure if there's something nicer.

The above patch fixes them all (excepting struct pid). In retrospect, I
should have asked to carry the struct pid fix in the hardening tree due
to that.

> I gave this thing a stab myself since I have a few minutes and so Kees
> doesn't have to do it. Authorship retained and dropped the ack. Is the
> following more acceptable? 

Thanks for reworking it!

> [...]
> [brauner: dropped unrelated changes and remove 0 with NULL cast]

However, this should use struct_size_t(); I'll send a new patch and
double check that UBSAN stays happy, etc.

Sorry for the mess!

-Kees
  

On Fri, Jun 30, 2023 at 09:59:47AM -0700, Kees Cook wrote:
> On Fri, Jun 30, 2023 at 10:04:14AM +0200, Christian Brauner wrote:
> > On Fri, Jun 30, 2023 at 12:12:22AM -0700, Linus Torvalds wrote:
> > > On Thu, 29 Jun 2023 at 23:51, Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > I have no preference for either syntax. Both work. But this is probably
> > > > more an objection to this being mixed in with the flex array change in
> > > > the first place.
> > > 
> > > Yes. I looked at it, and tried to figure out if it was related
> > > somehow, and decided that no, it can't possibly be, and must be just
> > > an unrelated change.
> 
> Yes, those changes were style changes because I was annoyed that a grep
> for 'numbers[' didn't turn anything up. :P Since it's an array I think
> it's just good form to use [] when accessing an element. But yes, it's
> conceptually the same.
> 
> > > > I did react to that in the original review here:
> > > > https://lore.kernel.org/all/20230518-zuneigen-brombeeren-0a57cd32b1a7@brauner
> > > > but then I grepped for it and saw it done in a few other places already
> > > 
> > > Yeah, we do end up growing new uses of 'use 0 as a pointer' almost as
> > > quickly as we get rid of them.
> 
> Apologies on this -- this patch was just before the addition of
> struct_size_t(), so I missed it in the cleanup I did for that:
> https://git.kernel.org/linus/d67790ddf0219aa0ad3e13b53ae0a7619b3425a2
> 
> > I've grepped around a bit and I saw that the
> > struct_size((struct bla *)NULL, ...)
> > pattern seems to be used in most places that have similar needs. Not
> > sure if there's something nicer.
> 
> The above patch fixes them all (excepting struct pid). In retrospect, I
> should have asked to carry the struct pid fix in the hardening tree due
> to that.
> 
> > I gave this thing a stab myself since I have a few minutes and so Kees
> > doesn't have to do it. Authorship retained and dropped the ack. Is the
> > following more acceptable? 
> 
> Thanks for reworking it!
> 
> > [...]
> > [brauner: dropped unrelated changes and remove 0 with NULL cast]
> 
> However, this should use struct_size_t(); I'll send a new patch and
> double check that UBSAN stays happy, etc.

I think Linus already applied it let me quickly send a follow-up
replacing the two open-coded cases with struct_size_t(). I didn't know
this existed and I think when you originally send the patch it didn't.