Message ID | 20230628-pokal-puzzeln-5199c679b051@brauner |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp8827217vqr; Wed, 28 Jun 2023 03:50:55 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7/eAH8/ed/g9GTzauTR0nUMvyluIP7l2oVLseg0X90T04+nXnvov8zfX/BSz+XeK6Jejdx X-Received: by 2002:a17:906:5198:b0:98d:dfde:eb80 with SMTP id y24-20020a170906519800b0098ddfdeeb80mr10860961ejk.27.1687949454900; Wed, 28 Jun 2023 03:50:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687949454; cv=none; d=google.com; s=arc-20160816; b=r7lpw9TnEIz8YgCmTzin/y7NnCKffCEgVjzEszhPdJEPFJVEHDdi7MO4G41xpY3l1d VPhOPS/dGlSessx+tK61cLUhzmmexQilc3Y2J14hzARGGcCU1Ff8TtWEq8p/rqSE1Jks Kmz9SNdn9a1mWLzGW2c4LEP0/e4CR3DyMN50d7//B8tvmA/rr02jL3cPYyUOcSwZWwIS ApI1edqHYGRMXR930X3rDn4pgwo4+sPT5gN/cB0tgjbJUs0aFWnpN4TgNN83Ekcjf2I/ s2vvbBxgBkNJG9c2BRl6vqvymbxLPb7GAB1IPpbvwJf1SsaDyTk8fO0vax63/YqjkblS SzEA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=C5tmQJJM86gejwpQSV0WJ/6JX25pMk2HtojKd/N2Lw8=; fh=GV5hS7Fb4vkUdcSbe3cUesu51QGVzreQb5UWuqSXTrk=; b=w6YT3GtCIl1vtf2gORnVyKUeq4/t7JmarpPBsWT1nTvwnvWhpi/QB3JrbzhHVYSpGW rJ+9U2WX5jmAeJmLF84C2/G2kr4b/j8i/QMwcArp5lpmcTC4q9yjvPDrSpXAw5pD8HD7 teZ+8s0HlTMnBXe3ThUK78JjyDbcOYdAq51j5i/vi1fYH3VORiTXdbSAllLmFQcRmmwd LQRW+9Ag/Tdn6cbop3k8izYnYhWM4+o5N7YJiQTAHyZ9DWrHVm/1RX9DYzWU46GLNIpu Ge69g/LXKVIhuZ56G5zSQb2GGi6kJjNFVzir5DkmF6AiS9zJwm6YbQNPuDXnlTA5ideZ B1rQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=J603JnL9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id v11-20020a170906338b00b009887e07265esi5261304eja.49.2023.06.28.03.50.31; Wed, 28 Jun 2023 03:50:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=J603JnL9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231204AbjF1Kji (ORCPT <rfc822;adanhawthorn@gmail.com> + 99 others); Wed, 28 Jun 2023 06:39:38 -0400 Received: from dfw.source.kernel.org ([139.178.84.217]:42504 "EHLO dfw.source.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231886AbjF1Khb (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 28 Jun 2023 06:37:31 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 582F96129E for <linux-kernel@vger.kernel.org>; Wed, 28 Jun 2023 10:37:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9650EC433C0; Wed, 28 Jun 2023 10:37:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1687948650; bh=aH5Zi2HWLcJaXkjibaUhIqVkjSjeydXkdQfw0nYYoF8=; h=From:To:Cc:Subject:Date:From; b=J603JnL9uRIuxNuO2BDLuGuweOCS3KbMRwdDoSVQUZmNUuAu/k3H8S7x1O2T/5KVI 9XsCl97Dbmi8RyiDzTS8aH/DHREMFwBLHWrJdItjpkWtJIn/5BvSVwOpwY2tf/tgZ4 JlSy5yl4kuYlDbzdvwXOeGD9LhgUR3QAgAQ/tkw+iUYX+HmHOJNgpfpEfafoiFgVnx dOSN4BSzYNGy0X90ONno2MR/e3sIW6Ue1+4zNAvsRD+HQNdURuYBtu3cipQIenDk47 plSPvSku7rfmgmAeNgoGT2cZGmVUlkAAZdAya/glNBtYJLjl7+ktTt8C/34/YXYPEv fLNjBOab5nXWA== From: Christian Brauner <brauner@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Christian Brauner <brauner@kernel.org>, linux-kernel@vger.kernel.org Subject: [GIT PULL] pid: use flex array Date: Wed, 28 Jun 2023 12:37:19 +0200 Message-Id: <20230628-pokal-puzzeln-5199c679b051@brauner> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=1638; i=brauner@kernel.org; h=from:subject:message-id; bh=aH5Zi2HWLcJaXkjibaUhIqVkjSjeydXkdQfw0nYYoF8=; b=owGbwMvMwCU28Zj0gdSKO4sYT6slMaTM4Y38KT598+7PjOI/V7PMvx49YZLl/f5NQmv6pR26xM+I /dOR7ShlYRDjYpAVU2RxaDcJl1vOU7HZKFMDZg4rE8gQBi5OAZhInT3D/wjrprikIKv73AXaN3XeKT jdnqb7aHaO/5zUb1N+XDtY/Jbhf3hGcLPxMca8u8suds1etmMzZ/mnn8aH+Tter52lKbKlkxkA X-Developer-Key: i=brauner@kernel.org; a=openpgp; fpr=4880B8C9BD0E5106FC070F4F7B3C391EFEA93624 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1769943288018432993?= X-GMAIL-MSGID: =?utf-8?q?1769943288018432993?= |
Series |
[GIT,PULL] pid: use flex array
|
|
Pull-request
git@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux tags/v6.5/kernel.pidMessage
Christian Brauner
June 28, 2023, 10:37 a.m. UTC
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(-)
Comments
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.