Re: [PATCH] netdev: Use flexible array for trailing private bytes
On Fri, Mar 1, 2024 at 7:59 AM Jakub Kicinski wrote:
>
> On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 118c40258d07..b476809d0bae 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
> > NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
> > };
> >
> > +#define NETDEV_ALIGN32
>
> Unless someone knows what this is for it should go.
> Align priv to cacheline size.
+2
#define NETDEV_ALIGNL1_CACHE_BYTES
or a general replacement of NETDEV_ALIGN
Re: [PATCH v1 1/8] kunit: Run tests when the kernel is fully setup
Hi Mickaël,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d206a76d7d2726f3b096037f2079ce0bd3ba329b]
url:
https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/kunit-Run-tests-when-the-kernel-is-fully-setup/20240301-011020
base: d206a76d7d2726f3b096037f2079ce0bd3ba329b
patch link:
https://lore.kernel.org/r/20240229170409.365386-2-mic%40digikod.net
patch subject: [PATCH v1 1/8] kunit: Run tests when the kernel is fully setup
config: s390-defconfig
(https://download.01.org/0day-ci/archive/20240301/202403011856.cje6do38-...@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project
325f51237252e6dab8e4e1ea1fa7acbb4faee1cd)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240301/202403011856.cje6do38-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202403011856.cje6do38-...@intel.com/
All warnings (new ones prefixed by >>):
In file included from lib/kunit/executor.c:4:
In file included from include/kunit/test.h:24:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2188:
include/linux/vmstat.h:508:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
508 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
509 |item];
|
include/linux/vmstat.h:515:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
515 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
516 |NR_VM_NUMA_EVENT_ITEMS +
|~~
include/linux/vmstat.h:522:36: warning: arithmetic between different
enumeration types ('enum node_stat_item' and 'enum lru_list')
[-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~ ^ ~~~
include/linux/vmstat.h:527:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
527 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
528 |NR_VM_NUMA_EVENT_ITEMS +
|~~
include/linux/vmstat.h:536:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
536 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
537 |NR_VM_NUMA_EVENT_ITEMS +
|~~
>> lib/kunit/executor.c:18:31: warning: unused variable 'final_suite_set'
>> [-Wunused-variable]
18 | static struct kunit_suite_set final_suite_set = {};
| ^~~
6 warnings generated.
vim +/final_suite_set +18 lib/kunit/executor.c
17
> 18 static struct kunit_suite_set final_suite_set = {};
19
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2 1/2] string: Convert selftest to KUnit
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote:
>
> Convert test_string.c to KUnit so it can be easily run with everything
> else.
Have you run it?
...
> if (i < 256)
> - return (i << 24) | (j << 16) | k | 0x8000;
> - return 0;
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
First of all, this special value encodes the problematic patterns, so
you missed proper messaging.
Second, the returned value has a constant, how do you expect 0 to be
equal to something (guaranteed not to be 0)?
This needs a good rethink of what you should do in the KUnit approach.
...
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
Ditto.
...
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
Ditto.
...
> for (i = 0; i < strlen(test_string) + 1; i++) {
> result = strchr(test_string, test_string[i]);
> - if (result - test_string != i)
> - return i + 'a';
> + KUNIT_ASSERT_EQ(test, result - test_string, i);
In a similar way, all returned values are *special*, you really need
to think about them before converting to a simple (and sometimes
wrong) checks)
...
I dunno if KUnit has a fault ejection simulation. It should, in order
to be sure that test cases are fine when they fail.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote:
>
> Convert test-string_helpers.c to KUnit so it can be easily run with
> everything else.
...
> -#include
> #include
> +#include
I know the order is broken here, but don't make it worse, please. And
stick with one schema where to put kunit/test.h always before
everything else and somewhere else (like always after linux/*).
> +#include
> #include
> #include
> #include
...
> +static void test_string_check_buf(struct kunit *test,
> + const char *name, unsigned int flags,
> + char *in, size_t p,
> + char *out_real, size_t q_real,
> + char *out_test, size_t q_test)
> {
> - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> - return true;
> + int result;
> +
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
This needs a message.
> + result = memcmp(out_test, out_real, q_test);
> + KUNIT_EXPECT_EQ(test, 0, result);
Why do we need this assertion? We have a dump below to show what's wrong.
> + if (q_real == q_test && result == 0)
> + return;
I'm not sure this is an equivalent change. IIRC KUnit assertions do
not continue on failure. (Long time last time I run KUnit test)
>
> pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name,
> unsigned int flags,
>out_test, q_test, true);
> print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
>out_real, q_real, true);
> -
> - return false;
> }
...
> +static void
> +test_string_escape_overflow(struct kunit *test,
> + const char *in, int p, unsigned int flags, const
> char *esc,
> int q_test, const char *name)
> {
> int q_real;
>
> q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
> - if (q_real != q_test)
> - pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d,
> got %d\n",
> - name, flags, q_test, q_real);
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
You killed the message, not good.
> }
...
> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2)
> \
> + do {
>\
> + BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf);
>\
> + BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf);
>\
No analogous assertions in KUnit?
> + __test_string_get_size(test, (size), (blk_size),
> (exp_result10), \
> + (exp_result2));
>\
> } while (0)
...
> {
> - if (!memcmp(res, exp, strlen(exp) + 1))
> + int result = memcmp(res, exp, strlen(exp) + 1);
> + KUNIT_EXPECT_EQ(test, 0, result);
As per above, what's the added value of this assertion?
> + if (!result)
> return;
...
> @@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
> for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
> const char *s = strings_upper[i].in;
> int len = strlen(strings_upper[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_upper(dst, s);
> - if (memcmp(dst, strings_upper[i].out, len)) {
> + result = memcmp(dst, strings_upper[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_upper' failed : expected %s,
> got %s!\n",
> strings_upper[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
>
> for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
> const char *s = strings_lower[i].in;
> int len = strlen(strings_lower[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_lower(dst, s);
> - if (memcmp(dst, strings_lower[i].out, len)) {
> + result = memcmp(dst, strings_lower[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_lower failed : : expected %s,
> got %s!\n",
> strings_lower[i].out, dst);
> -
Re: [PATCH v2 0/2] string: Convert selftests to KUnit
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote: > > Hi, > > I realized the string selftests hadn't been converted to KUnit yet. Do that. IIRC last time somebody wanted to do that KUnit was completely sucking in supporting the cases we need in these files. (I mean proper messaging when we need to dump the expected and resulting data). And please Cc Rasmus as well. -- With Best Regards, Andy Shevchenko
Re: [PATCH bpf-next RESEND v2 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
Le 01/03/2024 à 08:57, Christophe Leroy a écrit :
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> an error.
>
Definitely not a good day for me. I switched maintainers between patch 1
and patch 2 sorry.
Adding hengqi.c...@gmail.com, sv...@linux.ibm.com, dsah...@kernel.org,
mi...@redhat.com, james.bottom...@hansenpartnership.com,
tsbog...@alpha.franken.de, x...@kernel.org, t...@linutronix.de,
paulbur...@kernel.org, h...@linux.ibm.com, udkni...@gmail.com,
b...@alien8.de, chenhua...@kernel.org, h...@zytor.com, g...@linux.ibm.com,
li...@armlinux.org.uk, ker...@xen0n.name, dave.han...@linux.intel.com,
borntrae...@linux.ibm.com, agord...@linux.ibm.com, del...@gmx.de
Tell me if I need to resend the series once more.
Thanks
Christophe
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy
> Cc: linux-hardening@vger.kernel.org
> Reviewed-by: Kees Cook
> Reviewed-by: Puranjay Mohan
> Reviewed-by: Ilya Leoshkevich # s390x
> Acked-by: Tiezhu Yang # LoongArch
> Reviewed-by: Johan Almbladh # MIPS Part
> ---
> Sorry for the resend, I forgot to flag patch 2 as bpf-next
>
> Previous patch introduces a dependency on this patch because it modifies
> bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict
> with unmodified bpf_prog_lock_ro().
>
> v2:
> - Dropped arm64 change following commit 1dad391daef1 ("bpf, arm64: use
> bpf_prog_pack for memory management")
> - Fixed too long lines reported by checkpatch
> ---
> arch/arm/net/bpf_jit_32.c| 25 -
> arch/loongarch/net/bpf_jit.c | 22 --
> arch/mips/net/bpf_jit_comp.c | 3 ++-
> arch/parisc/net/bpf_jit_core.c | 8 +++-
> arch/s390/net/bpf_jit_comp.c | 6 +-
> arch/sparc/net/bpf_jit_comp_64.c | 6 +-
> arch/x86/net/bpf_jit_comp32.c| 3 +--
> include/linux/filter.h | 5 +++--
> 8 files changed, 51 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 1d672457d02f..01516f83a95a 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -,28 +,21 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog
> *prog)
> /* If building the body of the JITed code fails somehow,
>* we fall back to the interpretation.
>*/
> - if (build_body(&ctx) < 0) {
> - image_ptr = NULL;
> - bpf_jit_binary_free(header);
> - prog = orig_prog;
> - goto out_imms;
> - }
> + if (build_body(&ctx) < 0)
> + goto out_free;
> build_epilogue(&ctx);
>
> /* 3.) Extra pass to validate JITed Code */
> - if (validate_code(&ctx)) {
> - image_ptr = NULL;
> - bpf_jit_binary_free(header);
> - prog = orig_prog;
> - goto out_imms;
> - }
> + if (validate_code(&ctx))
> + goto out_free;
> flush_icache_range((u32)header, (u32)(ctx.target + ctx.idx));
>
> if (bpf_jit_enable > 1)
> /* there are 2 passes here */
> bpf_jit_dump(prog->len, image_size, 2, ctx.target);
>
> - bpf_jit_binary_lock_ro(header);
> + if (bpf_jit_binary_lock_ro(header))
> + goto out_free;
> prog->bpf_func = (void *)ctx.target;
> prog->jited = 1;
> prog->jited_len = image_size;
> @@ -2260,5 +2253,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog
> *prog)
> bpf_jit_prog_release_other(prog, prog == orig_prog ?
> tmp : orig_prog);
> return prog;
> +
> +out_free:
> + image_ptr = NULL;
> + bpf_jit_binary_free(header);
> + prog = orig_prog;
> + goto out_imms;
> }
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index e73323d759d0..7dbefd4ba210 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1294,16 +1294,19 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog
> *prog)
> flush_icache_range((unsigned long)header, (unsigned long)(ctx.image +
> ctx.idx));
>
> if (!prog->is_func || extra_pass) {
> + int err;
> +
> if (extra_pass && ctx.idx != jit_data->ctx.idx) {
> pr_err_once("multi-func JIT bug %d != %d\n",
> ctx.idx, jit_data->ctx.idx);
> - bpf_jit_binary_free(header);
> - prog->bpf_func = NULL;
> - prog->jited = 0;
> - prog->jited_len = 0;
> - goto out_offset;
> + goto out_free;
> + }
> + err = bpf_jit_binary_lock_ro(header);
> + if (err) {
> + pr_err_once("bpf_jit_binary_lock_ro() returne
Re: [PATCH] netdev: Use flexible array for trailing private bytes
On Thu, Feb 29, 2024 at 10:59:10PM -0800, Jakub Kicinski wrote:
> On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
> > Introduce a new struct net_device_priv that contains struct net_device
> > but also accounts for the commonly trailing bytes through the "size" and
> > "data" members.
>
> I'm a bit unclear on the benefit. Perhaps I'm unaccustomed to "safe C".
>
> > As many dummy struct net_device instances exist still,
> > it is non-trivial to but this flexible array inside struct net_device
>
> put
>
> Non-trivial, meaning what's the challenge?
> We also do somewhat silly things with netdev lifetime, because we can't
> assume netdev gets freed by netdev_free(). Cleaning up the "embedders"
> would be beneficial for multiple reasons.
>
> > itself. But we can add a sanity check in netdev_priv() to catch any
> > attempts to access the private data of a dummy struct.
> >
> > Adjust allocation logic to use the new full structure.
> >
> > Signed-off-by: Kees Cook
>
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 118c40258d07..b476809d0bae 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
> > NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
> > };
> >
> > +#defineNETDEV_ALIGN32
>
> Unless someone knows what this is for it should go.
> Align priv to cacheline size.
>
> > /**
> > * struct net_device - The DEVICE structure.
> > *
>
> > @@ -2665,7 +2673,14 @@ void dev_net_set(struct net_device *dev, struct net
> > *net)
> > */
> > static inline void *netdev_priv(const struct net_device *dev)
> > {
> > - return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> > + struct net_device_priv *priv;
> > +
> > + /* Dummy struct net_device have no trailing data. */
> > + if (WARN_ON_ONCE(dev->reg_state == NETREG_DUMMY))
> > + return NULL;
>
> This is a static inline with roughly 11,000 call sites, according to
> a quick grep. Aren't WARN_ONCE() in static inlines creating a "once"
> object in every compilation unit where they get used?
It also, if this every trips, will reboot the box for those that run
with panic-on-warn set, is that something that you all really want?
thanks,
greg k-h
Re: [PATCH] arm64: smp: smp_send_stop() and crash_smp_send_stop() should try non-NMI first
On Thu, Feb 29, 2024 at 10:34:26AM -0800, Doug Anderson wrote: > Hi, > > On Wed, Feb 28, 2024 at 5:11 AM Daniel Thompson > wrote: > > > > > I'm still hoping to get some sort of feedback here. If people think > > > this is a terrible idea then I'll shut up now and leave well enough > > > alone, but it would be nice to actively decide and get the patch out > > > of limbo. > > > > I've read patch through a couple of times and was generally convinced by > > the "do what x86 does" argument. > > > > However until now I've always held my council since I wasn't familiar > > with these code paths and I figured it was OK for me to have no opinion > > because the first line of the description says that kgdb/kdb is 100% not > > involved in causing the problem ;-) . > > > > However today I also took a look at the HAVE_NMI architectures and there > > is no consensus between them about how to implement this: PowerPC uses > > NMI and most of the others use IRQ only, s390 special cases for the > > panic code path and acts differently compared to a normal SMP shutdown. > > > > > > > > However, if we talking ourselves into copying x86 then perhaps we should > > more accurately copy x86! Assuming I read the x86 code correctly then > > crash_smp_send_stop() will (mostly) go staight to NMI rather > > than trialling an IRQ first! That is not what is currently implemented > > in the patch for arm64. > > Sure, I'm happy to change the patch to work that way, though I might > wait to get some confirmation from a maintainer that they think this > idea is worth pursuing before spending more time on it. 100%. Don't respin on my account. > I don't think it would be hard to have the "crash stop" code jump > straight to NMI if that's what people want. Matching x86 here seems > reasonable, though I'd also say that my gut still says that even for > crash stop we should try to stop things cleanly before jumping to NMI. > I guess I could imagine that the code we're kexec-ing to generate the > core file might be more likely to find the hardware in a funny state > if we stopped CPUs w/ NMI vs IRQ. In terms of the "right thing to do" for kdump then reviewing the s390 might be a good idea. Unfortunately it's a bit different to the other arches and I can't offer a 95% answer about what that arch does. Daniel.
Re: [PATCH] netdev: Use flexible array for trailing private bytes
From: Eric Dumazet
Date: Fri, 1 Mar 2024 09:03:55 +0100
> On Fri, Mar 1, 2024 at 7:59 AM Jakub Kicinski wrote:
>>
>> On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure
we want to add checks there. Maybe under CONFIG_DEBUG_NET?
>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 118c40258d07..b476809d0bae 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
>>> NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
>>> };
>>>
>>> +#define NETDEV_ALIGN32
>>
>> Unless someone knows what this is for it should go.
>> Align priv to cacheline size.
>
> +2
>
Maybe
> #define NETDEV_ALIGNL1_CACHE_BYTES
#define NETDEV_ALIGNmax(SMP_CACHE_BYTES, 32)
?
(or even max(1 << INTERNODE_CACHE_SHIFT, 32))
>
> or a general replacement of NETDEV_ALIGN
>
>
+ I'd align both struct net_device AND its private space to
%NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures
natural alignment of allocations for at least a couple years already
(IOW if struct net_device is aligned to 64, {k,v}malloc will *always*
return a 64-byte aligned address).
Thanks,
Olek
Re: [PATCH] netdev: Use flexible array for trailing private bytes
On Fri, Mar 1, 2024 at 1:59 PM Alexander Lobakin
wrote:
>
> From: Eric Dumazet
> Date: Fri, 1 Mar 2024 09:03:55 +0100
>
> > On Fri, Mar 1, 2024 at 7:59 AM Jakub Kicinski wrote:
> >>
> >> On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
>
> Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure
> we want to add checks there. Maybe under CONFIG_DEBUG_NET?
>
> >
> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>> index 118c40258d07..b476809d0bae 100644
> >>> --- a/include/linux/netdevice.h
> >>> +++ b/include/linux/netdevice.h
> >>> @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
> >>> NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
> >>> };
> >>>
> >>> +#define NETDEV_ALIGN32
> >>
> >> Unless someone knows what this is for it should go.
> >> Align priv to cacheline size.
> >
> > +2
> >
>
> Maybe
>
> > #define NETDEV_ALIGNL1_CACHE_BYTES
>
> #define NETDEV_ALIGNmax(SMP_CACHE_BYTES, 32)
Why would we care if some arches have a very small SMP_CACHE_BYTES ?
Bet it !
IMO nothing in networking mandates this minimal 32 byte alignment.
>
> ?
>
> (or even max(1 << INTERNODE_CACHE_SHIFT, 32))
I do not think so.
INTERNODE_CACHE_SHIFT is a bit extreme on allyesconfig on x86 :/
(with CONFIG_X86_VSMP=y)
>
> >
> > or a general replacement of NETDEV_ALIGN
> >
> >
>
> + I'd align both struct net_device AND its private space to
> %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures
> natural alignment of allocations for at least a couple years already
> (IOW if struct net_device is aligned to 64, {k,v}malloc will *always*
> return a 64-byte aligned address).
I think that with SLAB or SLOB in the past with some DEBUG options
there was no such guarantee.
But this is probably no longer the case, and heavy DEBUG options these
days (KASAN, KFENCE...)
do not expect fast networking anyway.
[PATCH v5 0/4] arm64: qcom: add AIM300 AIoT board suppo
Add AIM300 AIoT support along with usb, ufs, regulators, serial, PCIe,
and PMIC functions.
AIM300 Series is a highly optimized family of modules designed to
support AIoT applications. It integrates QCS8550 SoC, UFS and PMIC
chip etc.
Here is a diagram of AIM300 AIoT Carrie Board and SoM
+--+
| AIM300 AIOT Carrie Board |
| |
| +-+|
|power->| Fixed regulator |-+ |
| +-+ | |
| | |
| v VPH_PWR |
| +--+ |
| | AIM300 SOM || |
| | |VPH_PWR | |
| | v| |
| | +---+ ++ +--+| |
| | | UFS | | QCS8550| |PMIC || |
| | +---+ ++ +--+| |
| | | |
| +--+ |
| |
|++ +--+ |
||USB | | UART | |
|++ +--+ |
+--+
The following functions have been verified:
- uart
- usb
- ufs
- PCIe
- PMIC
- display
- adsp
- cdsp
- tlmm
Documentation for qcs8550[1] and sm8550[2]
[1]
https://docs.qualcomm.com/bundle/publicresource/87-61717-1_REV_A_Qualcomm_QCS8550_QCM8550_Processors_Product_Brief.pdf
[2]
https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/Snapdragon-8-Gen-2-Product-Brief.pdf
Signed-off-by: Tengfei Fan
---
v4 -> v5:
- "2023-2024" instead of "2023~2024" for License
- update patch commit message to previous comments and with an updated
board diagram
- use qcs8550.dtsi instead of qcm8550.dtsi
- remove the reserved memory regions which will be handled by
bootloader
- remove pm8550_flash, pm8550_pwm nodes, Type-C USB/DP function node,
remoteproc_mpss function node, audio sound DTS node, new patch will
be updated after respective team's end to end full verification
- address comments to vph_pwr, move vph_pwr node and related
references to qcs8550-aim300-aiot.dts
- use "regulator-vph-pwr" instead of "vph_pwr_regulator"
- add pcie0I AND pcie1 support together
- the following patches were applied, so remove these patches from new
patch series:
-
https://lore.kernel.org/linux-arm-msm/20240119100621.11788-3-quic_teng...@quicinc.com
-
https://lore.kernel.org/linux-arm-msm/20240119100621.11788-4-quic_teng...@quicinc.com
- verified with dtb check, and result is expected, because those
warnings are not introduced by current patch series.
DTC_CHK arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dtb
arch/arm64/boot/dts/qcom/sm8550.dtsi:3015.27-3070.6: Warning
(avoid_unnecessary_addr_size):
/soc@0/display-subsystem@ae0/dsi@ae96000: unnecessary
#address-cells/#size-cells without "ranges" or child "reg" property
arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dtb:
opp-table: opp-7500:opp-hz:0: [7500, 0, 0, 7500, 0, 0, 0, 0] is
too long
from schema $id: http://devicetree.org/schemas/opp/opp-v2.yaml#
arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dtb:
opp-table: opp-15000:opp-hz:0: [15000, 0, 0, 15000, 0, 0, 0, 0]
is too long
from schema $id: http://devicetree.org/schemas/opp/opp-v2.yaml#
arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dtb:
opp-table: opp-3:opp-hz:0: [3, 0, 0, 3, 0, 0, 0, 0]
is too long
from schema $id: http://devicetree.org/schemas/opp/opp-v2.yaml#
arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dtb:
opp-table: Unevaluated properties are not allowed ('opp-15000',
'opp-3', 'opp-7500' were unexpected)
from schema $id: http://devicetree.org/schemas/opp/opp-v2.yaml#
v3 -> v4:
- use qcm8550.dtsi instead of qcs8550.dtsi, qcs8550 is a QCS version
of qcm8550, another board with qcm8550 will be added later
- add AIM300 AIoT board string in qcom.yaml file
- add sm8550 and qcm8550 fallback compatible
- add qcm8550 SoC id
- add reserved memory map codes in qcm8550.dtsi
- pm8010 and pmr73d are splited into carrier board DTS file. Because
the regulators which in pm8550, pm8550ve and pm8550vs are present
on the SoM. The regulators which in pm8010 and pmr73d are present
on the carrier board.
- stay VPH_PWR at qcs8550-aim300.dtsi file
VPH_PWR is obtained by vonverting 12v voltage into 3.7 voltage
with a 3.7v buck. VPH_PWR is power supply for regulators in AIM300
SOM. VPH_PWR regulator is defined in AIM300
[PATCH v5 1/4] dt-bindings: arm: qcom: Document QCS8550 SoC and the AIM300 AIoT board
Document QCS8550 SoC and the AIM300 AIoT board bindings. QCS8550 is derived from SM8550. The difference between SM8550 and QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used in IoT scenarios. AIM300 Series is a highly optimized family of modules designed to support AIoT applications. It integrates QCS8550 SoC, UFS and PMIC chip etc. AIM stands for Artificial Intelligence Module. AIoT stands for AI IoT. Signed-off-by: Tengfei Fan --- Documentation/devicetree/bindings/arm/qcom.yaml | 8 1 file changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml index 66beaac60e1d..0ca4333fa8cf 100644 --- a/Documentation/devicetree/bindings/arm/qcom.yaml +++ b/Documentation/devicetree/bindings/arm/qcom.yaml @@ -42,6 +42,7 @@ description: | msm8996 msm8998 qcs404 +qcs8550 qcm2290 qcm6490 qdu1000 @@ -868,6 +869,13 @@ properties: - const: qcom,qcs404-evb - const: qcom,qcs404 + - items: + - enum: + - qcom,qcs8550-aim300-aiot + - const: qcom,qcs8550-aim300 + - const: qcom,qcs8550 + - const: qcom,sm8550 + - items: - enum: - qcom,sa8155p-adp -- 2.17.1
[PATCH v5 2/4] arm64: dts: qcom: qcs8550: introduce qcs8550 dtsi
QCS8550 is derived from SM8550. The differnece between SM8550 and
QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
in IoT scenarios.
QCS8550 firmware has different memory map with SM8550 firmware. The
memory map will be runtime added through bootloader.
There are 3 types of reserved memory regions here:
1. Firmware related regions which aren't shared with kernel.
The device tree source in kernel doesn't need to have node to indicate
the firmware related reserved information. OS bootloader conveys the
information by update device tree in runtime.
This will be described as: UEFI saves the physical address of the
UEFI System Table to dts file's chosen node. Kernel read this table and
add reserved memory regions to efi config table. Current reserved memory
region may have reserved region which was not yet used, release note of
the firmware have such kind of information.
2. Firmware related memory regions which are shared with Kernel
Each region has a specific node with specific label name for later
phandle reference from other driver dt node.
3. PIL regions.
PIL regions will be reserved and then assigned to subsystem firmware
later.
Here is a reserved memory map for this platform:
0x1 +--+
| |
| Firmware Related |
| |
0xd4d0 +--+
| |
| Kernel Available |
| |
0xa700 +--+
| |
|PIL Region|
| |
0x8a80 +--+
| |
| Firmware Related |
| |
0x8000 +--+
Note that:
0xa700..0xA800 is used by bootloader, when kernel boot up,
it is available for kernel usage. This region is not suggested to be
used by kernel features like ramoops, suspend resume etc.
Signed-off-by: Tengfei Fan
---
arch/arm64/boot/dts/qcom/qcs8550.dtsi | 169 ++
1 file changed, 169 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
new file mode 100644
index ..a3ebf3d4e16d
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved.
+ */
+
+#include "sm8550.dtsi"
+
+/delete-node/ &reserved_memory;
+
+/ {
+ reserved_memory: reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+
+ /* These are 3 types of reserved memory regions here:
+* 1. Firmware related regions which aren't shared with kernel.
+* The device tree source in kernel doesn't need to have
node to
+* indicate the firmware related reserved information. OS
bootloader
+* conveys the information by update device tree in runtime.
+* This will be described as: UEFI saves the physical
address of
+* the UEFI System Table to dts file's chosen node. Kernel read
this
+* table and add reserved memory regions to efi config table.
Current
+* reserved memory region may have reserved region which was
not yet
+* used, release note of the firmware have such kind of
information.
+* 2. Firmware related memory regions which are shared with
Kernel.
+* Each region has a specific node with specific label name
for
+* later phandle reference from other driver dt node.
+* 3. PIL regions.
+* PIL regions will be reserved and then assigned to
subsystem
+* firmware later.
+* Here is a reserved memory map for this platform:
+* 0x1 +--+
+* | |
+* | Firmware Related |
+* | |
+* 0xd4d0 +--+
+* | |
+* | Kernel Available |
+* | |
+* 0xa700 +--+
+* | |
+* |PIL Region|
+* | |
+* 0x8a80 +--+
+* | |
+* | Firmware Related |
+* | |
+* 0x8000 +--+
+* Note that:
+* 0xa70
[PATCH v5 3/4] arm64: dts: qcom: add base AIM300 dtsi
AIM300 Series is a highly optimized family of modules designed to
support AIoT applications. It integrates QCS8550 SoC, UFS and PMIC
chip etc.
Here is a diagram of AIM300 SoM:
++
|AIM300 SoM |
||
| +-+ |
| |--->| UFS | |
| |+-+ |
| | |
| | |
3.7v | +-+ |+-+ |
-->| PMIC |->| QCS8550 | |
| +-+ +-+ |
| | |
| | |
| |+-+ |
| |--->| ... | |
| +-+ |
||
++
Co-developed-by: Fenglin Wu
Signed-off-by: Fenglin Wu
Signed-off-by: Tengfei Fan
---
arch/arm64/boot/dts/qcom/qcs8550-aim300.dtsi | 345 +++
1 file changed, 345 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/qcs8550-aim300.dtsi
diff --git a/arch/arm64/boot/dts/qcom/qcs8550-aim300.dtsi
b/arch/arm64/boot/dts/qcom/qcs8550-aim300.dtsi
new file mode 100644
index ..43dde67df136
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/qcs8550-aim300.dtsi
@@ -0,0 +1,345 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved.
+ */
+
+#include
+#include "qcs8550.dtsi"
+#include "pm8550.dtsi"
+#include "pm8550b.dtsi"
+#define PMK8550VE_SID 5
+#include "pm8550ve.dtsi"
+#include "pm8550vs.dtsi"
+#include "pmk8550.dtsi"
+
+&apps_rsc {
+ regulators-0 {
+ compatible = "qcom,pm8550-rpmh-regulators";
+ qcom,pmic-id = "b";
+
+ vdd-l1-l4-l10-supply = <&vreg_s6g_1p86>;
+ vdd-l2-l13-l14-supply = <&vreg_bob1>;
+ vdd-l3-supply = <&vreg_s4g_1p25>;
+ vdd-l5-l16-supply = <&vreg_bob1>;
+ vdd-l6-l7-supply = <&vreg_bob1>;
+ vdd-l8-l9-supply = <&vreg_bob1>;
+ vdd-l11-supply = <&vreg_s4g_1p25>;
+ vdd-l12-supply = <&vreg_s6g_1p86>;
+ vdd-l15-supply = <&vreg_s6g_1p86>;
+ vdd-l17-supply = <&vreg_bob2>;
+
+ vreg_bob1: bob1 {
+ regulator-name = "vreg_bob1";
+ regulator-min-microvolt = <3296000>;
+ regulator-max-microvolt = <396>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_bob2: bob2 {
+ regulator-name = "vreg_bob2";
+ regulator-min-microvolt = <272>;
+ regulator-max-microvolt = <396>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l1b_1p8: ldo1 {
+ regulator-name = "vreg_l1b_1p8";
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <180>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l2b_3p0: ldo2 {
+ regulator-name = "vreg_l2b_3p0";
+ regulator-min-microvolt = <3008000>;
+ regulator-max-microvolt = <3008000>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l5b_3p1: ldo5 {
+ regulator-name = "vreg_l5b_3p1";
+ regulator-min-microvolt = <3104000>;
+ regulator-max-microvolt = <3104000>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l6b_1p8: ldo6 {
+ regulator-name = "vreg_l6b_1p8";
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <3008000>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l7b_1p8: ldo7 {
+ regulator-name = "vreg_l7b_1p8";
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <3008000>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l8b_1p8: ldo8 {
+ regulator-name = "vreg_l8b_1p8";
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <3008000>;
+ regulator-initial-mode = ;
+ };
+
+ vreg_l9b_2p9: ldo9 {
+ regulator-name = "vreg_l9b_2p9";
+
[PATCH v5 4/4] arm64: dts: qcom: aim300: add AIM300 AIoT
Add AIM300 AIoT Carrier board DTS support, including usb, UART, PCIe,
I2C functions support.
Here is a diagram of AIM300 AIoT Carrie Board and SoM
+--+
| AIM300 AIOT Carrie Board |
| |
| +-+|
|power->| Fixed regulator |-+ |
| +-+ | |
| | |
| v VPH_PWR |
| +--+ |
| | AIM300 SOM || |
| | |VPH_PWR | |
| | v| |
| | +---+ ++ +--+| |
| | | UFS | | QCS8550| |PMIC || |
| | +---+ ++ +--+| |
| | | |
| +--+ |
| |
|++ +--+ |
||USB | | UART | |
|++ +--+ |
+--+
Co-developed-by: Qiang Yu
Signed-off-by: Qiang Yu
Co-developed-by: Ziyue Zhang
Signed-off-by: Ziyue Zhang
Signed-off-by: Tengfei Fan
---
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../boot/dts/qcom/qcs8550-aim300-aiot.dts | 384 ++
2 files changed, 385 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
diff --git a/arch/arm64/boot/dts/qcom/Makefile
b/arch/arm64/boot/dts/qcom/Makefile
index 7d40ec5e7d21..02d9bc3bfce7 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -96,6 +96,7 @@ dtb-$(CONFIG_ARCH_QCOM) += qcm6490-idp.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qcs404-evb-1000.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qcs404-evb-4000.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qcs6490-rb3gen2.dtb
+dtb-$(CONFIG_ARCH_QCOM)+= qcs8550-aim300-aiot.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qdu1000-idp.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qrb2210-rb1.dtb
dtb-$(CONFIG_ARCH_QCOM)+= qrb4210-rb2.dtb
diff --git a/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
b/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
new file mode 100644
index ..8188766c3d84
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/qcs8550-aim300-aiot.dts
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved.
+ */
+
+/dts-v1/;
+
+#include
+#include "qcs8550-aim300.dtsi"
+#include "pm8010.dtsi"
+#include "pmr735d_a.dtsi"
+#include "pmr735d_b.dtsi"
+
+/ {
+ model = "Qualcomm Technologies, Inc. QCS8550 AIM300 AIOT";
+ compatible = "qcom,qcs8550-aim300-aiot", "qcom,qcs8550-aim300",
"qcom,qcs8550",
+"qcom,sm8550";
+
+ aliases {
+ serial0 = &uart7;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ gpio-keys {
+ compatible = "gpio-keys";
+
+ pinctrl-0 = <&volume_up_n>;
+ pinctrl-names = "default";
+
+ key-volume-up {
+ label = "Volume Up";
+ debounce-interval = <15>;
+ gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
+ linux,code = ;
+ linux,can-disable;
+ wakeup-source;
+ };
+ };
+
+ pmic-glink {
+ compatible = "qcom,sm8550-pmic-glink", "qcom,pmic-glink";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ orientation-gpios = <&tlmm 11 GPIO_ACTIVE_HIGH>;
+
+ connector@0 {
+ compatible = "usb-c-connector";
+ reg = <0>;
+ power-role = "dual";
+ data-role = "dual";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ pmic_glink_hs_in: endpoint {
+ remote-endpoint =
<&usb_1_dwc3_hs>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ pmic_glink_ss_in: endpoint {
+ remote-endpoint =
<&redriver_ss_out>;
+ };
+ };
+
+
Re: [PATCH] netdev: Use flexible array for trailing private bytes
From: Eric Dumazet
Date: Fri, 1 Mar 2024 14:25:37 +0100
> On Fri, Mar 1, 2024 at 1:59 PM Alexander Lobakin
> wrote:
>>
>> From: Eric Dumazet
>> Date: Fri, 1 Mar 2024 09:03:55 +0100
>>
>>> On Fri, Mar 1, 2024 at 7:59 AM Jakub Kicinski wrote:
On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
>>
>> Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure
>> we want to add checks there. Maybe under CONFIG_DEBUG_NET?
>>
>>>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 118c40258d07..b476809d0bae 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
> NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
> };
>
> +#define NETDEV_ALIGN32
Unless someone knows what this is for it should go.
Align priv to cacheline size.
>>>
>>> +2
>>>
>>
>> Maybe
>>
>>> #define NETDEV_ALIGNL1_CACHE_BYTES
>>
>> #define NETDEV_ALIGNmax(SMP_CACHE_BYTES, 32)
>
> Why would we care if some arches have a very small SMP_CACHE_BYTES ?
Oh sorry, I thought %SMP_CACHE_BYTES is 1 when !SMP.
We can then just add cacheline_aligned to both struct net_device and
its ::priv flex array and that's it.
I like the idea of declaring priv explicitly rather than doing size +
ptr magic. But maybe we could just add this flex array to struct
net_device and avoid introducing a new structure.
> Bet it !
>
> IMO nothing in networking mandates this minimal 32 byte alignment.
>
>>
>> ?
>>
>> (or even max(1 << INTERNODE_CACHE_SHIFT, 32))
>
> I do not think so.
>
> INTERNODE_CACHE_SHIFT is a bit extreme on allyesconfig on x86 :/
> (with CONFIG_X86_VSMP=y)
>
>
>>
>>>
>>> or a general replacement of NETDEV_ALIGN
>>>
>>>
>>
>> + I'd align both struct net_device AND its private space to
>> %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures
>> natural alignment of allocations for at least a couple years already
>> (IOW if struct net_device is aligned to 64, {k,v}malloc will *always*
>> return a 64-byte aligned address).
>
> I think that with SLAB or SLOB in the past with some DEBUG options
> there was no such guarantee.
>
> But this is probably no longer the case, and heavy DEBUG options these
> days (KASAN, KFENCE...)
> do not expect fast networking anyway.
Thanks,
Olek
Re: [PATCH] sh: Fix build with CONFIG_UBSAN=y
Hi Yujie, On Fri, 2024-03-01 at 05:46 +, Liu, Yujie wrote: > On Wed, 2024-02-14 at 13:52 +0100, John Paul Adrian Glaubitz wrote: > > Hi Kees, > > > > On Mon, 2024-02-12 at 19:45 +0100, John Paul Adrian Glaubitz wrote: > > > On Mon, 2024-02-12 at 10:26 -0800, Kees Cook wrote: > > > > > I just wanted to try reproduce the problem again with the reproducer > > > > > in [1] as well > > > > > as with gcc-13.2.0, but your branch devel/overflow/ubsan-only no > > > > > longer exists. > > > > > > > > > > Can you tell me where to find the patches now? > > > > > > > > Sure, they're in -next, but for an -rc2 based tree, see: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/kspp > > > > > > OK, thanks. I will give it a try with gcc-13. > > > > I'm still unable to reproduce the error that the kernel test robot reported. > > > > I'm using gcc-13: > > > > glaubitz@node54:/data/home/glaubitz/linux-kees> sh4-linux-gcc > > sh4-linux-gcc: fatal error: no input files > > compilation terminated. > > glaubitz@node54:/data/home/glaubitz/linux-kees> sh4-linux-gcc --version > > sh4-linux-gcc (GCC) 13.2.0 > > Copyright (C) 2023 Free Software Foundation, Inc. > > This is free software; see the source for copying conditions. There is NO > > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > > > glaubitz@node54:/data/home/glaubitz/linux-kees> > > > > I checked out your tree and the for-next/kspp branch. > > > > Then fetched the config that triggered the bug like this: > > > > $ wget > > https://download.01.org/0day-ci/archive/20240131/202401310416.s8hlilnc-...@intel.com/config > > -O .config > > > > Building the kernel with "make -j32 uImage" works fine. No errors except for > > some unreleated warnings that still need to be fixed. > > Sorry for late reply. Seems like the warnings can be reproduced by > "make zImage" which is the default make target, but cannot be > reproduced by "make uImage". > > HEAD is now at 918327e9b7ffb ubsan: Remove CONFIG_UBSAN_SANITIZE_ALL > > $ wget > https://download.01.org/0day-ci/archive/20240131/202401310416.s8hlilnc-...@intel.com/config > -O .config > $ sh4-linux-gcc --version > sh4-linux-gcc (GCC) 13.2.0 > > $ make -j72 CROSS_COMPILE=sh4-linux- ARCH=sh olddefconfig > $ make -j72 CROSS_COMPILE=sh4-linux- ARCH=sh > ... > LD arch/sh/boot/compressed/vmlinux > sh4-linux-ld: arch/sh/boot/compressed/misc.o: in function > `zlib_inflate_table': > misc.c:(.text+0x670): undefined reference to > `__ubsan_handle_shift_out_of_bounds' > sh4-linux-ld: arch/sh/boot/compressed/misc.o: in function `inflate_fast': > misc.c:(.text+0xc5c): undefined reference to > `__ubsan_handle_shift_out_of_bounds' > sh4-linux-ld: arch/sh/boot/compressed/misc.o: in function `zlib_inflateReset': > misc.c:(.text+0xd00): undefined reference to > `__ubsan_handle_shift_out_of_bounds' > sh4-linux-ld: arch/sh/boot/compressed/misc.o: in function `zlib_inflate': > misc.c:(.text+0x23fc): undefined reference to > `__ubsan_handle_shift_out_of_bounds' > sh4-linux-ld: arch/sh/boot/compressed/ashldi3.o: in function `__ashldi3': > ashldi3.c:(.text+0xc8): undefined reference to > `__ubsan_handle_shift_out_of_bounds' > make[3]: *** [arch/sh/boot/compressed/Makefile:38: > arch/sh/boot/compressed/vmlinux] Error 1 > make[2]: *** [arch/sh/boot/Makefile:40: arch/sh/boot/compressed/vmlinux] > Error 2 > make[1]: *** [arch/sh/Makefile:170: zImage] Error 2 > make: *** [Makefile:240: __sub-make] Error 2 > > $ make -j72 CROSS_COMPILE=sh4-linux- ARCH=sh uImage > ... > OBJCOPY arch/sh/boot/vmlinux.bin > GZIParch/sh/boot/vmlinux.bin.gz > UIMAGE arch/sh/boot/uImage.gz > Image Name: Linux-6.8.0-rc2+ > Created: Fri Mar 1 13:31:36 2024 > Image Type: SuperH Linux Kernel Image (gzip compressed) > Data Size:9297141 Bytes = 9079.24 KiB = 8.87 MiB > Load Address: 08001000 > Entry Point: 08002000 > Image arch/sh/boot/uImage is ready Thanks a lot for pointing this out. This explains the problem. I will have another look over the weekend and acknowledge the patch if Kees wants to pick it up through his own tree. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH] arm64: smp: smp_send_stop() and crash_smp_send_stop() should try non-NMI first
Hi Doug,
On Tue, Feb 27, 2024 at 04:57:31PM -0800, Doug Anderson wrote:
> On Mon, Jan 8, 2024 at 4:54 PM Doug Anderson wrote:
> > On Thu, Dec 7, 2023 at 5:03 PM Douglas Anderson
> > wrote:
> > The sound of crickets is overwhelming. ;-) Does anyone have any
> > comments here? Is this a terrible idea? Is this the best idea you've
> > heard all year (it's only been 8 days, so maybe)? Is this great but
> > the implementation is lacking (at best)? Do you hate that this waits
> > for 1 second and wish it waited for 1 ms? 10 ms? 100 ms? 8192 ms?
> >
> > Aside from the weirdness of a processor being killed while holding the
> > console lock, it does seem beneficial to give IRQs at least a little
> > time to finish before killing a processor. I don't have any other
> > explicit examples, but I could just imagine that things might be a
> > little more orderly in such a case...
>
> I'm still hoping to get some sort of feedback here. If people think
> this is a terrible idea then I'll shut up now and leave well enough
> alone, but it would be nice to actively decide and get the patch out
> of limbo.
>
> FWIW the serial console dumping issue that originally inspired me to
> track this down has been worked around at least well enough to not
> spew garbage in my console. See commit 9e957a155005 ("serial:
> qcom-geni: Don't cancel/abort if we can't get the port lock"). It's
> still a little awkward because we'll be running fully lockless during
> panic time, but it seems to work...
This is on my list of things to look into, but I haven't had the chance to go
through it in detail.
>From a high level, I think this sounds reasonable; I just want to make sure
this doesn't lead to any new surprises...
Mark.
Re: [PATCH v1 1/1] lib/string_helpers: Add flags param to string_get_size()
On Thu, Feb 29, 2024 at 04:54:30PM -0500, Kent Overstreet wrote: > On Thu, Feb 29, 2024 at 01:52:34PM -0800, Kees Cook wrote: > > On Thu, 29 Feb 2024 22:52:30 +0200, Andy Shevchenko wrote: > > > The new flags parameter allows controlling > > > - Whether or not the units suffix is separated by a space, for > > >compatibility with sort -h > > > - Whether or not to append a B suffix - we're not always printing > > >bytes. > > > > I cleaned up the Co-developed-by/S-o-b and applied to for-next/hardening, > > thanks! > > thanks Kees! Thank you, folks, glad we close this. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] string: Convert selftest to KUnit
On Fri, Mar 01, 2024 at 01:09:27PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote:
> >
> > Convert test_string.c to KUnit so it can be easily run with everything
> > else.
>
> Have you run it?
Yes:
$ ./tools/testing/kunit/kunit.py run string
[09:21:32] Starting KUnit Kernel (1/1)...
[09:21:32]
[09:21:32] === string (6 subtests)
[09:21:32] [PASSED] test_memset16
[09:21:32] [PASSED] test_memset32
[09:21:32] [PASSED] test_memset64
[09:21:32] [PASSED] test_strchr
[09:21:32] [PASSED] test_strnchr
[09:21:32] [PASSED] test_strspn
[09:21:32] = [PASSED] string ==
[09:21:32]
[09:21:32] Testing complete. Ran 6 tests: passed: 6
[09:21:32] Elapsed time: 11.545s total, 0.001s configuring, 11.327s building,
0.183s running
> ...
>
> > if (i < 256)
> > - return (i << 24) | (j << 16) | k | 0x8000;
> > - return 0;
> > + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k |
> > 0x8000);
>
> First of all, this special value encodes the problematic patterns, so
> you missed proper messaging.
Yeah, I see now this isn't a test but rather an encoded report. Since
the failures are caught earlier, I can improve those messages instead of
doing an encoded version.
> Second, the returned value has a constant, how do you expect 0 to be
> equal to something (guaranteed not to be 0)?
>
> This needs a good rethink of what you should do in the KUnit approach.
>
> ...
>
> > + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k |
> > 0x8000);
>
> Ditto.
>
> ...
>
> > + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k |
> > 0x8000);
>
> Ditto.
>
> ...
>
> > for (i = 0; i < strlen(test_string) + 1; i++) {
> > result = strchr(test_string, test_string[i]);
> > - if (result - test_string != i)
> > - return i + 'a';
> > + KUNIT_ASSERT_EQ(test, result - test_string, i);
>
> In a similar way, all returned values are *special*, you really need
> to think about them before converting to a simple (and sometimes
> wrong) checks)
This encoding is trying to report "i", so I've adjusted the error
reporting in v3.
> I dunno if KUnit has a fault ejection simulation. It should, in order
> to be sure that test cases are fine when they fail.
Yeah, bumping offsets and such produce expected failures.
--
Kees Cook
Re: [PATCH v2 1/2] string: Convert selftest to KUnit
On Fri, Mar 01, 2024 at 09:25:07AM -0800, Kees Cook wrote: > On Fri, Mar 01, 2024 at 01:09:27PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote: > > > > > > Convert test_string.c to KUnit so it can be easily run with everything > > > else. > > > > Have you run it? > > Yes: Of course I new the answer :-) The problem as I described is the (absence of) fault injection in KUnit itself. When tests are passed the picture is nice, when they are not, the reports become a disaster (if this is applied). -- With Best Regards, Andy Shevchenko
Re: [PATCH] netdev: Use flexible array for trailing private bytes
On Fri, 1 Mar 2024 15:30:03 +0100 Alexander Lobakin wrote: > I like the idea of declaring priv explicitly rather than doing size + > ptr magic. But maybe we could just add this flex array to struct > net_device and avoid introducing a new structure. 100% I should have linked to the thread that led to Kees's work. Adding directly to net_device would be way better but there's a handful of drivers which embed the struct. If we can switch them to dynamic allocation, that'd be great. And, as you may be alluding to, it removes the need for the WARN_ON() entirely as well.
[PATCH][next] RDMA/uverbs: Avoid -Wflex-array-member-not-at-end warnings
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
There are currently a couple of objects (`alloc_head` and `bundle`) in
`struct bundle_priv` that contain a couple of flexible structures:
struct bundle_priv {
/* Must be first */
struct bundle_alloc_head alloc_head;
...
/*
* Must be last. bundle ends in a flex array which overlaps
* internal_buffer.
*/
struct uverbs_attr_bundle bundle;
u64 internal_buffer[32];
};
So, in order to avoid ending up with a couple of flexible-array members
in the middle of a struct, we use the `struct_group_tagged()` helper to
separate the flexible array from the rest of the members in the flexible
structures:
struct uverbs_attr_bundle {
struct_group_tagged(uverbs_attr_bundle_hdr, hdr,
... the rest of the members
);
struct uverbs_attr attrs[];
};
With the change described above, we now declare objects of the type of
the tagged struct without embedding flexible arrays in the middle of
another struct:
struct bundle_priv {
/* Must be first */
struct bundle_alloc_head_hdr alloc_head;
...
struct uverbs_attr_bundle_hdr bundle;
u64 internal_buffer[32];
};
We also use `container_of()` whenever we need to retrieve a pointer
to the flexible structures.
Notice that the `bundle_size` computed in `uapi_compute_bundle_size()`
remains the same.
So, with these changes, fix the following warnings:
drivers/infiniband/core/uverbs_ioctl.c:45:34: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
45 | struct bundle_alloc_head alloc_head;
| ^~
drivers/infiniband/core/uverbs_ioctl.c:67:35: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
67 | struct uverbs_attr_bundle bundle;
| ^~
Signed-off-by: Gustavo A. R. Silva
---
drivers/infiniband/core/uverbs_ioctl.c | 78 +++---
include/rdma/uverbs_ioctl.h| 14 +++--
2 files changed, 53 insertions(+), 39 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_ioctl.c
b/drivers/infiniband/core/uverbs_ioctl.c
index d9799706c58e..f80da6a67e24 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -36,13 +36,15 @@
#include "uverbs.h"
struct bundle_alloc_head {
- struct bundle_alloc_head *next;
+ struct_group_tagged(bundle_alloc_head_hdr, hdr,
+ struct bundle_alloc_head *next;
+ );
u8 data[];
};
struct bundle_priv {
/* Must be first */
- struct bundle_alloc_head alloc_head;
+ struct bundle_alloc_head_hdr alloc_head;
struct bundle_alloc_head *allocated_mem;
size_t internal_avail;
size_t internal_used;
@@ -64,7 +66,7 @@ struct bundle_priv {
* Must be last. bundle ends in a flex array which overlaps
* internal_buffer.
*/
- struct uverbs_attr_bundle bundle;
+ struct uverbs_attr_bundle_hdr bundle;
u64 internal_buffer[32];
};
@@ -77,9 +79,10 @@ void uapi_compute_bundle_size(struct uverbs_api_ioctl_method
*method_elm,
unsigned int num_attrs)
{
struct bundle_priv *pbundle;
+ struct uverbs_attr_bundle *bundle;
size_t bundle_size =
offsetof(struct bundle_priv, internal_buffer) +
- sizeof(*pbundle->bundle.attrs) * method_elm->key_bitmap_len +
+ sizeof(*bundle->attrs) * method_elm->key_bitmap_len +
sizeof(*pbundle->uattrs) * num_attrs;
method_elm->use_stack = bundle_size <= sizeof(*pbundle);
@@ -107,7 +110,7 @@ __malloc void *_uverbs_alloc(struct uverbs_attr_bundle
*bundle, size_t size,
gfp_t flags)
{
struct bundle_priv *pbundle =
- container_of(bundle, struct bundle_priv, bundle);
+ container_of(&bundle->hdr, struct bundle_priv, bundle);
size_t new_used;
void *res;
@@ -149,7 +152,7 @@ static int uverbs_set_output(const struct
uverbs_attr_bundle *bundle,
const struct uverbs_attr *attr)
{
struct bundle_priv *pbundle =
- container_of(bundle, struct bundle_priv, bundle);
+ container_of(&bundle->hdr, struct bundle_priv, bundle);
u16 flags;
flags = pbundle->uattrs[attr->ptr_attr.uattr_idx].flags |
@@ -166,6 +169,8 @@ static int uverbs_process_idrs_array(struct bundle_priv
*pbundle,
struct ib_uverbs_attr *uattr,
u32 attr_bkey)
{
+ struct uverbs_attr_bundle *bundle =
+ container_of(&pbundle->bundle, str
[PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
that contain a couple of flexible structures:
struct smc_clc_msg_proposal_area {
...
struct smc_clc_v2_extension pclc_v2_ext;
...
struct smc_clc_smcd_v2_extensionpclc_smcd_v2_ext;
...
};
So, in order to avoid ending up with a couple of flexible-array members
in the middle of a struct, we use the `struct_group_tagged()` helper to
separate the flexible array from the rest of the members in the flexible
structure:
struct smc_clc_smcd_v2_extension {
struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
u8 system_eid[SMC_MAX_EID_LEN];
u8 reserved[16];
);
struct smc_clc_smcd_gid_chid gidchid[];
};
With the change described above, we now declare objects of the type of
the tagged struct without embedding flexible arrays in the middle of
another struct:
struct smc_clc_msg_proposal_area {
...
struct smc_clc_v2_extension_hdr pclc_v2_ext;
...
struct smc_clc_smcd_v2_extension_hdrpclc_smcd_v2_ext;
...
};
We also use `container_of()` when we need to retrieve a pointer to the
flexible structures.
So, with these changes, fix the following warnings:
In file included from net/smc/af_smc.c:42:
net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member
is not at the end of another structure [-Wflex-array-member-not-at-end]
186 | struct smc_clc_v2_extension pclc_v2_ext;
| ^~~
net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member
is not at the end of another structure [-Wflex-array-member-not-at-end]
188 | struct smc_clc_smcd_v2_extensionpclc_smcd_v2_ext;
| ^~~~
Signed-off-by: Gustavo A. R. Silva
---
net/smc/smc_clc.c | 5 +++--
net/smc/smc_clc.h | 24 ++--
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e55026c7529c..3094cfa1c458 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -853,8 +853,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct
smc_init_info *ini)
pclc_smcd = &pclc->pclc_smcd;
pclc_prfx = &pclc->pclc_prfx;
ipv6_prfx = pclc->pclc_prfx_ipv6;
- v2_ext = &pclc->pclc_v2_ext;
- smcd_v2_ext = &pclc->pclc_smcd_v2_ext;
+ v2_ext = container_of(&pclc->pclc_v2_ext, struct smc_clc_v2_extension,
_hdr);
+ smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext,
+ struct smc_clc_smcd_v2_extension, hdr);
gidchids = pclc->pclc_gidchids;
trl = &pclc->pclc_trl;
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 7cc7070b9772..5b91a1947078 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -134,12 +134,14 @@ struct smc_clc_smcd_gid_chid {
*/
struct smc_clc_v2_extension {
- struct smc_clnt_opts_area_hdr hdr;
- u8 roce[16];/* RoCEv2 GID */
- u8 max_conns;
- u8 max_links;
- __be16 feature_mask;
- u8 reserved[12];
+ struct_group_tagged(smc_clc_v2_extension_hdr, _hdr,
+ struct smc_clnt_opts_area_hdr hdr;
+ u8 roce[16];/* RoCEv2 GID */
+ u8 max_conns;
+ u8 max_links;
+ __be16 feature_mask;
+ u8 reserved[12];
+ );
u8 user_eids[][SMC_MAX_EID_LEN];
};
@@ -159,8 +161,10 @@ struct smc_clc_msg_smcd { /* SMC-D GID information */
};
struct smc_clc_smcd_v2_extension {
- u8 system_eid[SMC_MAX_EID_LEN];
- u8 reserved[16];
+ struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
+ u8 system_eid[SMC_MAX_EID_LEN];
+ u8 reserved[16];
+ );
struct smc_clc_smcd_gid_chid gidchid[];
};
@@ -183,9 +187,9 @@ struct smc_clc_msg_proposal_area {
struct smc_clc_msg_smcd pclc_smcd;
struct smc_clc_msg_proposal_prefix pclc_prfx;
struct smc_clc_ipv6_prefix pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX];
- struct smc_clc_v2_extension pclc_v2_ext;
+ struct smc_clc_v2_extension_hdr pclc_v2_ext;
u8 user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
- struct smc_clc_smcd_v2_extensionpclc_smcd_v2_ext;
+ struct smc_clc_smcd_v2_extension_hdrpclc_smcd_v2_ext;
struct smc_clc_smcd_gid_chid
pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
struct smc_clc_msg_trailpclc_trl;
--
2.34.1
Re: [PATCH v1 1/8] kunit: Run tests when the kernel is fully setup
On Fri, Mar 01, 2024 at 03:14:49PM +0800, David Gow wrote:
> On Fri, 1 Mar 2024 at 01:04, Mickaël Salaün wrote:
> >
> > Run all the KUnit tests just before the first userspace code is
> > launched. This makes it it possible to write new tests that check the
> > kernel in its final state i.e., with all async __init code called,
> > memory and RCU properly set up, and sysctl boot arguments evaluated.
>
> KUnit has explicit support for running tests which call __init code
> (or are themselves marked __init), so I'm not keen to move _all_ tests
> here.
Makes sense.
>
> That being said, I'm okay with running all of the other tests (ones
> not explicitly marked init) after __init.
I guess we should have to set of tests, the first one (marked with
__init) to run as it is now, and others set to run when the kernel is
fully setup. I guess we should be able to identify the init sections
for KUnit tests and rely on that to create the two sets. I'll work on
that in a separate patch series.
> >
> > The initial motivation is to run hardening tests (e.g. memory
> > protection, Heki's CR-pinning), which require such security protection
> > to be fully setup (e.g. memory marked as read-only).
> >
> > Because the suite set could refer to init data, initialize the suite set
> > with late_initcall(), before kunit_run_all_tests(), if KUnit is built-in
> > and enabled at boot time. To make it more consistent and easier to
> > manage, whatever filters are used or not, always copy test suite entries
> > and free them after all tests are run.
>
> The goal is to allow init data only for suites explicitly marked as
> such, so we should be able to split that along these lines.
>
> And yeah, the filter code is pretty convoluted when it comes to when
> they're allocated, and it does do things like check how the suite set
> was allocated. So we do need to make any changes carefully.
>
> As Kees suggested, I'd like to see any cleanup here as a separate patch.
Right, I'll send a v2 without this kernel init changes.
>
> >
> > Because of the prepare_namespace() call, we need to have a valid root
> > filesystem. To make it simple, let's use tmpfs with an empty root.
> > Teach kunit_kernel.py:LinuxSourceTreeOperations*() about the related
> > kernel boot argument, and add this filesystem to the kunit.py's kernel
> > build requirements.
>
> I think this is a big enough change we'll need to handle it very
> carefully: there are a few places where the fact that KUnit doesn't
> require a root filesystem is documented, and possibly some other
> (non-kunit.py) runners which rely on this.
>
> I don't think that's necessarily a blocker, but we'll definitely want
> to document it well, and make sure users are aware before this lands.
Indeed
>
> >
> > Remove __init and __refdata markers from iov_iter, bitfield, checksum,
> > and the example KUnit tests. Without this change, the kernel tries to
> > execute NX-protected pages (because the pages are deallocated).
>
> We still want to support these, but we should make sure the suites are
> declared with kunit_init_test_suite().
OK
>
> >
> > Tested with:
> > ./tools/testing/kunit/kunit.py run --alltests
> > ./tools/testing/kunit/kunit.py run --alltests --arch x86_64
>
> FYI: This breaks arm64:
> ./tools/testing/kunit/kunit.py run --raw_output --arch arm64
> --cross_compile=aarch64-linux-gnu-
>
> Unable to handle kernel paging request at virtual address 02016f88
> ...
> Call trace:
> kfree+0x30/0x184
> kunit_run_all_tests+0x88/0x154
> kernel_init+0x6c/0x1e0
> ret_from_fork+0x10/0x20
> Code: b25f7be1 aa0003f4 d34cfe73 8b131833 (f9400661)
> ---[ end trace ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
I guess it is realted to this patch, so I'll fix that with a future
series focus on these kernel init changes.
>
>
> >
> > Cc: Alan Maguire
> > Cc: Brendan Higgins
> > Cc: David Gow
> > Cc: Kees Cook
> > Cc: Luis Chamberlain
> > Cc: Marco Pagani
> > Cc: Rae Moar
> > Cc: Shuah Khan
> > Cc: Stephen Boyd
> > Signed-off-by: Mickaël Salaün
> > ---
> > init/main.c | 4 +-
> > lib/bitfield_kunit.c| 8 +--
> > lib/checksum_kunit.c| 2 +-
> > lib/kunit/executor.c| 81 +
> > lib/kunit/kunit-example-test.c | 6 +--
> > lib/kunit_iov_iter.c| 52 +-
> > tools/testing/kunit/kunit_kernel.py | 6 ++-
> > 7 files changed, 96 insertions(+), 63 deletions(-)
> >
> > diff --git a/init/main.c b/init/main.c
> > index e24b0780fdff..b39d74727aad 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -1463,6 +1463,8 @@ static int __ref kernel_init(void *unused)
> >
> > do_sysctl_args();
> >
> > + kunit_run_all_tests();
> > +
> > if (ramdisk_execute_command) {
> > ret = run_init_process(ramdisk_execute_command);
> > if (!ret)
> > @@ -1550,
Re: [PATCH v1 5/8] kunit: Handle test faults
On Thu, Feb 29, 2024 at 10:24:19AM -0800, Kees Cook wrote: > On Thu, Feb 29, 2024 at 06:04:06PM +0100, Mickaël Salaün wrote: > > Previously, when a kernel test thread crashed (e.g. NULL pointer > > dereference, general protection fault), the KUnit test hanged for 30 > > seconds and exited with a timeout error. > > > > Fix this issue by waiting on task_struct->vfork_done instead of the > > custom kunit_try_catch.try_completion, and track the execution state by > > initially setting try_result with -EFAULT and only setting it to 0 if > > the test passed. > > > > Fix kunit_generic_run_threadfn_adapter() signature by returning 0 > > instead of calling kthread_complete_and_exit(). Because thread's exit > > code is never checked, always set it to 0 to make it clear. > > > > Fix the -EINTR error message, which couldn't be reached until now. > > > > This is tested with a following patch. > > > > Cc: Brendan Higgins > > Cc: David Gow > > Cc: Rae Moar > > Cc: Shuah Khan > > Signed-off-by: Mickaël Salaün > > I assume we can start checking for "intentional" faults now? Yes, but adding dedicated exception handling for such faults would probably be cleaner. At least we can now easily write tests as I did with the last patch. The only potential issue is that the kernel will still print the related warning in logs, but I think it's OK for tests (and maybe something we'd like to test too by the way). > > Reviewed-by: Kees Cook > > -- > Kees Cook >
Re: [PATCH v1 8/8] kunit: Add tests for faults
On Thu, Feb 29, 2024 at 10:28:18AM -0800, Kees Cook wrote:
> On Thu, Feb 29, 2024 at 06:04:09PM +0100, Mickaël Salaün wrote:
> > The first test checks NULL pointer dereference and make sure it would
> > result as a failed test.
> >
> > The second and third tests check that read-only data is indeed read-only
> > and trying to modify it would result as a failed test.
> >
> > This kunit_x86_fault test suite is marked as skipped when run on a
> > non-x86 native architecture. It is then skipped on UML because such
> > test would result to a kernel panic.
> >
> > Tested with:
> > ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_x86_fault
> >
> > Cc: Brendan Higgins
> > Cc: David Gow
> > Cc: Rae Moar
> > Cc: Shuah Khan
> > Signed-off-by: Mickaël Salaün
>
> If we can add some way to collect WARN/BUG output for examination, I
> could rewrite most of LKDTM in KUnit! I really like this!
Thanks! About the WARN/BUG examination, I guess the easier way would be
to do in in user space by extending kunit_parser.py.
>
> > ---
> > lib/kunit/kunit-test.c | 115 -
> > 1 file changed, 114 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> > index f7980ef236a3..57d8eff00c66 100644
> > --- a/lib/kunit/kunit-test.c
> > +++ b/lib/kunit/kunit-test.c
> > @@ -10,6 +10,7 @@
> > #include
> >
> > #include
> > +#include
> > #include
> >
> > #include "string-stream.h"
> > @@ -109,6 +110,117 @@ static struct kunit_suite kunit_try_catch_test_suite
> > = {
> > .test_cases = kunit_try_catch_test_cases,
> > };
> >
> > +#ifdef CONFIG_X86
>
> Why is this x86 specific?
Because I didn't test on other architecture, and it looks it crashed on
arm64. :)
I'll test on arm64 and change this condition with !CONFIG_UML.
>
> --
> Kees Cook
>
Re: [PATCH v1 0/8] Run KUnit tests late and handle faults
On Fri, Mar 01, 2024 at 03:15:08PM +0800, David Gow wrote: > On Fri, 1 Mar 2024 at 01:04, Mickaël Salaün wrote: > > > > Hi, > > > > Thanks very much. I think there's a lot going on in this series, and > it'd probably be easier to address if it were broken up a bit more. > > To take things one at a time: > > > This patch series moves KUnit test execution at the very end of kernel > > initialization, just before launching the init process. This opens the > > way to test any kernel code in its normal state (i.e. fully > > initialized). > > I like the general idea here, but there are a few things to keep in mind: > - We can already do this with tests built as modules. > - We have explicit support for testing __init code, so if we want to > keep that (and I think we do), we'll need to make sure that there > remains a way to run tests before __init. > - Behaviour changes here will need to be documented and tested well > across all tests and architectures, so it's not something I'd want to > land quickly. > - The requirement to have a root filesystem set up is another thing > we'll want to handle carefully. > - As-is, the patch seems to break arm64. Fair, I'll remove this patch from the next series. > > > > > This patch series also teaches KUnit to handle kthread faults as errors, > > and it brings a few related fixes and improvements. > > These seem very good overall. I want to look at the last location > stuff in a bit more detail, but otherwise this is okay. Thanks! > > Personally, I'd like to see this split out into a separate series, > partly because I don't want to delay it while we sort the other parts > of this series out, and partly because I have some other changes to > the thread context stuff I think we need to make. I'll do that today. > > > > > New tests check NULL pointer dereference and read-only memory, which > > wasn't possible before. > > These look interesting, but I don't like that they are listed as x86-specific. I was reluctant to make it more broadly available because I only tested on x86... > > > > > This is useful to test current kernel self-protection mechanisms or > > future ones such as Heki: https://github.com/heki-linux > > > > Regards, > > Thanks again. I'll do a more detailed review of the individual patches > next week, but I'm excited to see this overall. Good, you'll review the v2 then. > > Cheers, > -- David > > > > > > Mickaël Salaün (8): > > kunit: Run tests when the kernel is fully setup > > kunit: Handle thread creation error > > kunit: Fix kthread reference > > kunit: Fix timeout message > > kunit: Handle test faults > > kunit: Fix KUNIT_SUCCESS() calls in iov_iter tests > > kunit: Print last test location on fault > > kunit: Add tests for faults > > > > include/kunit/test.h| 24 +- > > include/kunit/try-catch.h | 3 - > > init/main.c | 4 +- > > lib/bitfield_kunit.c| 8 +- > > lib/checksum_kunit.c| 2 +- > > lib/kunit/executor.c| 81 ++-- > > lib/kunit/kunit-example-test.c | 6 +- > > lib/kunit/kunit-test.c | 115 +++- > > lib/kunit/try-catch.c | 33 +--- > > lib/kunit_iov_iter.c| 70 - > > tools/testing/kunit/kunit_kernel.py | 6 +- > > 11 files changed, 261 insertions(+), 91 deletions(-) > > > > > > base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b > > -- > > 2.44.0 > >
[PATCH v2 1/7] kunit: Handle thread creation error
Previously, if a thread creation failed (e.g. -ENOMEM), the function was
called (kunit_catch_run_case or kunit_catch_run_case_cleanup) without
marking the test as failed. Instead, fill try_result with the error
code returned by kthread_run(), which will mark the test as failed and
print "internal error occurred...".
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-2-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
lib/kunit/try-catch.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index f7825991d576..a5cb2ef70a25 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -69,6 +69,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
try_catch,
"kunit_try_catch_thread");
if (IS_ERR(task_struct)) {
+ try_catch->try_result = PTR_ERR(task_struct);
try_catch->catch(try_catch->context);
return;
}
--
2.44.0
[PATCH v2 2/7] kunit: Fix kthread reference
There is a race condition when a kthread finishes after the deadline and
before the call to kthread_stop(), which may lead to use after free.
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-3-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
lib/kunit/try-catch.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index a5cb2ef70a25..73f5007f20ea 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include "try-catch-impl.h"
@@ -65,14 +66,15 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
try_catch->context = context;
try_catch->try_completion = &try_completion;
try_catch->try_result = 0;
- task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
- try_catch,
- "kunit_try_catch_thread");
+ task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
+try_catch, "kunit_try_catch_thread");
if (IS_ERR(task_struct)) {
try_catch->try_result = PTR_ERR(task_struct);
try_catch->catch(try_catch->context);
return;
}
+ get_task_struct(task_struct);
+ wake_up_process(task_struct);
time_remaining = wait_for_completion_timeout(&try_completion,
kunit_test_timeout());
@@ -82,6 +84,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
kthread_stop(task_struct);
}
+ put_task_struct(task_struct);
exit_code = try_catch->try_result;
if (!exit_code)
--
2.44.0
[PATCH v2 3/7] kunit: Fix timeout message
The exit code is always checked, so let's properly handle the -ETIMEDOUT
error code.
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-4-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
lib/kunit/try-catch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index 73f5007f20ea..cab8b24b5d5a 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -79,7 +79,6 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
time_remaining = wait_for_completion_timeout(&try_completion,
kunit_test_timeout());
if (time_remaining == 0) {
- kunit_err(test, "try timed out\n");
try_catch->try_result = -ETIMEDOUT;
kthread_stop(task_struct);
}
@@ -94,6 +93,8 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
try_catch->try_result = 0;
else if (exit_code == -EINTR)
kunit_err(test, "wake_up_process() was never called\n");
+ else if (exit_code == -ETIMEDOUT)
+ kunit_err(test, "try timed out\n");
else if (exit_code)
kunit_err(test, "Unknown error: %d\n", exit_code);
--
2.44.0
[PATCH v2 4/7] kunit: Handle test faults
Previously, when a kernel test thread crashed (e.g. NULL pointer
dereference, general protection fault), the KUnit test hanged for 30
seconds and exited with a timeout error.
Fix this issue by waiting on task_struct->vfork_done instead of the
custom kunit_try_catch.try_completion, and track the execution state by
initially setting try_result with -EFAULT and only setting it to 0 if
the test passed.
Fix kunit_generic_run_threadfn_adapter() signature by returning 0
instead of calling kthread_complete_and_exit(). Because thread's exit
code is never checked, always set it to 0 to make it clear.
Fix the -EINTR error message, which couldn't be reached until now.
This is tested with a following patch.
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-5-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
include/kunit/try-catch.h | 3 ---
lib/kunit/try-catch.c | 14 +++---
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
index c507dd43119d..7c966a1adbd3 100644
--- a/include/kunit/try-catch.h
+++ b/include/kunit/try-catch.h
@@ -14,13 +14,11 @@
typedef void (*kunit_try_catch_func_t)(void *);
-struct completion;
struct kunit;
/**
* struct kunit_try_catch - provides a generic way to run code which might
fail.
* @test: The test case that is currently being executed.
- * @try_completion: Completion that the control thread waits on while test
runs.
* @try_result: Contains any errno obtained while running test case.
* @try: The function, the test case, to attempt to run.
* @catch: The function called if @try bails out.
@@ -46,7 +44,6 @@ struct kunit;
struct kunit_try_catch {
/* private: internal use only. */
struct kunit *test;
- struct completion *try_completion;
int try_result;
kunit_try_catch_func_t try;
kunit_try_catch_func_t catch;
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index cab8b24b5d5a..c6ee4db0b3bd 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -18,7 +18,7 @@
void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
{
try_catch->try_result = -EFAULT;
- kthread_complete_and_exit(try_catch->try_completion, -EFAULT);
+ kthread_exit(0);
}
EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
@@ -26,9 +26,12 @@ static int kunit_generic_run_threadfn_adapter(void *data)
{
struct kunit_try_catch *try_catch = data;
+ try_catch->try_result = -EINTR;
try_catch->try(try_catch->context);
+ if (try_catch->try_result == -EINTR)
+ try_catch->try_result = 0;
- kthread_complete_and_exit(try_catch->try_completion, 0);
+ return 0;
}
static unsigned long kunit_test_timeout(void)
@@ -58,13 +61,11 @@ static unsigned long kunit_test_timeout(void)
void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
{
- DECLARE_COMPLETION_ONSTACK(try_completion);
struct kunit *test = try_catch->test;
struct task_struct *task_struct;
int exit_code, time_remaining;
try_catch->context = context;
- try_catch->try_completion = &try_completion;
try_catch->try_result = 0;
task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
try_catch, "kunit_try_catch_thread");
@@ -75,8 +76,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
}
get_task_struct(task_struct);
wake_up_process(task_struct);
-
- time_remaining = wait_for_completion_timeout(&try_completion,
+ time_remaining = wait_for_completion_timeout(task_struct->vfork_done,
kunit_test_timeout());
if (time_remaining == 0) {
try_catch->try_result = -ETIMEDOUT;
@@ -92,7 +92,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
void *context)
if (exit_code == -EFAULT)
try_catch->try_result = 0;
else if (exit_code == -EINTR)
- kunit_err(test, "wake_up_process() was never called\n");
+ kunit_err(test, "try faulted\n");
else if (exit_code == -ETIMEDOUT)
kunit_err(test, "try timed out\n");
else if (exit_code)
--
2.44.0
[PATCH v2 0/7] Handle faults in KUnit tests
Hi, This patch series teaches KUnit to handle kthread faults as errors, and it brings a few related fixes and improvements. I removed the previous patch that moved the KUnit test execution at the very end of kernel initialization. We'll address that with a separate series. A new test case check NULL pointer dereference, which wasn't possible before. This is useful to test current kernel self-protection mechanisms or future ones such as Heki: https://github.com/heki-linux Previous version: v1: https://lore.kernel.org/r/20240229170409.365386-1-...@digikod.net Regards, Mickaël Salaün (7): kunit: Handle thread creation error kunit: Fix kthread reference kunit: Fix timeout message kunit: Handle test faults kunit: Fix KUNIT_SUCCESS() calls in iov_iter tests kunit: Print last test location on fault kunit: Add tests for fault include/kunit/test.h | 24 ++--- include/kunit/try-catch.h | 3 --- lib/kunit/kunit-test.c| 45 ++- lib/kunit/try-catch.c | 33 +--- lib/kunit_iov_iter.c | 18 5 files changed, 95 insertions(+), 28 deletions(-) base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b -- 2.44.0
[PATCH v2 5/7] kunit: Fix KUNIT_SUCCESS() calls in iov_iter tests
Fix KUNIT_SUCCESS() calls to pass a test argument.
This is a no-op for now because this macro does nothing, but it will be
required for the next commit.
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-6-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
lib/kunit_iov_iter.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/kunit_iov_iter.c b/lib/kunit_iov_iter.c
index 859b67c4d697..27e0c8ee71d8 100644
--- a/lib/kunit_iov_iter.c
+++ b/lib/kunit_iov_iter.c
@@ -139,7 +139,7 @@ static void __init iov_kunit_copy_to_kvec(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -194,7 +194,7 @@ static void __init iov_kunit_copy_from_kvec(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
struct bvec_test_range {
@@ -302,7 +302,7 @@ static void __init iov_kunit_copy_to_bvec(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -359,7 +359,7 @@ static void __init iov_kunit_copy_from_bvec(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
static void iov_kunit_destroy_xarray(void *data)
@@ -453,7 +453,7 @@ static void __init iov_kunit_copy_to_xarray(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -516,7 +516,7 @@ static void __init iov_kunit_copy_from_xarray(struct kunit
*test)
return;
}
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -596,7 +596,7 @@ static void __init iov_kunit_extract_pages_kvec(struct
kunit *test)
stop:
KUNIT_EXPECT_EQ(test, size, 0);
KUNIT_EXPECT_EQ(test, iter.count, 0);
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -674,7 +674,7 @@ static void __init iov_kunit_extract_pages_bvec(struct
kunit *test)
stop:
KUNIT_EXPECT_EQ(test, size, 0);
KUNIT_EXPECT_EQ(test, iter.count, 0);
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
/*
@@ -753,7 +753,7 @@ static void __init iov_kunit_extract_pages_xarray(struct
kunit *test)
}
stop:
- KUNIT_SUCCEED();
+ KUNIT_SUCCEED(test);
}
static struct kunit_case __refdata iov_kunit_cases[] = {
--
2.44.0
[PATCH v2 7/7] kunit: Add tests for fault
Add a test case to check NULL pointer dereference and make sure it would
result as a failed test.
The full kunit_fault test suite is marked as skipped when run on UML
because it would result to a kernel panic.
Tested with:
./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
./tools/testing/kunit/kunit.py run --arch arm64 \
--cross_compile=aarch64-linux-gnu- kunit_fault
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-8-...@digikod.net
---
Changes since v1:
* Removed the rodata and const test cases for now.
* Replace CONFIG_X86 check with !CONFIG_UML, and remove the "_x86"
references.
---
lib/kunit/kunit-test.c | 45 +-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index f7980ef236a3..0fdca5fffaec 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -109,6 +109,48 @@ static struct kunit_suite kunit_try_catch_test_suite = {
.test_cases = kunit_try_catch_test_cases,
};
+#ifndef CONFIG_UML
+
+static void kunit_test_null_dereference(void *data)
+{
+ struct kunit *test = data;
+ int *null = NULL;
+
+ *null = 0;
+
+ KUNIT_FAIL(test, "This line should never be reached\n");
+}
+
+static void kunit_test_fault_null_dereference(struct kunit *test)
+{
+ struct kunit_try_catch_test_context *ctx = test->priv;
+ struct kunit_try_catch *try_catch = ctx->try_catch;
+
+ kunit_try_catch_init(try_catch,
+test,
+kunit_test_null_dereference,
+kunit_test_catch);
+ kunit_try_catch_run(try_catch, test);
+
+ KUNIT_EXPECT_EQ(test, try_catch->try_result, -EINTR);
+ KUNIT_EXPECT_TRUE(test, ctx->function_called);
+}
+
+#endif /* !CONFIG_UML */
+
+static struct kunit_case kunit_fault_test_cases[] = {
+#ifndef CONFIG_UML
+ KUNIT_CASE(kunit_test_fault_null_dereference),
+#endif /* !CONFIG_UML */
+ {}
+};
+
+static struct kunit_suite kunit_fault_test_suite = {
+ .name = "kunit_fault",
+ .init = kunit_try_catch_test_init,
+ .test_cases = kunit_fault_test_cases,
+};
+
/*
* Context for testing test managed resources
* is_resource_initialized is used to test arbitrary resources
@@ -826,6 +868,7 @@ static struct kunit_suite kunit_current_test_suite = {
kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
&kunit_log_test_suite, &kunit_status_test_suite,
- &kunit_current_test_suite, &kunit_device_test_suite);
+ &kunit_current_test_suite, &kunit_device_test_suite,
+ &kunit_fault_test_suite);
MODULE_LICENSE("GPL v2");
--
2.44.0
[PATCH v2 6/7] kunit: Print last test location on fault
This helps identify the location of test faults.
Cc: Brendan Higgins
Cc: David Gow
Cc: Rae Moar
Cc: Shuah Khan
Reviewed-by: Kees Cook
Signed-off-by: Mickaël Salaün
Link: https://lore.kernel.org/r/20240301194037.532117-7-...@digikod.net
---
Changes since v1:
* Added Kees's Reviewed-by.
---
include/kunit/test.h | 24 +---
lib/kunit/try-catch.c | 10 +++---
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index fcb4a4940ace..f3aa66eb0087 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -301,6 +301,8 @@ struct kunit {
struct list_head resources; /* Protected by lock. */
char status_comment[KUNIT_STATUS_COMMENT_SIZE];
+ /* Saves the last seen test. Useful to help with faults. */
+ struct kunit_loc last_seen;
};
static inline void kunit_set_failure(struct kunit *test)
@@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream
*log, const char *fmt,
#define kunit_err(test, fmt, ...) \
kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+/*
+ * Must be called at the beginning of each KUNIT_*_ASSERTION().
+ * Cf. KUNIT_CURRENT_LOC.
+ */
+#define _KUNIT_SAVE_LOC(test) do {\
+ WRITE_ONCE(test->last_seen.file, __FILE__);\
+ WRITE_ONCE(test->last_seen.line, __LINE__);\
+} while (0)
+
/**
* KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
* @test: The test context object.
@@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream
*log, const char *fmt,
* words, it does nothing and only exists for code clarity. See
* KUNIT_EXPECT_TRUE() for more information.
*/
-#define KUNIT_SUCCEED(test) do {} while (0)
+#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
void __noreturn __kunit_abort(struct kunit *test);
@@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
} while (0)
-#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
+#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {\
+ _KUNIT_SAVE_LOC(test); \
_KUNIT_FAILED(test,\
assert_type, \
kunit_fail_assert, \
kunit_fail_assert_format,\
{}, \
fmt, \
- ##__VA_ARGS__)
+ ##__VA_ARGS__); \
+} while (0)
/**
* KUNIT_FAIL() - Always causes a test to fail when evaluated.
@@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
fmt, \
...) \
do { \
+ _KUNIT_SAVE_LOC(test); \
if (likely(!!(condition_) == !!expected_true_))\
break; \
\
@@ -698,6 +712,7 @@ do {
\
.right_text = #right, \
}; \
\
+ _KUNIT_SAVE_LOC(test); \
if (likely(__left op __right)) \
break; \
\
@@ -758,6 +773,7 @@ do {
\
.right_text = #right, \
}; \
\
+ _KUNIT_SAVE_LOC(test); \
if (likely((__left) && (__right) && (strcmp(__left, __right) op 0))) \
break; \
\
@@ -791,6 +807,7 @@ do {
[PATCH 0/3] spi: axi-spi-engine: small cleanups
This series contains a few small cleanups to the axi-spi-engine driver, mostly suggested from previous reviews. --- David Lechner (3): spi: axi-spi-engine: remove p from struct spi_engine_message_state spi: axi-spi-engine: use __counted_by() attribute spi: axi-spi-engine: use struct_size() macro drivers/spi/spi-axi-spi-engine.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) --- base-commit: bf790d87088a04d5f3a4659e04ff2a5a16eca294 change-id: 20240301-mainline-axi-spi-engine-small-cleanups-cd08b51cb6d4 Best regards, -- David Lechner
[PATCH 1/3] spi: axi-spi-engine: remove p from struct spi_engine_message_state
The program pointer p in struct spi_engine_message_state in the AXI SPI
Engine controller driver was assigned but never read so it can be
removed.
Signed-off-by: David Lechner
---
drivers/spi/spi-axi-spi-engine.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index 6177c1a8d56e..d89f75170c9e 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -82,8 +82,6 @@ struct spi_engine_program {
* struct spi_engine_message_state - SPI engine per-message state
*/
struct spi_engine_message_state {
- /** @p: Instructions for executing this message. */
- struct spi_engine_program *p;
/** @cmd_length: Number of elements in cmd_buf array. */
unsigned cmd_length;
/** @cmd_buf: Array of commands not yet written to CMD FIFO. */
@@ -543,7 +541,6 @@ static int spi_engine_transfer_one_message(struct
spi_controller *host,
/* reinitialize message state for this transfer */
memset(st, 0, sizeof(*st));
- st->p = p;
st->cmd_buf = p->instructions;
st->cmd_length = p->length;
msg->state = st;
--
2.43.2
[PATCH 2/3] spi: axi-spi-engine: use __counted_by() attribute
This adds the __counted_by() attribute to the flex array at the end of
struct spi_engine_program in the AXI SPI Engine controller driver.
Suggested-by: Nuno Sá
Signed-off-by: David Lechner
---
drivers/spi/spi-axi-spi-engine.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index d89f75170c9e..e801eed820df 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -75,7 +75,7 @@
struct spi_engine_program {
unsigned int length;
- uint16_t instructions[];
+ uint16_t instructions[] __counted_by(length);
};
/**
--
2.43.2
[PATCH 3/3] spi: axi-spi-engine: use struct_size() macro
This makes use of the struct_size() macro to calculate the size of the
struct axi_spi_engine when allocating it.
Suggested-by: Christophe JAILLET
Signed-off-by: David Lechner
---
drivers/spi/spi-axi-spi-engine.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index e801eed820df..9646764b0042 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
@@ -501,15 +502,13 @@ static irqreturn_t spi_engine_irq(int irq, void *devid)
static int spi_engine_optimize_message(struct spi_message *msg)
{
struct spi_engine_program p_dry, *p;
- size_t size;
spi_engine_precompile_message(msg);
p_dry.length = 0;
spi_engine_compile_message(msg, true, &p_dry);
- size = sizeof(*p->instructions) * (p_dry.length + 1);
- p = kzalloc(sizeof(*p) + size, GFP_KERNEL);
+ p = kzalloc(struct_size(p, instructions, p_dry.length + 1), GFP_KERNEL);
if (!p)
return -ENOMEM;
--
2.43.2
[PATCH v3 0/2] string: Convert selftests to KUnit
Hi,
I realized the string selftests hadn't been converted to KUnit yet. Do that.
-Kees
v3: retain all text context in failure reporting
v2: https://lore.kernel.org/lkml/20240301002416.it.092-k...@kernel.org/
v1: https://lore.kernel.org/lkml/20240229233432.work.675-k...@kernel.org/
Kees Cook (2):
string: Convert selftest to KUnit
string: Convert helpers selftest to KUnit
MAINTAINERS | 4 +-
lib/Kconfig.debug | 12 +-
lib/Makefile | 4 +-
...tring_helpers.c => string_helpers_kunit.c} | 216 --
lib/{test_string.c => string_kunit.c} | 200 ++--
5 files changed, 178 insertions(+), 258 deletions(-)
rename lib/{test-string_helpers.c => string_helpers_kunit.c} (72%)
rename lib/{test_string.c => string_kunit.c} (46%)
--
2.34.1
[PATCH v3 1/2] string: Convert selftest to KUnit
Convert test_string.c to KUnit so it can be easily run with everything
else.
Additional text context is retained for failure reporting. For example,
when forcing a bad match, we can see the loop counters reported for the
memset() tests:
[09:21:52] # test_memset64: ASSERTION FAILED at lib/string_kunit.c:93
[09:21:52] Expected v == 0xa2a1a1a1a1a1a1a1ULL, but
[09:21:52] v == -6799976246779207263 (0xa1a1a1a1a1a1a1a1)
[09:21:52] 0xa2a1a1a1a1a1a1a1ULL == -6727918652741279327
(0xa2a1a1a1a1a1a1a1)
[09:21:52] i:0 j:0 k:0
[09:21:52] [FAILED] test_memset64
Currently passes without problems:
$ ./tools/testing/kunit/kunit.py run string
...
[09:37:40] Starting KUnit Kernel (1/1)...
[09:37:40]
[09:37:40] === string (6 subtests)
[09:37:40] [PASSED] test_memset16
[09:37:40] [PASSED] test_memset32
[09:37:40] [PASSED] test_memset64
[09:37:40] [PASSED] test_strchr
[09:37:40] [PASSED] test_strnchr
[09:37:40] [PASSED] test_strspn
[09:37:40] = [PASSED] string ==
[09:37:40]
[09:37:40] Testing complete. Ran 6 tests: passed: 6
[09:37:40] Elapsed time: 6.730s total, 0.001s configuring, 6.562s building,
0.131s running
Signed-off-by: Kees Cook
---
Cc: Andy Shevchenko
---
MAINTAINERS | 2 +-
lib/Kconfig.debug | 6 +-
lib/Makefile | 2 +-
lib/{test_string.c => string_kunit.c} | 200 +-
4 files changed, 77 insertions(+), 133 deletions(-)
rename lib/{test_string.c => string_kunit.c} (46%)
diff --git a/MAINTAINERS b/MAINTAINERS
index cd651c4df019..9f1f68cccd6a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8976,9 +8976,9 @@ F:include/linux/string.h
F: include/linux/string_choices.h
F: include/linux/string_helpers.h
F: lib/string.c
+F: lib/string_kunit.c
F: lib/string_helpers.c
F: lib/test-string_helpers.c
-F: lib/test_string.c
F: scripts/coccinelle/api/string_choices.cocci
GENERIC UIO DRIVER FOR PCI DEVICES
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4e2febe3b568..406cdf353488 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2352,8 +2352,10 @@ config ASYNC_RAID6_TEST
config TEST_HEXDUMP
tristate "Test functions located in the hexdump module at runtime"
-config STRING_SELFTEST
- tristate "Test string functions at runtime"
+config STRING_KUNIT_TEST
+ tristate "KUnit test string functions at runtime" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
config TEST_STRING_HELPERS
tristate "Test functions located in the string_helpers module at
runtime"
diff --git a/lib/Makefile b/lib/Makefile
index eae87c41b22b..946277c37831 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -49,7 +49,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
percpu-refcount.o rhashtable.o base64.o \
once.o refcount.o rcuref.o usercopy.o errseq.o bucket_locks.o \
generic-radix-tree.o bitmap-str.o
-obj-$(CONFIG_STRING_SELFTEST) += test_string.o
+obj-$(CONFIG_STRING_KUNIT_TEST) += string_kunit.o
obj-y += string_helpers.o
obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
obj-y += hexdump.o
diff --git a/lib/test_string.c b/lib/string_kunit.c
similarity index 46%
rename from lib/test_string.c
rename to lib/string_kunit.c
index c5cb92fb710e..15551200d858 100644
--- a/lib/test_string.c
+++ b/lib/string_kunit.c
@@ -1,17 +1,23 @@
// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test cases for string functions.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
#include
#include
#include
#include
-static __init int memset16_selftest(void)
+static void test_memset16(struct kunit *test)
{
unsigned i, j, k;
u16 v, *p;
- p = kmalloc(256 * 2 * 2, GFP_KERNEL);
- if (!p)
- return -1;
+ p = kunit_kzalloc(test, 256 * 2 * 2, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
for (i = 0; i < 256; i++) {
for (j = 0; j < 256; j++) {
@@ -20,34 +26,27 @@ static __init int memset16_selftest(void)
for (k = 0; k < 512; k++) {
v = p[k];
if (k < i) {
- if (v != 0xa1a1)
- goto fail;
+ KUNIT_ASSERT_EQ_MSG(test, v, 0xa1a1,
+ "i:%d j:%d k:%d", i, j, k);
} else if (k < i + j) {
- if (v != 0xb1b2)
- goto fail;
+ KUNIT_ASSERT_EQ_MSG(test, v, 0xb1b2,
+
[PATCH v3 2/2] string: Convert helpers selftest to KUnit
Convert test-string_helpers.c to KUnit so it can be easily run with
everything else.
Failure reporting doesn't need to be open-coded in most places, for
example, forcing a failure in the expected output for upper/lower
testing looks like this:
[12:18:43] # test_upper_lower: EXPECTATION FAILED at
lib/string_helpers_kunit.c:579
[12:18:43] Expected dst == strings_upper[i].out, but
[12:18:43] dst == "ABCDEFGH1234567890TEST"
[12:18:43] strings_upper[i].out == "ABCDEFGH1234567890TeST"
[12:18:43] [FAILED] test_upper_lower
Currently passes without problems:
$ ./tools/testing/kunit/kunit.py run string_helpers
...
[12:23:55] Starting KUnit Kernel (1/1)...
[12:23:55]
[12:23:55] === string_helpers (3 subtests)
[12:23:55] [PASSED] test_get_size
[12:23:55] [PASSED] test_upper_lower
[12:23:55] [PASSED] test_unescape
[12:23:55] = [PASSED] string_helpers ==
[12:23:55]
[12:23:55] Testing complete. Ran 3 tests: passed: 3
[12:23:55] Elapsed time: 6.709s total, 0.001s configuring, 6.591s building,
0.066s running
Signed-off-by: Kees Cook
---
Cc: Andy Shevchenko
---
MAINTAINERS | 2 +-
lib/Kconfig.debug | 6 +-
lib/Makefile | 2 +-
...tring_helpers.c => string_helpers_kunit.c} | 216 --
4 files changed, 101 insertions(+), 125 deletions(-)
rename lib/{test-string_helpers.c => string_helpers_kunit.c} (72%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9f1f68cccd6a..f3f26d2d4ffb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8978,7 +8978,7 @@ F:include/linux/string_helpers.h
F: lib/string.c
F: lib/string_kunit.c
F: lib/string_helpers.c
-F: lib/test-string_helpers.c
+F: lib/string_helpers_kunit.c
F: scripts/coccinelle/api/string_choices.cocci
GENERIC UIO DRIVER FOR PCI DEVICES
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 406cdf353488..5429e6f170f3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2357,8 +2357,10 @@ config STRING_KUNIT_TEST
depends on KUNIT
default KUNIT_ALL_TESTS
-config TEST_STRING_HELPERS
- tristate "Test functions located in the string_helpers module at
runtime"
+config STRING_HELPERS_KUNIT_TEST
+ tristate "KUnit test string helpers at runtime" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
config TEST_KSTRTOX
tristate "Test kstrto*() family of functions at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index 946277c37831..97c42e38046f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -51,7 +51,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
generic-radix-tree.o bitmap-str.o
obj-$(CONFIG_STRING_KUNIT_TEST) += string_kunit.o
obj-y += string_helpers.o
-obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
+obj-$(CONFIG_STRING_HELPERS_KUNIT_TEST) += string_helpers_kunit.o
obj-y += hexdump.o
obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
obj-y += kstrtox.o
diff --git a/lib/test-string_helpers.c b/lib/string_helpers_kunit.c
similarity index 72%
rename from lib/test-string_helpers.c
rename to lib/string_helpers_kunit.c
index dce67698297b..f88e39fd68d6 100644
--- a/lib/test-string_helpers.c
+++ b/lib/string_helpers_kunit.c
@@ -1,35 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
/*
* Test cases for lib/string_helpers.c module.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include
#include
-#include
#include
-#include
-#include
#include
#include
#include
-static __init bool test_string_check_buf(const char *name, unsigned int flags,
-char *in, size_t p,
-char *out_real, size_t q_real,
-char *out_test, size_t q_test)
+static void test_string_check_buf(struct kunit *test,
+ const char *name, unsigned int flags,
+ char *in, size_t p,
+ char *out_real, size_t q_real,
+ char *out_test, size_t q_test)
{
- if (q_real == q_test && !memcmp(out_test, out_real, q_test))
- return true;
-
- pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
-
- print_hex_dump(KERN_WARNING, "Input: ", DUMP_PREFIX_NONE, 16, 1,
- in, p, true);
- print_hex_dump(KERN_WARNING, "Expected: ", DUMP_PREFIX_NONE, 16, 1,
- out_test, q_test, true);
- print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
- out_real, q_real, true);
-
- return false;
+ KUNIT_ASSERT_EQ_MSG(test, q_real, q_test, "name:%s", name);
+ KUNIT_EXPECT_MEMEQ_MSG(test, out_test, out_real, q_test,
Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit
On Fri, Mar 01, 2024 at 01:20:41PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook wrote:
> >
> > Convert test-string_helpers.c to KUnit so it can be easily run with
> > everything else.
>
> ...
>
> > -#include
> > #include
> > +#include
>
> I know the order is broken here, but don't make it worse, please. And
> stick with one schema where to put kunit/test.h always before
> everything else and somewhere else (like always after linux/*).
Fixed.
>
> > +#include
> > #include
> > #include
> > #include
>
> ...
>
> > +static void test_string_check_buf(struct kunit *test,
> > + const char *name, unsigned int flags,
> > + char *in, size_t p,
> > + char *out_real, size_t q_real,
> > + char *out_test, size_t q_test)
> > {
> > - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> > - return true;
> > + int result;
> > +
> > + KUNIT_EXPECT_EQ(test, q_real, q_test);
>
> This needs a message.
>
> > + result = memcmp(out_test, out_real, q_test);
>
> > + KUNIT_EXPECT_EQ(test, 0, result);
>
> Why do we need this assertion? We have a dump below to show what's wrong.
I've improved this all around with ...STREQ and ...MEMEQ calls, and
added _MSG versions where needed to retain the prior reported test
context.
> ...
>
> > +#define test_string_get_size_one(size, blk_size, exp_result10,
> > exp_result2) \
> > + do {
> > \
> > + BUILD_BUG_ON(sizeof(exp_result10) >=
> > string_get_size_maxbuf);\
> > + BUILD_BUG_ON(sizeof(exp_result2) >=
> > string_get_size_maxbuf); \
>
> No analogous assertions in KUnit?
It's designed for runtime testing. This is fine as-is for compile-time
asserts.
Thanks for the reviews!
-Kees
--
Kees Cook
Re: [PATCH 2/3] spi: axi-spi-engine: use __counted_by() attribute
On Fri, Mar 01, 2024 at 02:25:19PM -0600, David Lechner wrote:
> This adds the __counted_by() attribute to the flex array at the end of
> struct spi_engine_program in the AXI SPI Engine controller driver.
>
> Suggested-by: Nuno Sá
> Signed-off-by: David Lechner
> ---
> drivers/spi/spi-axi-spi-engine.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-axi-spi-engine.c
> b/drivers/spi/spi-axi-spi-engine.c
> index d89f75170c9e..e801eed820df 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -75,7 +75,7 @@
>
> struct spi_engine_program {
> unsigned int length;
> - uint16_t instructions[];
> + uint16_t instructions[] __counted_by(length);
> };
You'll also need to change places where you deal with changes to
"length", as now accesses to "instructions" will be bounds-checked
by the compiler. For example, this change:
static void spi_engine_program_add_cmd(struct spi_engine_program *p,
bool dry, uint16_t cmd)
{
p->length++;
if (!dry)
p->instructions[p->length - 1] = cmd;
}
--
Kees Cook
Re: [PATCH 1/3] spi: axi-spi-engine: remove p from struct spi_engine_message_state
On Fri, Mar 01, 2024 at 02:25:18PM -0600, David Lechner wrote: > The program pointer p in struct spi_engine_message_state in the AXI SPI > Engine controller driver was assigned but never read so it can be > removed. > > Signed-off-by: David Lechner Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 8/8] kunit: Add tests for faults
Hi Mickaël, kernel test robot noticed the following build warnings: [auto build test WARNING on d206a76d7d2726f3b096037f2079ce0bd3ba329b] url: https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/kunit-Run-tests-when-the-kernel-is-fully-setup/20240301-011020 base: d206a76d7d2726f3b096037f2079ce0bd3ba329b patch link: https://lore.kernel.org/r/20240229170409.365386-9-mic%40digikod.net patch subject: [PATCH v1 8/8] kunit: Add tests for faults config: x86_64-randconfig-122-20240301 (https://download.01.org/0day-ci/archive/20240302/202403020418.felm-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240302/202403020418.felm-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202403020418.felm-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> lib/kunit/kunit-test.c:142:11: sparse: sparse: symbol 'test_const' was not >> declared. Should it be static? vim +/test_const +142 lib/kunit/kunit-test.c 141 > 142 const int test_const = 1; 143 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH 3/3] spi: axi-spi-engine: use struct_size() macro
On Fri, Mar 01, 2024 at 02:25:20PM -0600, David Lechner wrote: > This makes use of the struct_size() macro to calculate the size of the > struct axi_spi_engine when allocating it. > > Suggested-by: Christophe JAILLET > Signed-off-by: David Lechner Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings
On Fri, Mar 01, 2024 at 12:40:57PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
>
> There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
> that contain a couple of flexible structures:
>
> struct smc_clc_msg_proposal_area {
> ...
> struct smc_clc_v2_extension pclc_v2_ext;
> ...
> struct smc_clc_smcd_v2_extensionpclc_smcd_v2_ext;
> ...
> };
>
> So, in order to avoid ending up with a couple of flexible-array members
> in the middle of a struct, we use the `struct_group_tagged()` helper to
> separate the flexible array from the rest of the members in the flexible
> structure:
>
> struct smc_clc_smcd_v2_extension {
> struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
> u8 system_eid[SMC_MAX_EID_LEN];
> u8 reserved[16];
> );
> struct smc_clc_smcd_gid_chid gidchid[];
> };
>
> With the change described above, we now declare objects of the type of
> the tagged struct without embedding flexible arrays in the middle of
> another struct:
>
> struct smc_clc_msg_proposal_area {
> ...
> struct smc_clc_v2_extension_hdr pclc_v2_ext;
> ...
> struct smc_clc_smcd_v2_extension_hdr pclc_smcd_v2_ext;
> ...
> };
>
> We also use `container_of()` when we need to retrieve a pointer to the
> flexible structures.
>
> So, with these changes, fix the following warnings:
>
> In file included from net/smc/af_smc.c:42:
> net/smc/smc_clc.h:186:49: warning: structure containing a flexible array
> member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 186 | struct smc_clc_v2_extension pclc_v2_ext;
> | ^~~
> net/smc/smc_clc.h:188:49: warning: structure containing a flexible array
> member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 188 | struct smc_clc_smcd_v2_extensionpclc_smcd_v2_ext;
> | ^~~~
>
> Signed-off-by: Gustavo A. R. Silva
I think this is a nice way to deal with these flex-array cases. Using
the struct_group() and container_of() means there is very little
collateral impact. Since this is isolated to a single file, I wonder if
it's easy to check that there are no binary differences too? I wouldn't
expect any -- container_of() is all constant expressions, so the
assignment offsets should all be the same, etc.
Reviewed-by: Kees Cook
-Kees
--
Kees Cook
Re: [PATCH][next] RDMA/uverbs: Avoid -Wflex-array-member-not-at-end warnings
On Fri, Mar 01, 2024 at 12:37:45PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
>
> There are currently a couple of objects (`alloc_head` and `bundle`) in
> `struct bundle_priv` that contain a couple of flexible structures:
>
> struct bundle_priv {
> /* Must be first */
> struct bundle_alloc_head alloc_head;
>
> ...
>
> /*
> * Must be last. bundle ends in a flex array which overlaps
> * internal_buffer.
> */
> struct uverbs_attr_bundle bundle;
> u64 internal_buffer[32];
> };
>
> So, in order to avoid ending up with a couple of flexible-array members
> in the middle of a struct, we use the `struct_group_tagged()` helper to
> separate the flexible array from the rest of the members in the flexible
> structures:
>
> struct uverbs_attr_bundle {
> struct_group_tagged(uverbs_attr_bundle_hdr, hdr,
> ... the rest of the members
> );
> struct uverbs_attr attrs[];
> };
>
> With the change described above, we now declare objects of the type of
> the tagged struct without embedding flexible arrays in the middle of
> another struct:
>
> struct bundle_priv {
> /* Must be first */
> struct bundle_alloc_head_hdr alloc_head;
>
> ...
>
> struct uverbs_attr_bundle_hdr bundle;
> u64 internal_buffer[32];
> };
>
> We also use `container_of()` whenever we need to retrieve a pointer
> to the flexible structures.
>
> Notice that the `bundle_size` computed in `uapi_compute_bundle_size()`
> remains the same.
>
> So, with these changes, fix the following warnings:
>
> drivers/infiniband/core/uverbs_ioctl.c:45:34: warning: structure containing a
> flexible array member is not at the end of another structure
> [-Wflex-array-member-not-at-end]
>45 | struct bundle_alloc_head alloc_head;
> | ^~
> drivers/infiniband/core/uverbs_ioctl.c:67:35: warning: structure containing a
> flexible array member is not at the end of another structure
> [-Wflex-array-member-not-at-end]
>67 | struct uverbs_attr_bundle bundle;
> | ^~
>
> Signed-off-by: Gustavo A. R. Silva
This looks complex, but I think it's simpler that other changes that
would have much more collateral impact. Thanks for figuring out a
workable solution!
Reviewed-by: Kees Cook
--
Kees Cook
