On Tue, Oct 05, 2021 at 09:30:10AM -0700, Kees Cook wrote:
> On Mon, Sep 27, 2021 at 09:37:56AM -0700, Luis Chamberlain wrote:
> > --- /dev/null
> > +++ b/lib/test_sysfs.c
> > @@ -0,0 +1,921 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> > +/*
> > + * sysfs test driver
> > + *
> > + * Copyright (C) 2021 Luis Chamberlain <mcgrof(a)kernel.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the Free
> > + * Software Foundation; either version 2 of the License, or at your option any
> > + * later version; or, when distributed separately from the Linux kernel or
> > + * when incorporated into other software packages, subject to the following
> > + * license:
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of copyleft-next (version 0.3.1 or later) as published
> > + * at http://copyleft-next.org/.
>
> As Greg suggested, please drop the boilerplate here.
Sure, sorry for missing that fixed.
> > +static ssize_t config_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > + struct test_config *config = &test_dev->config;
> > + int len = 0;
> > +
> > + test_dev_config_lock(test_dev);
> > +
> > + len += snprintf(buf, PAGE_SIZE,
> > + "Configuration for: %s\n",
> > + dev_name(dev));
>
> Please use sysfs_emit() instead of snprintf().
Oh nice, done and fixed also in the other places.
> > +static int sysfs_test_dev_alloc_blockdev(struct sysfs_test_device *test_dev)
> > +{
> > + int ret = -ENOMEM;
> > +
> > + test_dev->disk = blk_alloc_disk(NUMA_NO_NODE);
> > + if (!test_dev->disk) {
> > + pr_err("Error allocating disk structure for device %d\n",
> > + test_dev->dev_idx);
> > + goto out;
> > + }
> > +
> > + test_dev->disk->major = sysfs_test_major;
> > + test_dev->disk->first_minor = test_dev->dev_idx + 1;
> > + test_dev->disk->fops = &sysfs_testdev_ops;
> > + test_dev->disk->private_data = test_dev;
> > + snprintf(test_dev->disk->disk_name, 16, "test_sysfs%d",
> > + test_dev->dev_idx);
>
> Prefer sizeof(test_dev->disk->disk_name) over open-coded "16".
Sure.
> > +static ssize_t read_reset_first_test_dev(struct file *file,
> > + char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + ssize_t len;
> > + char buf[32];
> > +
> > + reset_first_test_dev++;
> > + len = sprintf(buf, "%d\n", reset_first_test_dev);
>
> Even though it's safe as-is, I was going to suggest scnprintf() here
> (i.e. explicit bounds and a bounds-checked "len"). However, scnprintf()
> returns ssize_t, and there's no bounds checking in
> simple_read_from_buffer. That needs fixing (I'll send a patch).
OK we can later change it to scnprintf() once your patch gets merged.
> > --- /dev/null
> > +++ b/tools/testing/selftests/sysfs/sysfs.sh
> > @@ -0,0 +1,1208 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (C) 2021 Luis Chamberlain <mcgrof(a)kernel.org>
> > +#
> > +# This program is free software; you can redistribute it and/or modify it
> > +# under the terms of the GNU General Public License as published by the Free
> > +# Software Foundation; either version 2 of the License, or at your option any
> > +# later version; or, when distributed separately from the Linux kernel or
> > +# when incorporated into other software packages, subject to the following
> > +# license:
> > +#
> > +# This program is free software; you can redistribute it and/or modify it
> > +# under the terms of copyleft-next (version 0.3.1 or later) as published
> > +# at http://copyleft-next.org/.
> > +
> > +# This performs a series tests against the sysfs filesystem.
>
> -boilerplate
Nuked.
> > +check_dmesg()
> > +{
> > + # filter out intentional WARNINGs or Oopses
> > + local filter=${1:-_check_dmesg_filter}
> > +
> > + _dmesg_since_test_start | $filter >$seqres.dmesg
> > + egrep -q -e "kernel BUG at" \
> > + -e "WARNING:" \
> > + -e "\bBUG:" \
> > + -e "Oops:" \
> > + -e "possible recursive locking detected" \
> > + -e "Internal error" \
> > + -e "(INFO|ERR): suspicious RCU usage" \
> > + -e "INFO: possible circular locking dependency detected" \
> > + -e "general protection fault:" \
> > + -e "BUG .* remaining" \
> > + -e "UBSAN:" \
> > + $seqres.dmesg
>
> Is just looking for "call trace" sufficient here?
So far from my testing yes. This strategy is also borrowed from fstests
and that's what is done there, and so quite a lot of testing has been
done with that. If we are to consider an enhancement here we should then
also consider an enhancement welcome for fstests.
Luis
On Mon, Oct 11, 2021 at 10:37:42AM -0700, Luis Chamberlain wrote:
> On Tue, Oct 05, 2021 at 09:08:59AM -0700, Kees Cook wrote:
> > On Mon, Sep 27, 2021 at 09:37:54AM -0700, Luis Chamberlain wrote:
> > I can confirm that LICENSES/dual/copyleft-next-0.3.1 matches
> > https://github.com/copyleft-next/copyleft-next/blob/master/Releases/copylef…
> >
> > Reviewed-by: Kees Cook <keescook(a)chromium.org>
> >
> > > + If the Derived Work includes material licensed under the GPL, You may
> > > + instead license the Derived Work under the GPL.
> > > +
> >
> > nit: needless whitespace, though technically the original license
> > includes this too. :)
>
> Indeed, I decided to leave the white space as the original had it too.
> Should I really get rid of the space or keep it?
Probably keep it for 0 diff with original. :)
--
Kees Cook
v3: https://lore.kernel.org/linux-media/20211007150339.28910-1-andriy.shevchenk…
v2: https://lore.kernel.org/linux-media/20211007095129.22037-1-andriy.shevchenk…
The kernel.h is a set of something which is not related to each other
and often used in non-crossed compilation units, especially when drivers
need only one or two macro definitions from it.
Here is the split of container_of(). The goals are the following:
- untwist the dependency hell a bit
- drop kernel.h inclusion where it's only used for container_of()
- speed up C preprocessing.
The build speedup is
1.83% (ccache approach, see v2 cover letter for the details)
0.5% (kcbench approach, see v3 cover letter for the details)
In v4:
- dropped kobject.h change (Greg)
- Cc'ed more people (as per v1)
In v3:
- split patch 2 to more patches (Greg)
- excluded C changes (Herbert, Greg)
- measured with kcbench, see below (Greg)
Andy Shevchenko (7):
kernel.h: Drop unneeded <linux/kernel.h> inclusion from other headers
kernel.h: Split out container_of() and typeof_member() macros
kunit: Replace kernel.h with the necessary inclusions
list.h: Replace kernel.h with the necessary inclusions
llist: Replace kernel.h with the necessary inclusions
plist: Replace kernel.h with the necessary inclusions
media: entity: Replace kernel.h with the necessary inclusions
include/kunit/test.h | 14 ++++++++++++--
include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++
include/linux/kernel.h | 31 +-----------------------------
include/linux/list.h | 6 ++++--
include/linux/llist.h | 4 +++-
include/linux/plist.h | 5 ++++-
include/linux/rwsem.h | 1 -
include/linux/spinlock.h | 1 -
include/media/media-entity.h | 3 ++-
9 files changed, 63 insertions(+), 39 deletions(-)
create mode 100644 include/linux/container_of.h
--
2.33.0
v2: https://lore.kernel.org/linux-media/20211007095129.22037-1-andriy.shevchenk…
The kernel.h is a set of something which is not related to each other
and often used in non-crossed compilation units, especially when drivers
need only one or two macro definitions from it.
Here is the split of container_of(). The goals are the following:
- untwist the dependency hell a bit
- drop kernel.h inclusion where it's only used for container_of()
- speed up C preprocessing.
In v3:
- split patch 2 to more patches (Greg)
- exclude C changes (Herbert, Greg)
- measure with kcbench, see below (Greg)
Cc: Thorsten Leemhuis <regressions(a)leemhuis.info>
People, like Greg KH and Miguel Ojeda, were asking about the latter.
My methodology an testing has been provided in cover letter for v2
(see above) and here is what Greg KH insisted to have which is speedup
of the kernel build.
$ kcbench -i 3 -j 64 -o $O -s $PWD --no-download -m
Processor: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz [88 CPUs]
Cpufreq; Memory: powersave [intel_pstate]; 128823 MiB
Linux running: 5.6.0-2-amd64 [x86_64]
Compiler: gcc (Debian 10.3.0-11) 10.3.0
Linux compiled: 5.15.0-rc4
Config; Environment: allmodconfig; CCACHE_DISABLE="1"
Build command: make vmlinux modules
Filling caches: This might take a while... Done
Run 1 (-j 64): 464.07 seconds / 7.76 kernels/hour [P:6001%]
Run 2 (-j 64): 464.64 seconds / 7.75 kernels/hour [P:6000%]
Run 3 (-j 64): 486.41 seconds / 7.40 kernels/hour [P:5727%]
$ kcbench -i 3 -j 64 -o $O -s $PWD --no-download -m
Processor: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz [88 CPUs]
Cpufreq; Memory: powersave [intel_pstate]; 128823 MiB
Linux running: 5.6.0-2-amd64 [x86_64]
Compiler: gcc (Debian 10.3.0-11) 10.3.0
Linux compiled: 5.15.0-rc4
Config; Environment: allmodconfig; CCACHE_DISABLE="1"
Build command: make vmlinux modules
Filling caches: This might take a while... Done
Run 1 (-j 64): 462.32 seconds / 7.79 kernels/hour [P:6009%]
Run 2 (-j 64): 462.33 seconds / 7.79 kernels/hour [P:6006%]
Run 3 (-j 64): 465.45 seconds / 7.73 kernels/hour [P:5999%]
Median values
464.64 before
462.33 after
Speedup: +0.5%
This supports and in align with my own approach, but shows lower numbers
due to additional big take in the measurements (compilation without ccache).
Andy Shevchenko (7):
kernel.h: Drop unneeded <linux/kernel.h> inclusion from other headers
kernel.h: Split out container_of() and typeof_member() macros
kunit: Replace kernel.h with the necessary inclusions
list.h: Replace kernel.h with the necessary inclusions
llist: Replace kernel.h with the necessary inclusions
plist: Replace kernel.h with the necessary inclusions
media: entity: Replace kernel.h with the necessary inclusions
include/kunit/test.h | 14 ++++++++++++--
include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++
include/linux/kernel.h | 31 +-----------------------------
include/linux/kobject.h | 1 +
include/linux/list.h | 6 ++++--
include/linux/llist.h | 4 +++-
include/linux/plist.h | 5 ++++-
include/linux/rwsem.h | 1 -
include/linux/spinlock.h | 1 -
include/media/media-entity.h | 3 ++-
10 files changed, 64 insertions(+), 39 deletions(-)
create mode 100644 include/linux/container_of.h
--
2.33.0