* [PATCH v4 0/2] kheaders fixes for -rc
From: Joel Fernandes (Google) @ 2019-05-15 21:35 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Adrian Ratiu, Alexei Starovoitov,
Andrew Morton, atishp04, bpf, Brendan Gregg, Brendan Gregg,
dancol, Daniel Borkmann, Dan Williams, dietmar.eggemann, duyuchao,
gregkh, Guenter Roeck, Jonathan Corbet, Karim Yaghmour, Kees Cook,
kernel-team, linux-doc, linux-kselftest, linux-trace-devel,
Manjo Raja Rao, Masahiro Yamada, Masami Hiramatsu,
Michał Gregorczyk, Michal Gregorczyk, Mohammad Husain,
Olof Johansson, qais.yousef, rdunlap, rostedt, Shuah Khan,
Srinivas Ramana, Tamir Carmeli, yhs
Linus, Greg, Masahiro, Here are some simple fixes for the kheaders feature.
Please consider these patches for an rc release. They are based on Linus's
master branch. The only difference between the last series [1] and this one is
I squashed 1/3 and 3/3 and rebased. Thanks!
[1] https://patchwork.kernel.org/cover/10939557/
Joel Fernandes (Google) (2):
kheaders: Move from proc to sysfs
kheaders: Do not regenerate archive if config is not changed
init/Kconfig | 17 +++++----
kernel/Makefile | 4 +--
kernel/{gen_ikh_data.sh => gen_kheaders.sh} | 17 ++++++---
kernel/kheaders.c | 40 +++++++++------------
4 files changed, 38 insertions(+), 40 deletions(-)
rename kernel/{gen_ikh_data.sh => gen_kheaders.sh} (82%)
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply
* [PATCH v4 1/2] kheaders: Move from proc to sysfs
From: Joel Fernandes (Google) @ 2019-05-15 21:35 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Steven Rostedt, Adrian Ratiu,
Alexei Starovoitov, Andrew Morton, atishp04, bpf, Brendan Gregg,
Brendan Gregg, dancol, Daniel Borkmann, Dan Williams,
dietmar.eggemann, duyuchao, gregkh, Guenter Roeck,
Jonathan Corbet, Karim Yaghmour, Kees Cook, kernel-team,
linux-doc, linux-kselftest, linux-trace-devel, Manjo Raja Rao,
Masahiro Yamada, Masami Hiramatsu, Michał Gregorczyk,
Michal Gregorczyk, Mohammad Husain, Olof Johansson, qais.yousef,
rdunlap, Shuah Khan, Srinivas Ramana, Tamir Carmeli, yhs
In-Reply-To: <20190515213552.203737-1-joel@joelfernandes.org>
The kheaders archive consisting of the kernel headers used for compiling
bpf programs is in /proc. However there is concern that moving it here
will make it permanent. Let us move it to /sys/kernel as discussed [1].
[1] https://lore.kernel.org/patchwork/patch/1067310/#1265969
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
This patch applies on top of the previous patch that was applied to the
driver tree:
https://lore.kernel.org/patchwork/patch/1067310/
v2->v3: Fixed sysfs file mode nit (Greg).
v1->v2: Fixed some kconfig nits (Masami).
init/Kconfig | 17 +++++----
kernel/Makefile | 4 +--
kernel/{gen_ikh_data.sh => gen_kheaders.sh} | 2 +-
kernel/kheaders.c | 40 +++++++++------------
4 files changed, 27 insertions(+), 36 deletions(-)
rename kernel/{gen_ikh_data.sh => gen_kheaders.sh} (98%)
diff --git a/init/Kconfig b/init/Kconfig
index 8b9ffe236e4f..16a7540d60c8 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -579,15 +579,14 @@ config IKCONFIG_PROC
This option enables access to the kernel configuration file
through /proc/config.gz.
-config IKHEADERS_PROC
- tristate "Enable kernel header artifacts through /proc/kheaders.tar.xz"
- depends on PROC_FS
- help
- This option enables access to the kernel header and other artifacts that
- are generated during the build process. These can be used to build eBPF
- tracing programs, or similar programs. If you build the headers as a
- module, a module called kheaders.ko is built which can be loaded on-demand
- to get access to the headers.
+config IKHEADERS
+ tristate "Enable kernel headers through /sys/kernel/kheaders.tar.xz"
+ depends on SYSFS
+ help
+ This option enables access to the in-kernel headers that are generated during
+ the build process. These can be used to build eBPF tracing programs,
+ or similar programs. If you build the headers as a module, a module called
+ kheaders.ko is built which can be loaded on-demand to get access to headers.
config LOG_BUF_SHIFT
int "Kernel log buffer size (16 => 64KB, 17 => 128KB)"
diff --git a/kernel/Makefile b/kernel/Makefile
index 33824f0385b3..a8d923b5481b 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -71,7 +71,7 @@ obj-$(CONFIG_UTS_NS) += utsname.o
obj-$(CONFIG_USER_NS) += user_namespace.o
obj-$(CONFIG_PID_NS) += pid_namespace.o
obj-$(CONFIG_IKCONFIG) += configs.o
-obj-$(CONFIG_IKHEADERS_PROC) += kheaders.o
+obj-$(CONFIG_IKHEADERS) += kheaders.o
obj-$(CONFIG_SMP) += stop_machine.o
obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
@@ -127,7 +127,7 @@ $(obj)/config_data.gz: $(KCONFIG_CONFIG) FORCE
$(obj)/kheaders.o: $(obj)/kheaders_data.tar.xz
quiet_cmd_genikh = CHK $(obj)/kheaders_data.tar.xz
-cmd_genikh = $(CONFIG_SHELL) $(srctree)/kernel/gen_ikh_data.sh $@
+cmd_genikh = $(CONFIG_SHELL) $(srctree)/kernel/gen_kheaders.sh $@
$(obj)/kheaders_data.tar.xz: FORCE
$(call cmd,genikh)
diff --git a/kernel/gen_ikh_data.sh b/kernel/gen_kheaders.sh
similarity index 98%
rename from kernel/gen_ikh_data.sh
rename to kernel/gen_kheaders.sh
index 591a94f7b387..581b83534587 100755
--- a/kernel/gen_ikh_data.sh
+++ b/kernel/gen_kheaders.sh
@@ -2,7 +2,7 @@
# SPDX-License-Identifier: GPL-2.0
# This script generates an archive consisting of kernel headers
-# for CONFIG_IKHEADERS_PROC.
+# for CONFIG_IKHEADERS.
set -e
spath="$(dirname "$(readlink -f "$0")")"
kroot="$spath/.."
diff --git a/kernel/kheaders.c b/kernel/kheaders.c
index 70ae6052920d..8f69772af77b 100644
--- a/kernel/kheaders.c
+++ b/kernel/kheaders.c
@@ -8,9 +8,8 @@
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/proc_fs.h>
+#include <linux/kobject.h>
#include <linux/init.h>
-#include <linux/uaccess.h>
/*
* Define kernel_headers_data and kernel_headers_data_end, within which the
@@ -31,39 +30,32 @@ extern char kernel_headers_data;
extern char kernel_headers_data_end;
static ssize_t
-ikheaders_read_current(struct file *file, char __user *buf,
- size_t len, loff_t *offset)
+ikheaders_read(struct file *file, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t off, size_t len)
{
- return simple_read_from_buffer(buf, len, offset,
- &kernel_headers_data,
- &kernel_headers_data_end -
- &kernel_headers_data);
+ memcpy(buf, &kernel_headers_data + off, len);
+ return len;
}
-static const struct file_operations ikheaders_file_ops = {
- .read = ikheaders_read_current,
- .llseek = default_llseek,
+static struct bin_attribute kheaders_attr __ro_after_init = {
+ .attr = {
+ .name = "kheaders.tar.xz",
+ .mode = 0444,
+ },
+ .read = &ikheaders_read,
};
static int __init ikheaders_init(void)
{
- struct proc_dir_entry *entry;
-
- /* create the current headers file */
- entry = proc_create("kheaders.tar.xz", S_IRUGO, NULL,
- &ikheaders_file_ops);
- if (!entry)
- return -ENOMEM;
-
- proc_set_size(entry,
- &kernel_headers_data_end -
- &kernel_headers_data);
- return 0;
+ kheaders_attr.size = (&kernel_headers_data_end -
+ &kernel_headers_data);
+ return sysfs_create_bin_file(kernel_kobj, &kheaders_attr);
}
static void __exit ikheaders_cleanup(void)
{
- remove_proc_entry("kheaders.tar.xz", NULL);
+ sysfs_remove_bin_file(kernel_kobj, &kheaders_attr);
}
module_init(ikheaders_init);
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH v4 2/2] kheaders: Do not regenerate archive if config is not changed
From: Joel Fernandes (Google) @ 2019-05-15 21:35 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Linus Torvalds, Adrian Ratiu,
Alexei Starovoitov, Andrew Morton, atishp04, bpf, Brendan Gregg,
Brendan Gregg, dancol, Daniel Borkmann, Dan Williams,
dietmar.eggemann, duyuchao, gregkh, Guenter Roeck,
Jonathan Corbet, Karim Yaghmour, Kees Cook, kernel-team,
linux-doc, linux-kselftest, linux-trace-devel, Manjo Raja Rao,
Masahiro Yamada, Masami Hiramatsu, Michał Gregorczyk,
Michal Gregorczyk, Mohammad Husain, Olof Johansson, qais.yousef,
rdunlap, rostedt, Shuah Khan, Srinivas Ramana, Tamir Carmeli, yhs
In-Reply-To: <20190515213552.203737-1-joel@joelfernandes.org>
Linus reported an issue that doing an allmodconfig was causing the
kheaders archive to be regenerated even though the config is the same.
This patch fixes the issue by ignoring the config-related header files
for "knowing when to regenerate based on timestamps". Instead, if the
CONFIG_X_Y option really changes, then we there are the
include/config/X/Y.h which will already tells us "if a config really
changed". So we don't really need these files for regeneration detection
anyway, and ignoring them fixes Linus's issue.
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/gen_kheaders.sh | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/kernel/gen_kheaders.sh b/kernel/gen_kheaders.sh
index 581b83534587..9a34e1d9bd7f 100755
--- a/kernel/gen_kheaders.sh
+++ b/kernel/gen_kheaders.sh
@@ -31,9 +31,8 @@ arch/$SRCARCH/include/
# This block is useful for debugging the incremental builds.
# Uncomment it for debugging.
-# iter=1
-# if [ ! -f /tmp/iter ]; then echo 1 > /tmp/iter;
-# else; iter=$(($(cat /tmp/iter) + 1)); fi
+# if [ ! -f /tmp/iter ]; then iter=1; echo 1 > /tmp/iter;
+# else iter=$(($(cat /tmp/iter) + 1)); echo $iter > /tmp/iter; fi
# find $src_file_list -type f | xargs ls -lR > /tmp/src-ls-$iter
# find $obj_file_list -type f | xargs ls -lR > /tmp/obj-ls-$iter
@@ -43,10 +42,18 @@ arch/$SRCARCH/include/
pushd $kroot > /dev/null
src_files_md5="$(find $src_file_list -type f |
grep -v "include/generated/compile.h" |
+ grep -v "include/generated/autoconf.h" |
+ grep -v "include/config/auto.conf" |
+ grep -v "include/config/auto.conf.cmd" |
+ grep -v "include/config/tristate.conf" |
xargs ls -lR | md5sum | cut -d ' ' -f1)"
popd > /dev/null
obj_files_md5="$(find $obj_file_list -type f |
grep -v "include/generated/compile.h" |
+ grep -v "include/generated/autoconf.h" |
+ grep -v "include/config/auto.conf" |
+ grep -v "include/config/auto.conf.cmd" |
+ grep -v "include/config/tristate.conf" |
xargs ls -lR | md5sum | cut -d ' ' -f1)"
if [ -f $tarfile ]; then tarfile_md5="$(md5sum $tarfile | cut -d ' ' -f1)"; fi
@@ -82,7 +89,7 @@ find $cpio_dir -type f -print0 |
tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
-echo "$src_files_md5" > kernel/kheaders.md5
+echo "$src_files_md5" > kernel/kheaders.md5
echo "$obj_files_md5" >> kernel/kheaders.md5
echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver
From: Thirupathaiah Annapureddy @ 2019-05-16 1:00 UTC (permalink / raw)
To: Sumit Garg, Sasha Levin
Cc: Jarkko Sakkinen, peterhuewe@gmx.de, jgg@ziepe.ca, corbet@lwn.net,
Linux Kernel Mailing List, linux-doc@vger.kernel.org,
linux-integrity@vger.kernel.org, Microsoft Linux Kernel List,
Bryan Kelly (CSI)
In-Reply-To: <CAFA6WYM06E0y9o6+CLNPe48spiL=UDEqoGsidMbk1dBa5Rbmkg@mail.gmail.com>
> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Tuesday, May 14, 2019 7:02 PM
> To: Sasha Levin <sashal@kernel.org>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de;
> jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-doc@vger.kernel.org; linux-
> integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> kernel@microsoft.com>; Thirupathaiah Annapureddy <thiruan@microsoft.com>;
> Bryan Kelly (CSI) <bryankel@microsoft.com>
> Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
>
> On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> >
> > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > >> >
> > >> > Changes since v2:
> > >> >
> > >> > - Drop the devicetree bindings patch (we don't add any new ones).
> > >> > - More code cleanups based on Jason Gunthorpe's review.
> > >> >
> > >> > Sasha Levin (2):
> > >> > ftpm: firmware TPM running in TEE
> > >> > ftpm: add documentation for ftpm driver
> > >>
> > >> Ping? Does anyone have any objections to this?
> > >
> > >Sorry I've been on vacation week before last week and last week
> > >I was extremely busy because I had been on vacation. This in
> > >my TODO list. Will look into it tomorrow in detail.
> > >
> > >Apologies for the delay with this!
> >
> > Hi Jarkko,
> >
> > If there aren't any big objections to this, can we get it merged in?
> > We'll be happy to address any comments that come up.
>
> I guess you have missed or ignored this comment [1]. Please address it.
>
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> 2Flkml%2F2019%2F5%2F8%2F11&data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&sd
> ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&reserved=0
>
> -Sumit
Thanks for reviewing and adding comments.
We tried to use TEE bus framework you suggested for fTPM enumeration.
We were not able to pass the TCG Logs collected by the boot loaders.
Currently there are 3 ways to pass TCG Logs based on the code
in drivers/char/tpm/eventlog:
1. ACPI Table
2. EFI Table
3. OF Device node properties
Our ARM system is booting using U-boot and Device Tree.
So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
We needed to use OF device node properties to pass TCG2 Logs.
TEE bus enumeration framework does not work for our use case due to the above.
Is it possible to add flexibility in TEE bus enumeration framework to support
platform specific properties through OF nodes or ACPI?
>
> >
> > --
> > Thanks,
> > Sasha
^ permalink raw reply
* Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
From: Sumit Garg @ 2019-05-16 7:06 UTC (permalink / raw)
To: Thirupathaiah Annapureddy
Cc: Sasha Levin, Jarkko Sakkinen, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, Linux Kernel Mailing List,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI)
In-Reply-To: <CY4PR21MB0279339E8B0A15414C8F9E14BC0A0@CY4PR21MB0279.namprd21.prod.outlook.com>
On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
<thiruan@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Tuesday, May 14, 2019 7:02 PM
> > To: Sasha Levin <sashal@kernel.org>
> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de;
> > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; linux-doc@vger.kernel.org; linux-
> > integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> > kernel@microsoft.com>; Thirupathaiah Annapureddy <thiruan@microsoft.com>;
> > Bryan Kelly (CSI) <bryankel@microsoft.com>
> > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> >
> > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > >
> > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > >> >
> > > >> > Changes since v2:
> > > >> >
> > > >> > - Drop the devicetree bindings patch (we don't add any new ones).
> > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > >> >
> > > >> > Sasha Levin (2):
> > > >> > ftpm: firmware TPM running in TEE
> > > >> > ftpm: add documentation for ftpm driver
> > > >>
> > > >> Ping? Does anyone have any objections to this?
> > > >
> > > >Sorry I've been on vacation week before last week and last week
> > > >I was extremely busy because I had been on vacation. This in
> > > >my TODO list. Will look into it tomorrow in detail.
> > > >
> > > >Apologies for the delay with this!
> > >
> > > Hi Jarkko,
> > >
> > > If there aren't any big objections to this, can we get it merged in?
> > > We'll be happy to address any comments that come up.
> >
> > I guess you have missed or ignored this comment [1]. Please address it.
> >
> > [1]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > 2Flkml%2F2019%2F5%2F8%2F11&data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&sd
> > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&reserved=0
> >
> > -Sumit
>
> Thanks for reviewing and adding comments.
>
> We tried to use TEE bus framework you suggested for fTPM enumeration.
> We were not able to pass the TCG Logs collected by the boot loaders.
>
> Currently there are 3 ways to pass TCG Logs based on the code
> in drivers/char/tpm/eventlog:
>
> 1. ACPI Table
> 2. EFI Table
> 3. OF Device node properties
>
> Our ARM system is booting using U-boot and Device Tree.
> So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> We needed to use OF device node properties to pass TCG2 Logs.
> TEE bus enumeration framework does not work for our use case due to the above.
Firstly let me clarify that this framework is intended to communicate
with TEE based services/devices rather than boot loader. And in this
case fTPM being a TEE based service, so this framework should be used.
>
> Is it possible to add flexibility in TEE bus enumeration framework to support
> platform specific properties through OF nodes or ACPI?
>
As you mentioned above, TCG logs are collected by boot loader. So it
should find a way to pass them to Linux.
How about if boot loader register these TCG logs with fTPM TA which
could be fetched during fTPM driver probe or new api like
tpm_read_log_tee()? This is something similar to what I used in
optee-rng [1] driver to fetch RNG properties.
[1] https://github.com/torvalds/linux/blob/master/drivers/char/hw_random/optee-rng.c#L176
-Sumit
> >
> > >
> > > --
> > > Thanks,
> > > Sasha
^ permalink raw reply
* [PATCH AUTOSEL 5.0 28/34] KVM: fix KVM_CLEAR_DIRTY_LOG for memory slots of unaligned size
From: Sasha Levin @ 2019-05-16 11:39 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Paolo Bonzini, Peter Xu, Sasha Levin, kvm, linux-doc,
linux-kselftest
In-Reply-To: <20190516113932.8348-1-sashal@kernel.org>
From: Paolo Bonzini <pbonzini@redhat.com>
[ Upstream commit 76d58e0f07ec203bbdfcaabd9a9fc10a5a3ed5ea ]
If a memory slot's size is not a multiple of 64 pages (256K), then
the KVM_CLEAR_DIRTY_LOG API is unusable: clearing the final 64 pages
either requires the requested page range to go beyond memslot->npages,
or requires log->num_pages to be unaligned, and kvm_clear_dirty_log_protect
requires log->num_pages to be both in range and aligned.
To allow this case, allow log->num_pages not to be a multiple of 64 if
it ends exactly on the last page of the slot.
Reported-by: Peter Xu <peterx@redhat.com>
Fixes: 98938aa8edd6 ("KVM: validate userspace input in kvm_clear_dirty_log_protect()", 2019-01-02)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
Documentation/virtual/kvm/api.txt | 5 +++--
tools/testing/selftests/kvm/dirty_log_test.c | 9 ++++++---
virt/kvm/kvm_main.c | 7 ++++---
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ba8927c0d45c4..a1b8e6d92298c 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3790,8 +3790,9 @@ The ioctl clears the dirty status of pages in a memory slot, according to
the bitmap that is passed in struct kvm_clear_dirty_log's dirty_bitmap
field. Bit 0 of the bitmap corresponds to page "first_page" in the
memory slot, and num_pages is the size in bits of the input bitmap.
-Both first_page and num_pages must be a multiple of 64. For each bit
-that is set in the input bitmap, the corresponding page is marked "clean"
+first_page must be a multiple of 64; num_pages must also be a multiple of
+64 unless first_page + num_pages is the size of the memory slot. For each
+bit that is set in the input bitmap, the corresponding page is marked "clean"
in KVM's dirty bitmap, and dirty tracking is re-enabled for that page
(for example via write-protection, or by clearing the dirty bit in
a page table entry).
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 4715cfba20dce..93f99c6b7d79e 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -288,8 +288,11 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
#endif
max_gfn = (1ul << (guest_pa_bits - guest_page_shift)) - 1;
guest_page_size = (1ul << guest_page_shift);
- /* 1G of guest page sized pages */
- guest_num_pages = (1ul << (30 - guest_page_shift));
+ /*
+ * A little more than 1G of guest page sized pages. Cover the
+ * case where the size is not aligned to 64 pages.
+ */
+ guest_num_pages = (1ul << (30 - guest_page_shift)) + 3;
host_page_size = getpagesize();
host_num_pages = (guest_num_pages * guest_page_size) / host_page_size +
!!((guest_num_pages * guest_page_size) % host_page_size);
@@ -359,7 +362,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
kvm_vm_get_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap);
#ifdef USE_CLEAR_DIRTY_LOG
kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap, 0,
- DIV_ROUND_UP(host_num_pages, 64) * 64);
+ host_num_pages);
#endif
vm_dirty_log_verify(bmap);
iteration++;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b4f2d892a1d36..c9b102e65a3a7 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1241,7 +1241,7 @@ int kvm_clear_dirty_log_protect(struct kvm *kvm,
if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_USER_MEM_SLOTS)
return -EINVAL;
- if ((log->first_page & 63) || (log->num_pages & 63))
+ if (log->first_page & 63)
return -EINVAL;
slots = __kvm_memslots(kvm, as_id);
@@ -1254,8 +1254,9 @@ int kvm_clear_dirty_log_protect(struct kvm *kvm,
n = kvm_dirty_bitmap_bytes(memslot);
if (log->first_page > memslot->npages ||
- log->num_pages > memslot->npages - log->first_page)
- return -EINVAL;
+ log->num_pages > memslot->npages - log->first_page ||
+ (log->num_pages < memslot->npages - log->first_page && (log->num_pages & 63)))
+ return -EINVAL;
*flush = false;
dirty_bitmap_buffer = kvm_second_dirty_bitmap(memslot);
--
2.20.1
^ permalink raw reply related
* [PATCH 1/4] evm: check hash algorithm passed to init_desc()
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
This patch prevents memory access beyond the evm_tfm array by checking the
validity of the index (hash algorithm) passed to init_desc(). The hash
algorithm can be arbitrarily set if the security.ima xattr type is not
EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_crypto.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index e11564eb645b..82a38e801ee4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
tfm = &hmac_tfm;
algo = evm_hmac;
} else {
+ if (hash_algo >= HASH_ALGO__LAST)
+ return ERR_PTR(-EINVAL);
+
tfm = &evm_tfm[hash_algo];
algo = hash_algo_name[hash_algo];
}
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] evm: reset status in evm_inode_post_setattr()
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
This patch adds a call to evm_reset_status() in evm_inode_post_setattr(),
before security.evm is updated. The same is done in the other
evm_inode_post_* functions.
Fixes: 523b74b16bcbb ("evm: reset EVM status when file attributes change")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..b41c2d8a8834 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -512,8 +512,11 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
if (!evm_key_loaded())
return;
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
+ if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) {
+ evm_reset_status(dentry->d_inode);
+
evm_update_evmxattr(dentry, NULL, NULL, 0);
+ }
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH 3/4] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
Currently, ima_appraise_measurement() ignores the EVM status when
evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
status. The problem is that the EVM status is overwritten with the
appraisal status.
This patch mitigates the issue by selecting signature verification as the
only method allowed for appraisal when EVM is not initialized. Since the
new behavior might break user space, it must be turned on by adding
ima_appraise_req_evm to the kernel command line.
Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
Documentation/admin-guide/kernel-parameters.txt | 3 +++
security/integrity/ima/ima_appraise.c | 12 ++++++++++++
2 files changed, 15 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 52e6fbb042cc..80e1c233656b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1588,6 +1588,9 @@
Format: { "off" | "enforce" | "fix" | "log" }
default: "enforce"
+ ima_appraise_req_evm
+ [IMA] require EVM for appraisal with file digests.
+
ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead.
The builtin appraise policy appraises all files
owned by uid=0.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..a32ed5d7afd1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -33,6 +33,14 @@ static int __init default_appraise_setup(char *str)
__setup("ima_appraise=", default_appraise_setup);
+static bool ima_appraise_req_evm;
+static int __init appraise_req_evm_setup(char *str)
+{
+ ima_appraise_req_evm = true;
+ return 1;
+}
+__setup("ima_appraise_req_evm", appraise_req_evm_setup);
+
/*
* is_ima_appraise_enabled - return appraise status
*
@@ -245,7 +253,11 @@ int ima_appraise_measurement(enum ima_hooks func,
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_PASS_IMMUTABLE:
+ break;
case INTEGRITY_UNKNOWN:
+ if (ima_appraise_req_evm &&
+ xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ goto out;
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
--
2.17.1
^ permalink raw reply related
* [PATCH 4/4] ima: only audit failed appraisal verifications
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
This patch ensures that integrity_audit_msg() is called only when the
status is not INTEGRITY_PASS.
Fixes: 8606404fa555c ("ima: digital signature verification support")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/ima/ima_appraise.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a32ed5d7afd1..f5f4506bcb8e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -359,8 +359,9 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_PASS;
}
- integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ if (status != INTEGRITY_PASS)
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+ filename, op, cause, rc, 0);
} else {
ima_cache_flags(iint, func);
}
--
2.17.1
^ permalink raw reply related
* RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver
From: Thirupathaiah Annapureddy @ 2019-05-16 19:24 UTC (permalink / raw)
To: Sumit Garg
Cc: Sasha Levin, Jarkko Sakkinen, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, Linux Kernel Mailing List,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI)
In-Reply-To: <CAFA6WYMvuF+tAA_GmkVg=FTvuuAhMuM=um7kakq=YARaP8un5Q@mail.gmail.com>
> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Thursday, May 16, 2019 12:06 AM
> To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> <bryankel@microsoft.com>
> Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
>
> On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> <thiruan@microsoft.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > To: Sasha Levin <sashal@kernel.org>
> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> peterhuewe@gmx.de;
> > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > kernel@vger.kernel.org>; linux-doc@vger.kernel.org; linux-
> > > integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> > > kernel@microsoft.com>; Thirupathaiah Annapureddy
> <thiruan@microsoft.com>;
> > > Bryan Kelly (CSI) <bryankel@microsoft.com>
> > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > >
> > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > >> >
> > > > >> > Changes since v2:
> > > > >> >
> > > > >> > - Drop the devicetree bindings patch (we don't add any new
> ones).
> > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > >> >
> > > > >> > Sasha Levin (2):
> > > > >> > ftpm: firmware TPM running in TEE
> > > > >> > ftpm: add documentation for ftpm driver
> > > > >>
> > > > >> Ping? Does anyone have any objections to this?
> > > > >
> > > > >Sorry I've been on vacation week before last week and last week
> > > > >I was extremely busy because I had been on vacation. This in
> > > > >my TODO list. Will look into it tomorrow in detail.
> > > > >
> > > > >Apologies for the delay with this!
> > > >
> > > > Hi Jarkko,
> > > >
> > > > If there aren't any big objections to this, can we get it merged in?
> > > > We'll be happy to address any comments that come up.
> > >
> > > I guess you have missed or ignored this comment [1]. Please address it.
> > >
> > > [1]
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > >
> 2Flkml%2F2019%2F5%2F8%2F11&data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > >
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&sd
> > > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&reserved=0
> > >
> > > -Sumit
> >
> > Thanks for reviewing and adding comments.
> >
> > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > We were not able to pass the TCG Logs collected by the boot loaders.
> >
> > Currently there are 3 ways to pass TCG Logs based on the code
> > in drivers/char/tpm/eventlog:
> >
> > 1. ACPI Table
> > 2. EFI Table
> > 3. OF Device node properties
> >
> > Our ARM system is booting using U-boot and Device Tree.
> > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > We needed to use OF device node properties to pass TCG2 Logs.
> > TEE bus enumeration framework does not work for our use case due to the
> above.
>
> Firstly let me clarify that this framework is intended to communicate
> with TEE based services/devices rather than boot loader. And in this
> case fTPM being a TEE based service, so this framework should be used.
>
It does not work for our use case. We gave enough justification so far.
TEE bus enumeration needs to be flexible to support our use case and
more future use cases.
> >
> > Is it possible to add flexibility in TEE bus enumeration framework to
> support
> > platform specific properties through OF nodes or ACPI?
> >
>
> As you mentioned above, TCG logs are collected by boot loader. So it
> should find a way to pass them to Linux.
>
> How about if boot loader register these TCG logs with fTPM TA which
> could be fetched during fTPM driver probe or new api like
> tpm_read_log_tee()?
And then how does fTPM driver pass TCG Logs to the TPM framework?
It requires changes to the upstream TPM framework to ask the driver
explicitly for the TCG Logs.
Note that this also requires changes to the fTPM TA that has been existing for few years.
Is it not possible to add flexibility in TEE bus enumeration framework to
support platform specific properties through OF nodes or ACPI?
Devices enumerated by buses such as i2c can read platform specific properties.
With this flexibility added, more future use cases through TEE bus.
> This is something similar to what I used in
> optee-rng [1] driver to fetch RNG properties.
>
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> rng.c%23L176&data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> 3006&sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&reserved=
> 0
>
> -Sumit
>
> > >
> > > >
> > > > --
> > > > Thanks,
> > > > Sasha
^ permalink raw reply
* Re: [TRIVIA] Re: [PATCH] docs: Move kref.txt to core-api/kref.rst
From: Tobin C. Harding @ 2019-05-16 23:19 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Thomas Hellstrom, linux-kernel@vger.kernel.org, minyard@acm.org,
linux-doc@vger.kernel.org, Tobin C. Harding
In-Reply-To: <20190515105648.61164eab@lwn.net>
On Wed, May 15, 2019 at 10:56:48AM -0600, Jonathan Corbet wrote:
> On Fri, 10 May 2019 16:45:45 -0400
> "Tobin C. Harding" <me@tobin.cc> wrote:
>
> > I read once that they used 72 characters on punch cards at times because
> > the other 8 characters got mangled for some reason.
>
> Those of use who worked in Fortran understand these things... columns
> 73-80 were ignored by the compiler. The normal use was to put line
> numbers in there to help recovery when you dropped your card deck on the
> floor and had to unshuffle things. A diagonal line drawn across the
> top of deck helped a lot, but it was good to have verification for the
> marginal cases.
>
> Kids today just don't have any culture at all...:)
I was just going through my editors init file and I see I have default
column width for kernel RST files set to 75. Did you tell me that some
time Jon, I vaguely remember?
+1 for the trivia
Cheers,
Tobin.
^ permalink raw reply
* Re: [PATCH 3/4] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Mimi Zohar @ 2019-05-17 0:30 UTC (permalink / raw)
To: Sasha Levin, Roberto Sassu, dmitry.kasatkin
Cc: linux-integrity, linux-doc, stable
In-Reply-To: <20190517001001.9BEF620848@mail.kernel.org>
On Fri, 2019-05-17 at 00:10 +0000, Sasha Levin wrote:
>
> How should we proceed with this patch?
Yikes! This was posted earlier today. I haven't even had a chance to
look at it yet. Similarly for "[PATCH 4/4] ima: only audit failed
appraisal verifications".
Mimi
^ permalink raw reply
* Re: [PATCH v4 01/18] kunit: test: add KUnit test runner core
From: Stephen Boyd @ 2019-05-17 0:35 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190514221711.248228-2-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:16:54)
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> new file mode 100644
> index 0000000000000..e682ea0e1f9a5
> --- /dev/null
> +++ b/include/kunit/test.h
> @@ -0,0 +1,162 @@
[..]
> +/**
> + * struct kunit - represents a running instance of a test.
> + * @priv: for user to store arbitrary data. Commonly used to pass data created
> + * in the init function (see &struct kunit_module).
> + *
> + * Used to store information about the current context under which the test is
> + * running. Most of this data is private and should only be accessed indirectly
> + * via public functions; the one exception is @priv which can be used by the
> + * test writer to store arbitrary data.
> + */
> +struct kunit {
> + void *priv;
> +
> + /* private: internal use only. */
> + const char *name; /* Read only after initialization! */
> + spinlock_t lock; /* Gaurds all mutable test state. */
> + bool success; /* Protected by lock. */
Is this all the spinlock protects? Doesn't seem useful if it's just
protecting access to the variable being set or not because code that
reads it will have a stale view of the value.
> diff --git a/kunit/test.c b/kunit/test.c
> new file mode 100644
> index 0000000000000..86f65ba2bcf92
> --- /dev/null
> +++ b/kunit/test.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Base unit test (KUnit) API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/sched/debug.h>
> +#include <kunit/test.h>
> +
[...]
> +
> +size_t kunit_module_counter = 1;
static?
^ permalink raw reply
* Re: [PATCH v4 02/18] kunit: test: add test resource management API
From: Stephen Boyd @ 2019-05-17 0:38 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190514221711.248228-3-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:16:55)
> diff --git a/kunit/test.c b/kunit/test.c
> index 86f65ba2bcf92..a15e6f8c41582 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
[..]
> +
> +void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
> +{
> + struct kunit_kmalloc_params params;
> + struct kunit_resource *res;
> +
> + params.size = size;
> + params.gfp = gfp;
> +
> + res = kunit_alloc_resource(test,
> + kunit_kmalloc_init,
> + kunit_kmalloc_free,
> + ¶ms);
> +
> + if (res)
> + return res->allocation;
> + else
> + return NULL;
Can be written as
if (res)
return ....
return
and some static analysis tools prefer this.
> +}
> +
> +void kunit_cleanup(struct kunit *test)
> +{
> + struct kunit_resource *resource, *resource_safe;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&test->lock, flags);
Ah ok, test->lock is protecting everything now? Does it need to be a
spinlock, or can it be a mutex?
> + list_for_each_entry_safe(resource,
> + resource_safe,
> + &test->resources,
> + node) {
> + kunit_free_resource(test, resource);
> + }
> + spin_unlock_irqrestore(&test->lock, flags);
> +}
> +
^ permalink raw reply
* Re: [PATCH 3/4] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Sasha Levin @ 2019-05-17 1:07 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, dmitry.kasatkin, linux-integrity, linux-doc,
stable
In-Reply-To: <1558053020.4507.32.camel@linux.ibm.com>
On Thu, May 16, 2019 at 08:30:20PM -0400, Mimi Zohar wrote:
>On Fri, 2019-05-17 at 00:10 +0000, Sasha Levin wrote:
>>
>> How should we proceed with this patch?
>
>Yikes! This was posted earlier today. I haven't even had a chance to
>look at it yet. Similarly for "[PATCH 4/4] ima: only audit failed
>appraisal verifications".
Hi Mimi,
This is just a very early warning, it doesn't mean it's going in -stable
any time soon :)
I find that giving this alert now results in more responses as people
still have this patch + context in their mind. If we sent alerts such as
these before we actually add patches to -stable people tend to respond
less as usually they have moved to work on something else.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
From: Sumit Garg @ 2019-05-17 6:57 UTC (permalink / raw)
To: Thirupathaiah Annapureddy
Cc: Sasha Levin, Jarkko Sakkinen, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, Linux Kernel Mailing List,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI), Rob Herring
In-Reply-To: <CY4PR21MB02790D399645EFCA02FBE124BC0A0@CY4PR21MB0279.namprd21.prod.outlook.com>
+ Rob
On Fri, 17 May 2019 at 00:54, Thirupathaiah Annapureddy
<thiruan@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Thursday, May 16, 2019 12:06 AM
> > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > <bryankel@microsoft.com>
> > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> >
> > On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> > <thiruan@microsoft.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > > To: Sasha Levin <sashal@kernel.org>
> > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> > peterhuewe@gmx.de;
> > > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > > kernel@vger.kernel.org>; linux-doc@vger.kernel.org; linux-
> > > > integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> > > > kernel@microsoft.com>; Thirupathaiah Annapureddy
> > <thiruan@microsoft.com>;
> > > > Bryan Kelly (CSI) <bryankel@microsoft.com>
> > > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > > >
> > > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > > > >
> > > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > > >> >
> > > > > >> > Changes since v2:
> > > > > >> >
> > > > > >> > - Drop the devicetree bindings patch (we don't add any new
> > ones).
> > > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > > >> >
> > > > > >> > Sasha Levin (2):
> > > > > >> > ftpm: firmware TPM running in TEE
> > > > > >> > ftpm: add documentation for ftpm driver
> > > > > >>
> > > > > >> Ping? Does anyone have any objections to this?
> > > > > >
> > > > > >Sorry I've been on vacation week before last week and last week
> > > > > >I was extremely busy because I had been on vacation. This in
> > > > > >my TODO list. Will look into it tomorrow in detail.
> > > > > >
> > > > > >Apologies for the delay with this!
> > > > >
> > > > > Hi Jarkko,
> > > > >
> > > > > If there aren't any big objections to this, can we get it merged in?
> > > > > We'll be happy to address any comments that come up.
> > > >
> > > > I guess you have missed or ignored this comment [1]. Please address it.
> > > >
> > > > [1]
> > > >
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > > >
> > 2Flkml%2F2019%2F5%2F8%2F11&data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > > >
> > 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&sd
> > > > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&reserved=0
> > > >
> > > > -Sumit
> > >
> > > Thanks for reviewing and adding comments.
> > >
> > > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > > We were not able to pass the TCG Logs collected by the boot loaders.
> > >
> > > Currently there are 3 ways to pass TCG Logs based on the code
> > > in drivers/char/tpm/eventlog:
> > >
> > > 1. ACPI Table
> > > 2. EFI Table
> > > 3. OF Device node properties
> > >
> > > Our ARM system is booting using U-boot and Device Tree.
> > > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > > We needed to use OF device node properties to pass TCG2 Logs.
> > > TEE bus enumeration framework does not work for our use case due to the
> > above.
> >
> > Firstly let me clarify that this framework is intended to communicate
> > with TEE based services/devices rather than boot loader. And in this
> > case fTPM being a TEE based service, so this framework should be used.
> >
> It does not work for our use case. We gave enough justification so far.
> TEE bus enumeration needs to be flexible to support our use case and
> more future use cases.
>
TEE based services are virtual devices which could be very well be
aware about the platform and device driver could easily query these
devices for platform specific properties. In case of firmware TPM as a
TEE based service, it could easily store the event logs generated
during PCR extend operations which could be fetched at runtime. But a
real TPM device doesn't possess that storage capability leading to
software managing these event logs.
> > >
> > > Is it possible to add flexibility in TEE bus enumeration framework to
> > support
> > > platform specific properties through OF nodes or ACPI?
> > >
> >
> > As you mentioned above, TCG logs are collected by boot loader. So it
> > should find a way to pass them to Linux.
> >
> > How about if boot loader register these TCG logs with fTPM TA which
> > could be fetched during fTPM driver probe or new api like
> > tpm_read_log_tee()?
>
> And then how does fTPM driver pass TCG Logs to the TPM framework?
> It requires changes to the upstream TPM framework to ask the driver
> explicitly for the TCG Logs.
My suggestion was to add 4th way to pass TCG logs as follows:
--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -95,6 +95,10 @@ static int tpm_read_log(struct tpm_chip *chip)
if (rc != -ENODEV)
return rc;
+ rc = tpm_read_log_tee(chip);
+ if (rc != -ENODEV)
+ return rc;
+
return tpm_read_log_of(chip);
}
--- /dev/null
+++ b/drivers/char/tpm/eventlog/tee.c
@@ -0,0 +1,43 @@
<snip>
+int tpm_read_log_tee(struct tpm_chip *chip)
+{
+ struct tpm_bios_log *log;
+ struct ftpm_tee_private *pvt_data;
+
+ log = &chip->log;
+ if (!strcmp(chip->dev.bus->name, tee_bus_type.name))
+ pvt_data = dev_get_drvdata(chip->dev.parent);
+ else
+ return -ENODEV;
+
+ // Here you could simply do an invoke command to fetch the TCG logs.
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+}
>
> Note that this also requires changes to the fTPM TA that has been existing for few years.
>
That's not a sane reason to avoid implementing generic stuff. As there
could be other fTPM implementations and specific DT nodes for each
which might not be maintainable. BTW, in current implementation also I
don't find DT bindings corresponding to DT node used in this
patch-set.
> Is it not possible to add flexibility in TEE bus enumeration framework to
> support platform specific properties through OF nodes or ACPI?
>
TEE bus framework was designed specifically to avoid OF nodes or ACPI
properties. As devices could be intelligent enough to be queried for
required properties.
> Devices enumerated by buses such as i2c can read platform specific properties.
i2c devices are real hardware which could be platform agnostic, so you
need to have platform specific properties.
-Sumit
> With this flexibility added, more future use cases through TEE bus.
>
>
> > This is something similar to what I used in
> > optee-rng [1] driver to fetch RNG properties.
> >
> > [1]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> > m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> > rng.c%23L176&data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> > 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> > 3006&sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&reserved=
> > 0
> >
> > -Sumit
> >
> > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Sasha
^ permalink raw reply
* Великденски бонуси
From: Radoslav Dobrev @ 2019-05-17 7:30 UTC (permalink / raw)
To: linux-doc
Здравейте!
Нуждаете ли се от мотивационен пакет за персонала, който е удобен и привлекателен, както за работодателя, така и за служителите?
В такъв случай Ви препоръчваме да обмислите използването на все по-популярните ваучери за храна - работодателят осигурява ваучери за определена сума, а служителят може да я използва в различни вериги хранителни магазини или заведения за хранене според своите предпочитания.
Ще се радвам да Ви представя възможностите на ваучерите – мога ли да Ви се обадя, за да обсъдим в детайли?
Радослав Добрев
Head of HR Benefit Team
www.lunchkarty.eu
^ permalink raw reply
* Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE
From: Sasha Levin @ 2019-05-17 13:22 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel
In-Reply-To: <20190515081250.GA7708@linux.intel.com>
On Wed, May 15, 2019 at 11:12:50AM +0300, Jarkko Sakkinen wrote:
>On Mon, Apr 15, 2019 at 11:56:35AM -0400, Sasha Levin wrote:
>> This patch adds support for a software-only implementation of a TPM
>> running in TEE.
>>
>> There is extensive documentation of the design here:
>> https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/ .
>>
>> As well as reference code for the firmware available here:
>> https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM
>
>The commit message should include at least a brief description what TEE
>is.
The whole TEE subsystem is already well documented in our kernel tree
(https://www.kernel.org/doc/Documentation/tee.txt) and beyond. I can add
a reference to the doc here, but I'd rather not add a bunch of TEE
related comments as you suggest later in your review.
The same way a PCI device driver doesn't describe what PCI is in it's
code, we shouldn't be doing the same for TEE here.
>> +
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/acpi.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/tee_drv.h>
>> +#include <linux/uuid.h>
>> +#include <linux/tpm.h>
>> +
>> +#include "tpm.h"
>> +#include "tpm_ftpm_tee.h"
>> +
>> +#define DRIVER_NAME "ftpm-tee"
>> +
>> +/* TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896 */
>> +static const uuid_t ftpm_ta_uuid =
>> + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
>> + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
>
>Just wondering why prefixes are here in different order in the comment
>and code.
No prefixes, this is a completely randomly generated UUID.
I'll address the rest of your comments in the next ver.
--
Thanks,
Sasha
^ permalink raw reply
* RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver
From: Thirupathaiah Annapureddy @ 2019-05-17 17:23 UTC (permalink / raw)
To: Sumit Garg
Cc: Sasha Levin, Jarkko Sakkinen, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, Linux Kernel Mailing List,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI), Rob Herring
In-Reply-To: <CAFA6WYMsQ53L=Ge5_yX+mvQNgKQRxPq=GxAn6zCksaDU18aTtw@mail.gmail.com>
> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Thursday, May 16, 2019 11:57 PM
> To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> <bryankel@microsoft.com>; Rob Herring <robh+dt@kernel.org>
> Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
>
> + Rob
>
> On Fri, 17 May 2019 at 00:54, Thirupathaiah Annapureddy
> <thiruan@microsoft.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Thursday, May 16, 2019 12:06 AM
> > > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > > corbet@lwn.net; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>;
> > > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft
> Linux
> > > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > > <bryankel@microsoft.com>
> > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > >
> > > On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> > > <thiruan@microsoft.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > > > To: Sasha Levin <sashal@kernel.org>
> > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> > > peterhuewe@gmx.de;
> > > > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > > > kernel@vger.kernel.org>; linux-doc@vger.kernel.org; linux-
> > > > > integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> > > > > kernel@microsoft.com>; Thirupathaiah Annapureddy
> > > <thiruan@microsoft.com>;
> > > > > Bryan Kelly (CSI) <bryankel@microsoft.com>
> > > > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > > > >
> > > > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org>
> wrote:
> > > > > >
> > > > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > > > >> >
> > > > > > >> > Changes since v2:
> > > > > > >> >
> > > > > > >> > - Drop the devicetree bindings patch (we don't add any new
> > > ones).
> > > > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > > > >> >
> > > > > > >> > Sasha Levin (2):
> > > > > > >> > ftpm: firmware TPM running in TEE
> > > > > > >> > ftpm: add documentation for ftpm driver
> > > > > > >>
> > > > > > >> Ping? Does anyone have any objections to this?
> > > > > > >
> > > > > > >Sorry I've been on vacation week before last week and last week
> > > > > > >I was extremely busy because I had been on vacation. This in
> > > > > > >my TODO list. Will look into it tomorrow in detail.
> > > > > > >
> > > > > > >Apologies for the delay with this!
> > > > > >
> > > > > > Hi Jarkko,
> > > > > >
> > > > > > If there aren't any big objections to this, can we get it merged
> in?
> > > > > > We'll be happy to address any comments that come up.
> > > > >
> > > > > I guess you have missed or ignored this comment [1]. Please address
> it.
> > > > >
> > > > > [1]
> > > > >
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > > > >
> > >
> 2Flkml%2F2019%2F5%2F8%2F11&data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > > > >
> > >
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&sd
> > > > >
> ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&reserved=0
> > > > >
> > > > > -Sumit
> > > >
> > > > Thanks for reviewing and adding comments.
> > > >
> > > > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > > > We were not able to pass the TCG Logs collected by the boot loaders.
> > > >
> > > > Currently there are 3 ways to pass TCG Logs based on the code
> > > > in drivers/char/tpm/eventlog:
> > > >
> > > > 1. ACPI Table
> > > > 2. EFI Table
> > > > 3. OF Device node properties
> > > >
> > > > Our ARM system is booting using U-boot and Device Tree.
> > > > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > > > We needed to use OF device node properties to pass TCG2 Logs.
> > > > TEE bus enumeration framework does not work for our use case due to
> the
> > > above.
> > >
> > > Firstly let me clarify that this framework is intended to communicate
> > > with TEE based services/devices rather than boot loader. And in this
> > > case fTPM being a TEE based service, so this framework should be used.
> > >
> > It does not work for our use case. We gave enough justification so far.
> > TEE bus enumeration needs to be flexible to support our use case and
> > more future use cases.
> >
>
> TEE based services are virtual devices which could be very well be
> aware about the platform and device driver could easily query these
> devices for platform specific properties. In case of firmware TPM as a
> TEE based service, it could easily store the event logs generated
> during PCR extend operations which could be fetched at runtime. But a
> real TPM device doesn't possess that storage capability leading to
> software managing these event logs.
>
> > > >
> > > > Is it possible to add flexibility in TEE bus enumeration framework to
> > > support
> > > > platform specific properties through OF nodes or ACPI?
> > > >
> > >
> > > As you mentioned above, TCG logs are collected by boot loader. So it
> > > should find a way to pass them to Linux.
> > >
> > > How about if boot loader register these TCG logs with fTPM TA which
> > > could be fetched during fTPM driver probe or new api like
> > > tpm_read_log_tee()?
> >
> > And then how does fTPM driver pass TCG Logs to the TPM framework?
> > It requires changes to the upstream TPM framework to ask the driver
> > explicitly for the TCG Logs.
>
> My suggestion was to add 4th way to pass TCG logs as follows:
>
> --- a/drivers/char/tpm/eventlog/common.c
> +++ b/drivers/char/tpm/eventlog/common.c
> @@ -95,6 +95,10 @@ static int tpm_read_log(struct tpm_chip *chip)
> if (rc != -ENODEV)
> return rc;
>
> + rc = tpm_read_log_tee(chip);
> + if (rc != -ENODEV)
> + return rc;
> +
> return tpm_read_log_of(chip);
> }
> --- /dev/null
> +++ b/drivers/char/tpm/eventlog/tee.c
> @@ -0,0 +1,43 @@
> <snip>
> +int tpm_read_log_tee(struct tpm_chip *chip)
> +{
> + struct tpm_bios_log *log;
> + struct ftpm_tee_private *pvt_data;
> +
> + log = &chip->log;
> + if (!strcmp(chip->dev.bus->name, tee_bus_type.name))
> + pvt_data = dev_get_drvdata(chip->dev.parent);
> + else
> + return -ENODEV;
> +
> + // Here you could simply do an invoke command to fetch the TCG
> logs.
> +
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> + return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> +}
But I do not see a need for this 4th method if TEE bus enumeration framework supports flexibility.
The problem is with the TEE bus enumeration.
>
> >
> > Note that this also requires changes to the fTPM TA that has been
> existing for few years.
> >
>
> That's not a sane reason to avoid implementing generic stuff.
Exactly, TEE bus enumeration framework need to support generic stuff that is expected
of a bus driver. You are expecting everything else to be generic enough, but not TEE bus enumeration.
> As there
> could be other fTPM implementations and specific DT nodes for each
> which might not be maintainable. BTW, in current implementation also I
> don't find DT bindings corresponding to DT node used in this
> patch-set.
>
> > Is it not possible to add flexibility in TEE bus enumeration framework to
> > support platform specific properties through OF nodes or ACPI?
> >
>
> TEE bus framework was designed specifically to avoid OF nodes or ACPI
> properties. As devices could be intelligent enough to be queried for
> required properties.
Is there an expectation that TEE bus enumerated devices to be intelligent?
If so, this is another limitation of TEE bus enumeration.
Please fix these limitations to make TEE bus enumeration
scalable and flexible for future use cases.
>
> > Devices enumerated by buses such as i2c can read platform specific
> properties.
>
> i2c devices are real hardware which could be platform agnostic, so you
> need to have platform specific properties.
>
> -Sumit
>
> > With this flexibility added, more future use cases through TEE bus.
> >
> >
> > > This is something similar to what I used in
> > > optee-rng [1] driver to fetch RNG properties.
> > >
> > > [1]
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> > >
> m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> > >
> rng.c%23L176&data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> > >
> 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> > >
> 3006&sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&reserved=
> > > 0
> > >
> > > -Sumit
> > >
> > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > > Sasha
^ permalink raw reply
* Re: [PATCH v4 03/18] kunit: test: add string_stream a std::stream like string builder
From: Stephen Boyd @ 2019-05-17 17:42 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190514221711.248228-4-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:16:56)
> A number of test features need to do pretty complicated string printing
> where it may not be possible to rely on a single preallocated string
> with parameters.
>
> So provide a library for constructing the string as you go similar to
> C++'s std::string.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Is there any reason why we can't use the seqfile API for this? These
both share a similar goal, formatting strings into a buffer to be read
later. Maybe some new APIs would be needed to extract the buffer
differently, but I hope we could share the code.
If it can't be used, can you please add the reasoning to the commit text
here?
^ permalink raw reply
* Re: [PATCH v4 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Stephen Boyd @ 2019-05-17 17:58 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190514221711.248228-5-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:16:57)
> diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
> new file mode 100644
> index 0000000000000..1884f1b550888
> --- /dev/null
> +++ b/kunit/kunit-stream.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * C++ stream style string formatter and printer used in KUnit for outputting
> + * KUnit messages.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <kunit/test.h>
> +#include <kunit/kunit-stream.h>
> +#include <kunit/string-stream.h>
> +
> +static const char *kunit_stream_get_level(struct kunit_stream *this)
> +{
> + unsigned long flags;
> + const char *level;
> +
> + spin_lock_irqsave(&this->lock, flags);
> + level = this->level;
> + spin_unlock_irqrestore(&this->lock, flags);
> +
> + return level;
Please remove this whole function and inline it to the one call-site.
> +}
> +
> +void kunit_stream_set_level(struct kunit_stream *this, const char *level)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&this->lock, flags);
> + this->level = level;
> + spin_unlock_irqrestore(&this->lock, flags);
I don't get the locking here. What are we protecting against? Are tests
running in parallel using the same kunit_stream? If so, why is the level
changeable in one call and then adding strings is done in a different
function call? It would make sense to combine the level setting and
string adding so that it's one atomic operation if it's truly a parallel
operation, or remove the locking entirely.
> +}
> +
> +void kunit_stream_add(struct kunit_stream *this, const char *fmt, ...)
> +{
> + va_list args;
> + struct string_stream *stream = this->internal_stream;
> +
> + va_start(args, fmt);
> +
> + if (string_stream_vadd(stream, fmt, args) < 0)
> + kunit_err(this->test, "Failed to allocate fragment: %s\n", fmt);
> +
> + va_end(args);
> +}
> +
> +void kunit_stream_append(struct kunit_stream *this,
> + struct kunit_stream *other)
> +{
> + struct string_stream *other_stream = other->internal_stream;
> + const char *other_content;
> +
> + other_content = string_stream_get_string(other_stream);
> +
> + if (!other_content) {
> + kunit_err(this->test,
> + "Failed to get string from second argument for appending.\n");
> + return;
> + }
> +
> + kunit_stream_add(this, other_content);
> +}
> +
> +void kunit_stream_clear(struct kunit_stream *this)
> +{
> + string_stream_clear(this->internal_stream);
> +}
> +
> +void kunit_stream_commit(struct kunit_stream *this)
Should this be rather called kunit_stream_flush()?
> +{
> + struct string_stream *stream = this->internal_stream;
> + struct string_stream_fragment *fragment;
> + const char *level;
> + char *buf;
> +
> + level = kunit_stream_get_level(this);
> + if (!level) {
> + kunit_err(this->test,
> + "Stream was committed without a specified log level.\n");
Drop the full-stop?
> + level = KERN_ERR;
> + kunit_stream_set_level(this, level);
> + }
> +
> + buf = string_stream_get_string(stream);
> + if (!buf) {
> + kunit_err(this->test,
Can you grow a local variable for 'this->test'? It's used many times.
Also, 'this' is not very kernel idiomatic. We usually name variables by
their type instead of 'this' which is a keyword in other languages.
Perhaps it could be named 'kstream'?
> + "Could not allocate buffer, dumping stream:\n");
> + list_for_each_entry(fragment, &stream->fragments, node) {
> + kunit_err(this->test, fragment->fragment);
> + }
> + kunit_err(this->test, "\n");
> + goto cleanup;
> + }
> +
> + kunit_printk(level, this->test, buf);
> + kfree(buf);
> +
> +cleanup:
> + kunit_stream_clear(this);
> +}
> +
> +static int kunit_stream_init(struct kunit_resource *res, void *context)
> +{
> + struct kunit *test = context;
> + struct kunit_stream *stream;
> +
> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
Of course, here it's called 'stream', so maybe it should be 'kstream'
here too.
> + if (!stream)
> + return -ENOMEM;
> +
> + res->allocation = stream;
> + stream->test = test;
> + spin_lock_init(&stream->lock);
> + stream->internal_stream = new_string_stream();
Can new_string_stream() be renamed to alloc_string_stream()? Sorry, I
just see so much C++ isms in here it's hard to read from the kernel
developer perspective.
> +
> + if (!stream->internal_stream) {
Nitpick: Please join this to the "allocation" event above instead of
keeping it separated.
> + kfree(stream);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void kunit_stream_free(struct kunit_resource *res)
> +{
> + struct kunit_stream *stream = res->allocation;
> +
> + if (!string_stream_is_empty(stream->internal_stream)) {
> + kunit_err(stream->test,
> + "End of test case reached with uncommitted stream entries.\n");
> + kunit_stream_commit(stream);
> + }
> +
> + destroy_string_stream(stream->internal_stream);
> + kfree(stream);
> +}
> +
> +struct kunit_stream *kunit_new_stream(struct kunit *test)
> +{
> + struct kunit_resource *res;
> +
> + res = kunit_alloc_resource(test,
> + kunit_stream_init,
> + kunit_stream_free,
> + test);
> +
> + if (res)
> + return res->allocation;
> + else
> + return NULL;
Don't have if (...) return ...; else return ..., just return instead of
else.
^ permalink raw reply
* Re: [PATCH v4 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Stephen Boyd @ 2019-05-17 18:22 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Iurii Zaikin, Brendan Higgins
In-Reply-To: <20190514221711.248228-18-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:17:10)
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> new file mode 100644
> index 0000000000000..fe0f2bae66085
> --- /dev/null
> +++ b/kernel/sysctl-test.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test of proc sysctl.
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/printk.h>
Is this include used?
> +#include <linux/sysctl.h>
> +#include <linux/uaccess.h>
Is this include used?
> +
> +
> +static void sysctl_test_dointvec_happy_single_negative(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[] = "-9";
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> +
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, pos);
> + KUNIT_EXPECT_EQ(test, -9, *(int *)table.data);
Is the casting necessary? Or can the macro do a type coercion of the
second parameter based on the first type?
> +}
> +
> +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[32];
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> + unsigned long abs_of_less_than_min = (unsigned long)INT_MAX
> + - (INT_MAX + INT_MIN) + 1;
> +
> + KUNIT_EXPECT_LT(test,
> + snprintf(input, sizeof(input), "-%lu",
> + abs_of_less_than_min),
> + sizeof(input));
> +
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, -EINVAL,
> + proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, 0, *(int *)table.data);
> +}
> +
> +static void sysctl_test_dointvec_single_greater_int_max(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[32];
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> + unsigned long greater_than_max = (unsigned long)INT_MAX + 1;
> +
> + KUNIT_EXPECT_GT(test, greater_than_max, INT_MAX);
> + KUNIT_EXPECT_LT(test, snprintf(input, sizeof(input), "%lu",
> + greater_than_max),
> + sizeof(input));
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, -EINVAL,
> + proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, 0, *(int *)table.data);
> +}
> +
> +static int sysctl_test_init(struct kunit *test)
> +{
> + return 0;
> +}
> +
> +/*
> + * This is run once after each test case, see the comment on example_test_module
> + * for more information.
> + */
> +static void sysctl_test_exit(struct kunit *test)
> +{
> +}
Can the above two be omitted? If they can be empty sometimes it would be
nice to avoid the extra symbols and code by letting them be assigned to
NULL in the kunit_module.
> +
> +/*
> + * Here we make a list of all the test cases we want to add to the test module
> + * below.
> + */
> +static struct kunit_case sysctl_test_cases[] = {
> + /*
> + * This is a helper to create a test case object from a test case
> + * function; its exact function is not important to understand how to
> + * use KUnit, just know that this is how you associate test cases with a
> + * test module.
> + */
> + KUNIT_CASE(sysctl_test_dointvec_null_tbl_data),
> + KUNIT_CASE(sysctl_test_dointvec_table_maxlen_unset),
> + KUNIT_CASE(sysctl_test_dointvec_table_len_is_zero),
> + KUNIT_CASE(sysctl_test_dointvec_table_read_but_position_set),
> + KUNIT_CASE(sysctl_test_dointvec_happy_single_positive),
> + KUNIT_CASE(sysctl_test_dointvec_happy_single_negative),
> + KUNIT_CASE(sysctl_test_dointvec_single_less_int_min),
> + KUNIT_CASE(sysctl_test_dointvec_single_greater_int_max),
> + {},
> +};
> +
> +/*
> + * This defines a suite or grouping of tests.
> + *
> + * Test cases are defined as belonging to the suite by adding them to
> + * `test_cases`.
> + *
> + * Often it is desirable to run some function which will set up things which
> + * will be used by every test; this is accomplished with an `init` function
> + * which runs before each test case is invoked. Similarly, an `exit` function
> + * may be specified which runs after every test case and can be used to for
> + * cleanup. For clarity, running tests in a test module would behave as follows:
> + *
> + * module.init(test);
> + * module.test_case[0](test);
> + * module.exit(test);
> + * module.init(test);
> + * module.test_case[1](test);
> + * module.exit(test);
> + * ...;
This comment (and the one above for "this is a helper") looks generic
and should probably only be in some documentation somewhere and not for
a sysctl test?
> + */
> +static struct kunit_module sysctl_test_module = {
> + .name = "sysctl_test",
> + .init = sysctl_test_init,
> + .exit = sysctl_test_exit,
> + .test_cases = sysctl_test_cases,
> +};
> +
> +/*
> + * This registers the above test module telling KUnit that this is a suite of
> + * tests that need to be run.
> + */
Same comment about generic comment.
> +module_test(sysctl_test_module);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d5a4a4036d2f8..772af4ec70111 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1908,6 +1908,12 @@ config TEST_SYSCTL
>
> If unsure, say N.
>
> +config SYSCTL_KUNIT_TEST
> + bool "KUnit test for sysctl"
Why not tristate?
> + depends on KUNIT
> + help
> + Enables KUnit sysctl test.
> +
^ permalink raw reply
* Re: [PATCH v4 01/18] kunit: test: add KUnit test runner core
From: Stephen Boyd @ 2019-05-17 18:53 UTC (permalink / raw)
To: Brendan Higgins, frowand.list, gregkh, jpoimboe, keescook,
kieran.bingham, mcgrof, peterz, robh, shuah, tytso,
yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190514221711.248228-2-brendanhiggins@google.com>
Quoting Brendan Higgins (2019-05-14 15:16:54)
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> new file mode 100644
> index 0000000000000..e682ea0e1f9a5
> --- /dev/null
> +++ b/include/kunit/test.h
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Base unit test (KUnit) API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_TEST_H
> +#define _KUNIT_TEST_H
> +
> +#include <linux/types.h>
> +#include <linux/slab.h>
Is this include used here?
> +
> +struct kunit;
> +
> +/**
> + * struct kunit_case - represents an individual test case.
> + * @run_case: the function representing the actual test case.
> + * @name: the name of the test case.
> + *
> + * A test case is a function with the signature, ``void (*)(struct kunit *)``
> + * that makes expectations (see KUNIT_EXPECT_TRUE()) about code under test. Each
> + * test case is associated with a &struct kunit_module and will be run after the
> + * module's init function and followed by the module's exit function.
> + *
> + * A test case should be static and should only be created with the KUNIT_CASE()
> + * macro; additionally, every array of test cases should be terminated with an
> + * empty test case.
> + *
> + * Example:
> + *
> + * .. code-block:: c
> + *
> + * void add_test_basic(struct kunit *test)
> + * {
> + * KUNIT_EXPECT_EQ(test, 1, add(1, 0));
> + * KUNIT_EXPECT_EQ(test, 2, add(1, 1));
> + * KUNIT_EXPECT_EQ(test, 0, add(-1, 1));
> + * KUNIT_EXPECT_EQ(test, INT_MAX, add(0, INT_MAX));
> + * KUNIT_EXPECT_EQ(test, -1, add(INT_MAX, INT_MIN));
> + * }
> + *
> + * static struct kunit_case example_test_cases[] = {
> + * KUNIT_CASE(add_test_basic),
> + * {},
Nitpick: Please drop the comma on the sentinel so nobody gets ideas to
add another entry after it.
> + * };
> + *
> + */
> +struct kunit_case {
> + void (*run_case)(struct kunit *test);
> + const char name[256];
Maybe 256 can be a #define KUNIT_NAME_MAX_LEN? Or it could just be a
const char pointer to a literal pool? Are unit tests making up names at
runtime?
> +
> + /* private: internal use only. */
> + bool success;
> +};
> +
> +/**
> + * KUNIT_CASE - A helper for creating a &struct kunit_case
> + * @test_name: a reference to a test case function.
> + *
> + * Takes a symbol for a function representing a test case and creates a
> + * &struct kunit_case object from it. See the documentation for
> + * &struct kunit_case for an example on how to use it.
> + */
> +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> +
> +/**
> + * struct kunit_module - describes a related collection of &struct kunit_case s.
> + * @name: the name of the test. Purely informational.
> + * @init: called before every test case.
> + * @exit: called after every test case.
> + * @test_cases: a null terminated array of test cases.
> + *
> + * A kunit_module is a collection of related &struct kunit_case s, such that
> + * @init is called before every test case and @exit is called after every test
> + * case, similar to the notion of a *test fixture* or a *test class* in other
> + * unit testing frameworks like JUnit or Googletest.
> + *
> + * Every &struct kunit_case must be associated with a kunit_module for KUnit to
> + * run it.
> + */
> +struct kunit_module {
> + const char name[256];
> + int (*init)(struct kunit *test);
> + void (*exit)(struct kunit *test);
> + struct kunit_case *test_cases;
Can this variable be const? Or we expect test modules to adjust test_cases after
the fact?
> +};
> +
> +/**
> + * struct kunit - represents a running instance of a test.
> + * @priv: for user to store arbitrary data. Commonly used to pass data created
> + * in the init function (see &struct kunit_module).
> + *
> + * Used to store information about the current context under which the test is
> + * running. Most of this data is private and should only be accessed indirectly
> + * via public functions; the one exception is @priv which can be used by the
> + * test writer to store arbitrary data.
> + */
> +struct kunit {
> + void *priv;
> +
> + /* private: internal use only. */
> + const char *name; /* Read only after initialization! */
> + spinlock_t lock; /* Gaurds all mutable test state. */
> + bool success; /* Protected by lock. */
> +};
> +
> +void kunit_init_test(struct kunit *test, const char *name);
> +
> +int kunit_run_tests(struct kunit_module *module);
> +
> +/**
> + * module_test() - used to register a &struct kunit_module with KUnit.
> + * @module: a statically allocated &struct kunit_module.
> + *
> + * Registers @module with the test framework. See &struct kunit_module for more
> + * information.
> + */
> +#define module_test(module) \
> + static int module_kunit_init##module(void) \
> + { \
> + return kunit_run_tests(&module); \
> + } \
> + late_initcall(module_kunit_init##module)
Maybe we need to introduce another initcall level after
late_initcall_sync() for tests? I wonder if there will be tests that
need to run after all other initcalls have run, including late sync
initcalls.
> +
> +void __printf(3, 4) kunit_printk(const char *level,
> + const struct kunit *test,
> + const char *fmt, ...);
> +
> +/**
> + * kunit_info() - Prints an INFO level message associated with the current test.
> + * @test: The test context object.
> + * @fmt: A printk() style format string.
> + *
> + * Prints an info level message associated with the test module being run. Takes
> + * a variable number of format parameters just like printk().
> + */
> +#define kunit_info(test, fmt, ...) \
> + kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
> +
> +/**
> + * kunit_warn() - Prints a WARN level message associated with the current test.
> + * @test: The test context object.
> + * @fmt: A printk() style format string.
> + *
> + * See kunit_info().
Why? Just write out that it "Prints a warning level message".
> + */
> +#define kunit_warn(test, fmt, ...) \
> + kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
> +
> +/**
> + * kunit_err() - Prints an ERROR level message associated with the current test.
> + * @test: The test context object.
> + * @fmt: A printk() style format string.
> + *
> + * See kunit_info().
Same comment.
> + */
> +#define kunit_err(test, fmt, ...) \
> + kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> +
> +#endif /* _KUNIT_TEST_H */
> diff --git a/kunit/Kconfig b/kunit/Kconfig
> new file mode 100644
> index 0000000000000..64480092b2c24
> --- /dev/null
> +++ b/kunit/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# KUnit base configuration
> +#
> +
> +menu "KUnit support"
> +
> +config KUNIT
> + bool "Enable support for unit tests (KUnit)"
> + help
> + Enables support for kernel unit tests (KUnit), a lightweight unit
> + testing and mocking framework for the Linux kernel. These tests are
> + able to be run locally on a developer's workstation without a VM or
> + special hardware. For more information, please see
> + Documentation/kunit/
This moved and needs an update.
> +
> +endmenu
> diff --git a/kunit/Makefile b/kunit/Makefile
> new file mode 100644
> index 0000000000000..5efdc4dea2c08
> --- /dev/null
> +++ b/kunit/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_KUNIT) += test.o
> diff --git a/kunit/test.c b/kunit/test.c
> new file mode 100644
> index 0000000000000..86f65ba2bcf92
> --- /dev/null
> +++ b/kunit/test.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Base unit test (KUnit) API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <linux/sched.h>
This include gets removed later in the series, was it ever needed?
> +#include <linux/sched/debug.h>
> +#include <kunit/test.h>
> +
> +static bool kunit_get_success(struct kunit *test)
> +{
> + unsigned long flags;
> + bool success;
> +
> + spin_lock_irqsave(&test->lock, flags);
> + success = test->success;
> + spin_unlock_irqrestore(&test->lock, flags);
> +
> + return success;
> +}
> +
> +static void kunit_set_success(struct kunit *test, bool success)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&test->lock, flags);
> + test->success = success;
> + spin_unlock_irqrestore(&test->lock, flags);
> +}
> +
> +static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> +{
> + return vprintk_emit(0, level, NULL, 0, fmt, args);
> +}
> +
> +static int kunit_printk_emit(int level, const char *fmt, ...)
> +{
> + va_list args;
> + int ret;
> +
> + va_start(args, fmt);
> + ret = kunit_vprintk_emit(level, fmt, args);
> + va_end(args);
> +
> + return ret;
> +}
> +
> +static void kunit_vprintk(const struct kunit *test,
> + const char *level,
> + struct va_format *vaf)
> +{
> + kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
> +}
> +
> +static bool kunit_has_printed_tap_version;
> +
> +static void kunit_print_tap_version(void)
> +{
> + if (!kunit_has_printed_tap_version) {
> + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> + kunit_has_printed_tap_version = true;
> + }
> +}
> +
> +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> +{
> + struct kunit_case *test_case;
> + size_t len = 0;
> +
> + for (test_case = test_cases; test_case->run_case; test_case++)
> + len++;
> +
> + return len;
> +}
> +
> +static void kunit_print_subtest_start(struct kunit_module *module)
> +{
> + kunit_print_tap_version();
> + kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", module->name);
> + kunit_printk_emit(LOGLEVEL_INFO,
> + "\t1..%zd\n",
> + kunit_test_cases_len(module->test_cases));
> +}
> +
> +static void kunit_print_ok_not_ok(bool should_indent,
> + bool is_ok,
> + size_t test_number,
> + const char *description)
> +{
> + const char *indent, *ok_not_ok;
> +
> + if (should_indent)
> + indent = "\t";
> + else
> + indent = "";
> +
> + if (is_ok)
> + ok_not_ok = "ok";
> + else
> + ok_not_ok = "not ok";
> +
> + kunit_printk_emit(LOGLEVEL_INFO,
> + "%s%s %zd - %s\n",
> + indent, ok_not_ok, test_number, description);
> +}
> +
> +static bool kunit_module_has_succeeded(struct kunit_module *module)
> +{
> + struct kunit_case *test_case;
This can be const?
> + bool success = true;
> +
> + for (test_case = module->test_cases; test_case->run_case; test_case++)
> + if (!test_case->success)
> + success = false;
Bail out early here on first "fail" with return false?
> +
> + return success;
> +}
> +
> +size_t kunit_module_counter = 1;
> +
> +static void kunit_print_subtest_end(struct kunit_module *module)
> +{
> + kunit_print_ok_not_ok(false,
> + kunit_module_has_succeeded(module),
> + kunit_module_counter++,
> + module->name);
> +}
> +
> +static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
> + size_t test_number)
> +{
> + kunit_print_ok_not_ok(true,
> + test_case->success,
> + test_number,
> + test_case->name);
> +}
> +
> +void kunit_init_test(struct kunit *test, const char *name)
> +{
> + spin_lock_init(&test->lock);
> + test->name = name;
> +}
> +
> +/*
> + * Initializes and runs test case. Does not clean up or do post validations.
> + */
> +static void kunit_run_case_internal(struct kunit *test,
> + struct kunit_module *module,
> + struct kunit_case *test_case)
> +{
> + int ret;
> +
> + if (module->init) {
> + ret = module->init(test);
> + if (ret) {
> + kunit_err(test, "failed to initialize: %d\n", ret);
> + kunit_set_success(test, false);
> + return;
> + }
> + }
> +
> + test_case->run_case(test);
> +}
> +
> +/*
> + * Performs post validations and cleanup after a test case was run.
> + * XXX: Should ONLY BE CALLED AFTER kunit_run_case_internal!
> + */
> +static void kunit_run_case_cleanup(struct kunit *test,
> + struct kunit_module *module,
> + struct kunit_case *test_case)
But test_case isn't used?
> +{
> + if (module->exit)
Aha, so we don't need empty functions in the sysctl test.
> + module->exit(test);
> +}
> +
> +/*
> + * Performs all logic to run a test case.
> + */
> +static void kunit_run_case(struct kunit_module *module,
> + struct kunit_case *test_case)
> +{
> + struct kunit test;
> +
> + kunit_init_test(&test, test_case->name);
> + kunit_set_success(&test, true);
Can kunit_init_test() also kunit_set_success() to true or false,
depending on what is desired as the initial state?
> +
> + kunit_run_case_internal(&test, module, test_case);
> + kunit_run_case_cleanup(&test, module, test_case);
I find this odd, we have run_case_internal() that does two things, init
and run_case, while case_cleanup() does one thing, call module->exit().
Can we just inline all those functions in here so that it looks like
this:
int ret = 0;
if (module->init) {
ret = module->init(test);
if (ret) {
kunit_err(test, "failed to initialize: %d\n", ret);
kunit_set_success(&test, false);
}
}
if (!ret)
test_case->run_case(&test);
if (module->exit)
module->exit(&test);
return kunit_get_success(&test);
Then I don't have to read two more functions to figure out the flow of
running a test case.
> +
> + test_case->success = kunit_get_success(&test);
> +}
> +
> +int kunit_run_tests(struct kunit_module *module)
> +{
> + struct kunit_case *test_case;
> + size_t test_case_count = 1;
Might make sense to assign this to 0 first and then pre-increment so
that test_case_count can't be 1 when there aren't any tests?
> +
> + kunit_print_subtest_start(module);
> +
> + for (test_case = module->test_cases; test_case->run_case; test_case++) {
> + kunit_run_case(module, test_case);
> + kunit_print_test_case_ok_not_ok(test_case, test_case_count++);
Can this be pushed into kunit_run_case() and have that function take a
test_case_count number? Maybe that would allow us to avoid storing
test_case->success entirely? Assuming that kunit_run_case() returned a
value like success or failure, then yes it would work.
unsigned int failed = 0;
for (test_case = module->test_cases; test_case->run_case; test_case++) {
failed |= kunit_run_case(module, test_case, ++test_case_count);
kunit_print_ok_not_ok(false,
!failed,
kunit_module_counter++,
module->name);
> + kunit_print_subtest_end(module);
> +
> + return 0;
> +}
^ permalink raw reply
* [PATCH] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu slices
From: Dave Chiluk @ 2019-05-17 19:30 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, cgroups, linux-kernel, Brendan Gregg,
Kyle Anderson, Gabriel Munos, John Hammond, Cong Wang,
Jonathan Corbet, linux-doc
It has been observed, that highly-threaded, non-cpu-bound applications
running under cpu.cfs_quota_us constraints can hit a high percentage of
periods throttled while simultaneously not consuming the allocated
amount of quota. This use case is typical of user-interactive non-cpu
bound applications, such as those running in kubernetes or mesos when
run on multiple cpu cores.
This has been root caused to threads being allocated per cpu bandwidth
slices, and then not fully using that slice within the period. At which
point the slice and quota expires. This expiration of unused slice
results in applications not being able to utilize the quota for which
they are allocated.
The expiration of per-cpu slices was recently fixed by commit
512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition").
Prior to that it appears that this has been broken since a least commit
51f2176d74ac ("sched/fair: Fix unlocked reads of some
cfs_b->quota/period") which was introduced in v3.16-rc1 in 2014. That
commit added the following conditional which resulted in slices never
being expired.
if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
/* extend local deadline, drift is bounded above by 2 ticks */
cfs_rq->runtime_expires += TICK_NSEC;
Because this was broken for nearly 5 years, and has recently been fixed
and is now being noticed by many users running kubernetes
(https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
that the mechanisms around expiring runtime should be removed
altogether.
This allows only per-cpu slices to live longer than the period boundary.
This allows threads on runqueues that do not use much CPU to continue to
use their remaining slice over a longer period of time than
cpu.cfs_period_us. However, this helps prevents the above condition of
hitting throttling while also not fully utilizing your cpu quota.
This theoretically allows a machine to use slightly more than it's
allotted quota in some periods. This overflow would be bounded by the
remaining per-cpu slice that was left un-used in the previous period.
For CPU bound tasks this will change nothing, as they should
theoretically fully utilize all of their quota and slices in each
period. For user-interactive tasks as described above this provides a
much better user/application experience as their cpu utilization will
more closely match the amount they requested when they hit throttling.
This greatly improves performance of high-thread-count, non-cpu bound
applications with low cfs_quota_us allocation on high-core-count
machines. In the case of an artificial testcase, this performance
discrepancy has been observed to be almost 30x performance improvement,
while still maintaining correct cpu quota restrictions albeit over
longer time intervals than cpu.cfs_period_us. That testcase is available
at https://github.com/indeedeng/fibtest.
Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
---
Documentation/scheduler/sched-bwc.txt | 29 +++++++++++---
kernel/sched/fair.c | 71 +++--------------------------------
kernel/sched/sched.h | 4 --
3 files changed, 29 insertions(+), 75 deletions(-)
diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
index f6b1873..4ded8ae 100644
--- a/Documentation/scheduler/sched-bwc.txt
+++ b/Documentation/scheduler/sched-bwc.txt
@@ -8,16 +8,33 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
specification of the maximum CPU bandwidth available to a group or hierarchy.
The bandwidth allowed for a group is specified using a quota and period. Within
-each given "period" (microseconds), a group is allowed to consume only up to
-"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
-group exceeds this limit (for that period), the tasks belonging to its
-hierarchy will be throttled and are not allowed to run again until the next
-period.
+each given "period" (microseconds), a task group is allocated up to "quota"
+microseconds of CPU time. When the CPU bandwidth consumption of a group
+exceeds this limit (for that period), the tasks belonging to its hierarchy will
+be throttled and are not allowed to run again until the next period.
A group's unused runtime is globally tracked, being refreshed with quota units
above at each period boundary. As threads consume this bandwidth it is
transferred to cpu-local "silos" on a demand basis. The amount transferred
-within each of these updates is tunable and described as the "slice".
+within each of these updates is tunable and described as the "slice". Slices
+that are allocated to cpu-local silos do not expire at the end of the period,
+but unallocated quota does. This doesn't affect cpu-bound applications as they
+by definition consume all of their bandwidth in each each period.
+
+However for highly-threaded user-interactive/non-cpu bound applications this
+non-expiration nuance allows applications to burst past their quota limits
+equal to the amount of unused slice per cpu that the task group is running on.
+This slight burst requires that quota had gone unused in previous periods.
+Additionally this burst amount is limited to the size of a slice for every cpu
+a task group is run on. As a result, this mechanism still strictly limits the
+task group to quota average usage over a longer time windows. This provides
+better more predictable user experience for highly threaded applications with
+small quota limits on high core count machines. It also eliminates the
+propensity to throttle these applications while simultanously using less than
+quota amounts of cpu. Another way to say this, is that by allowing the unused
+portion of a slice to be used in following periods we have decreased the
+possibility of wasting unused quota on cpu-local silos that don't need much cpu
+time.
Management
----------
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f..a675c69 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
now = sched_clock_cpu(smp_processor_id());
cfs_b->runtime = cfs_b->quota;
- cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
}
static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
{
struct task_group *tg = cfs_rq->tg;
struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
- u64 amount = 0, min_amount, expires;
- int expires_seq;
+ u64 amount = 0, min_amount;
/* note: this is a positive sum as runtime_remaining <= 0 */
min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
@@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
cfs_b->idle = 0;
}
}
- expires_seq = cfs_b->expires_seq;
- expires = cfs_b->runtime_expires;
raw_spin_unlock(&cfs_b->lock);
cfs_rq->runtime_remaining += amount;
- /*
- * we may have advanced our local expiration to account for allowed
- * spread between our sched_clock and the one on which runtime was
- * issued.
- */
- if (cfs_rq->expires_seq != expires_seq) {
- cfs_rq->expires_seq = expires_seq;
- cfs_rq->runtime_expires = expires;
- }
return cfs_rq->runtime_remaining > 0;
}
-/*
- * Note: This depends on the synchronization provided by sched_clock and the
- * fact that rq->clock snapshots this value.
- */
-static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
-{
- struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
-
- /* if the deadline is ahead of our clock, nothing to do */
- if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
- return;
-
- if (cfs_rq->runtime_remaining < 0)
- return;
-
- /*
- * If the local deadline has passed we have to consider the
- * possibility that our sched_clock is 'fast' and the global deadline
- * has not truly expired.
- *
- * Fortunately we can check determine whether this the case by checking
- * whether the global deadline(cfs_b->expires_seq) has advanced.
- */
- if (cfs_rq->expires_seq == cfs_b->expires_seq) {
- /* extend local deadline, drift is bounded above by 2 ticks */
- cfs_rq->runtime_expires += TICK_NSEC;
- } else {
- /* global deadline is ahead, expiration has passed */
- cfs_rq->runtime_remaining = 0;
- }
-}
-
static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
{
/* dock delta_exec before expiring quota (as it could span periods) */
cfs_rq->runtime_remaining -= delta_exec;
- expire_cfs_rq_runtime(cfs_rq);
if (likely(cfs_rq->runtime_remaining > 0))
return;
@@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
resched_curr(rq);
}
-static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
- u64 remaining, u64 expires)
+static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
{
struct cfs_rq *cfs_rq;
u64 runtime;
@@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
remaining -= runtime;
cfs_rq->runtime_remaining += runtime;
- cfs_rq->runtime_expires = expires;
/* we check whether we're throttled above */
if (cfs_rq->runtime_remaining > 0)
@@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
*/
static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
{
- u64 runtime, runtime_expires;
+ u64 runtime;
int throttled;
/* no need to continue the timer with no bandwidth constraint */
@@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
/* account preceding periods in which throttling occurred */
cfs_b->nr_throttled += overrun;
- runtime_expires = cfs_b->runtime_expires;
-
/*
* This check is repeated as we are holding onto the new bandwidth while
* we unthrottle. This can potentially race with an unthrottled group
@@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
cfs_b->distribute_running = 1;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
- runtime = distribute_cfs_runtime(cfs_b, runtime,
- runtime_expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
cfs_b->distribute_running = 0;
@@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
return;
raw_spin_lock(&cfs_b->lock);
- if (cfs_b->quota != RUNTIME_INF &&
- cfs_rq->runtime_expires == cfs_b->runtime_expires) {
+ if (cfs_b->quota != RUNTIME_INF) {
cfs_b->runtime += slack_runtime;
/* we are under rq->lock, defer unthrottling using a timer */
@@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
unsigned long flags;
- u64 expires;
/* confirm we're still not at a refresh boundary */
raw_spin_lock_irqsave(&cfs_b->lock, flags);
@@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
runtime = cfs_b->runtime;
- expires = cfs_b->runtime_expires;
if (runtime)
cfs_b->distribute_running = 1;
@@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (!runtime)
return;
- runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
- if (expires == cfs_b->runtime_expires)
- lsub_positive(&cfs_b->runtime, runtime);
cfs_b->distribute_running = 0;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
}
@@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
cfs_b->period_active = 1;
overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
- cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b52ed1a..0c0ed23 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -341,8 +341,6 @@ struct cfs_bandwidth {
u64 quota;
u64 runtime;
s64 hierarchical_quota;
- u64 runtime_expires;
- int expires_seq;
short idle;
short period_active;
@@ -562,8 +560,6 @@ struct cfs_rq {
#ifdef CONFIG_CFS_BANDWIDTH
int runtime_enabled;
- int expires_seq;
- u64 runtime_expires;
s64 runtime_remaining;
u64 throttled_clock;
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox