* [PATCH] powerpc/prom: move the device tree to the right space
From: Youlin Song @ 2021-03-03 5:00 UTC (permalink / raw)
To: mpe, benh, paulus, christophe.leroy
Cc: Youlin Song, linuxppc-dev, linux-kernel
If the device tree has been allocated memory and it will
be in the memblock reserved space.Obviously it is in a
valid memory declaration and will be mapped by the kernel.
Signed-off-by: Youlin Song <syl.loop@gmail.com>
---
arch/powerpc/kernel/prom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 9a4797d1d40d..ef5f93e7d7f2 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -121,7 +121,7 @@ static void __init move_device_tree(void)
size = fdt_totalsize(initial_boot_params);
if ((memory_limit && (start + size) > PHYSICAL_START + memory_limit) ||
- !memblock_is_memory(start + size - 1) ||
+ (!memblock_is_memory(start + size - 1) && !memblock_is_reserved(start + size - 1)) ||
overlaps_crashkernel(start, size) || overlaps_initrd(start, size)) {
p = memblock_alloc_raw(size, PAGE_SIZE);
if (!p)
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 40/44] tty: hvc, drop unneeded forward declarations
From: Uwe Kleine-König @ 2021-03-03 7:44 UTC (permalink / raw)
To: Jiri Slaby, Michael Ellerman
Cc: gregkh, linuxppc-dev, linux-kernel, linux-serial
In-Reply-To: <20210302062214.29627-40-jslaby@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]
Hello Jiri,
On Tue, Mar 02, 2021 at 07:22:10AM +0100, Jiri Slaby wrote:
> Forward declarations make the code larger and rewrites harder. Harder as
> they are often omitted from global changes. Remove forward declarations
> which are not really needed, i.e. the definition of the function is
> before its first use.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
> drivers/tty/hvc/hvcs.c | 25 -------------------------
note this conflicts with commit 386a966f5ce71a0364b158c5d0a6971f4e418ea8
that currently sits in the powerpc tree. I think it's easy to resolve.
Other than that:
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Best regards
Uwe
> 1 file changed, 25 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index c90848919644..0b89d878a108 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -290,36 +290,11 @@ static LIST_HEAD(hvcs_structs);
> static DEFINE_SPINLOCK(hvcs_structs_lock);
> static DEFINE_MUTEX(hvcs_init_mutex);
>
> -static void hvcs_unthrottle(struct tty_struct *tty);
> -static void hvcs_throttle(struct tty_struct *tty);
> -static irqreturn_t hvcs_handle_interrupt(int irq, void *dev_instance);
> -
> -static int hvcs_write(struct tty_struct *tty,
> - const unsigned char *buf, int count);
> -static int hvcs_write_room(struct tty_struct *tty);
> -static int hvcs_chars_in_buffer(struct tty_struct *tty);
> -
> -static int hvcs_has_pi(struct hvcs_struct *hvcsd);
> -static void hvcs_set_pi(struct hvcs_partner_info *pi,
> - struct hvcs_struct *hvcsd);
> static int hvcs_get_pi(struct hvcs_struct *hvcsd);
> static int hvcs_rescan_devices_list(void);
>
> -static int hvcs_partner_connect(struct hvcs_struct *hvcsd);
> static void hvcs_partner_free(struct hvcs_struct *hvcsd);
>
> -static int hvcs_enable_device(struct hvcs_struct *hvcsd,
> - uint32_t unit_address, unsigned int irq, struct vio_dev *dev);
> -
> -static int hvcs_open(struct tty_struct *tty, struct file *filp);
> -static void hvcs_close(struct tty_struct *tty, struct file *filp);
> -static void hvcs_hangup(struct tty_struct * tty);
> -
> -static int hvcs_probe(struct vio_dev *dev,
> - const struct vio_device_id *id);
> -static int hvcs_remove(struct vio_dev *dev);
> -static int __init hvcs_module_init(void);
> -static void __exit hvcs_module_exit(void);
> static int hvcs_initialize(void);
>
> #define HVCS_SCHED_READ 0x00000001
> --
> 2.30.1
>
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] ASoC: hdmi-codec: fix platform_no_drv_owner.cocci warnings
From: Yang Li @ 2021-03-03 8:54 UTC (permalink / raw)
To: timur
Cc: alsa-devel, linuxppc-dev, linux-kernel, Xiubo.Lee, festevam,
s.hauer, tiwai, lgirdwood, perex, nicoleotsuka, broonie, Yang Li,
linux-imx, kernel, shawnguo, shengjiu.wang, linux-arm-kernel
./sound/soc/fsl/imx-hdmi.c:226:3-8: No need to set .owner here. The core
will do it.
Remove .owner field if calls are used which set it automatically
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
---
sound/soc/fsl/imx-hdmi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c
index dbbb761..cd0235a 100644
--- a/sound/soc/fsl/imx-hdmi.c
+++ b/sound/soc/fsl/imx-hdmi.c
@@ -223,7 +223,6 @@ static int imx_hdmi_probe(struct platform_device *pdev)
static struct platform_driver imx_hdmi_driver = {
.driver = {
.name = "imx-hdmi",
- .owner = THIS_MODULE,
.pm = &snd_soc_pm_ops,
.of_match_table = imx_hdmi_dt_ids,
},
--
1.8.3.1
^ permalink raw reply related
* [PATCH][next] ASoC: fsl: fsl_easrc: Fix uninitialized variable st2_mem_alloc
From: Colin King @ 2021-03-03 9:18 UTC (permalink / raw)
To: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Pierre-Louis Bossart, alsa-devel, linuxppc-dev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
A previous cleanup commit removed the ininitialization of st2_mem_alloc.
Fix this by restoring the original behaviour by initializing it to zero.
Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: e80382fe721f ("ASoC: fsl: fsl_easrc: remove useless assignments")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
sound/soc/fsl/fsl_easrc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index 725a5d3aaa02..e823c9c13764 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -710,7 +710,7 @@ static int fsl_easrc_max_ch_for_slot(struct fsl_asrc_pair *ctx,
struct fsl_easrc_slot *slot)
{
struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
- int st1_mem_alloc = 0, st2_mem_alloc;
+ int st1_mem_alloc = 0, st2_mem_alloc = 0;
int pf_mem_alloc = 0;
int max_channels = 8 - slot->num_channel;
int channels = 0;
--
2.30.0
^ permalink raw reply related
* [PATCH next v4 00/15] printk: remove logbuf_lock
From: John Ogness @ 2021-03-03 10:15 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson, linux-mtd,
Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Daniel Thompson, Madhavan Srinivasan,
Stephen Hemminger, Richard Weinberger, Anton Vorontsov,
Jordan Niethe, Anton Ivanov, Wei Li, Haiyang Zhang, Ravi Bangoria,
Kees Cook, Alistair Popple, Jeff Dike, Colin Cross, linux-um,
Wei Liu, Steven Rostedt, Davidlohr Bueso, Nicholas Piggin,
Oleg Nesterov, Thomas Gleixner, Andy Shevchenko, Michael Kelley,
Christophe Leroy, Sumit Garg, Tony Luck, Pavel Tatashin,
linux-kernel, Sergey Senozhatsky, Jason Wessel, kgdb-bugreport,
Paul Mackerras, linuxppc-dev, Mike Rapoport
Hello,
Here is v4 of a series to remove @logbuf_lock, exposing the
ringbuffer locklessly to both readers and writers. v3 is
here [0].
Since @logbuf_lock was protecting much more than just the
ringbuffer, this series clarifies and cleans up the various
protections using comments, lockless accessors, atomic types,
and a new finer-grained @syslog_lock.
Removing @logbuf_lock required changing the semantics of the
kmsg_dumper callback in order to work locklessly. This series
adjusts all kmsg_dumpers and users of the kmsg_dump_get_*()
functions for the new semantics.
This series is based on next-20210303.
Changes since v3:
- disable interrupts in the arch/um kmsg_dumper
- reduce CONSOLE_LOG_MAX value from 4096 back to 1024 to revert
the increasd 3KiB static memory footprint
- change the kmsg_dumper() callback prototype back to how it
was because some dumpers need the registered object for
container_of() usage
- for kmsg_dump_get_line()/kmsg_dump_get_buffer() restrict the
minimal allowed sequence number to the cleared sequence number
John Ogness
[0] https://lore.kernel.org/lkml/20210225202438.28985-1-john.ogness@linutronix.de/
John Ogness (15):
um: synchronize kmsg_dumper
mtd: mtdoops: synchronize kmsg_dumper
printk: limit second loop of syslog_print_all
printk: kmsg_dump: remove unused fields
printk: refactor kmsg_dump_get_buffer()
printk: consolidate kmsg_dump_get_buffer/syslog_print_all code
printk: introduce CONSOLE_LOG_MAX
printk: use seqcount_latch for clear_seq
printk: use atomic64_t for devkmsg_user.seq
printk: add syslog_lock
printk: kmsg_dumper: remove @active field
printk: introduce a kmsg_dump iterator
printk: remove logbuf_lock
printk: kmsg_dump: remove _nolock() variants
printk: console: remove unnecessary safe buffer usage
arch/powerpc/kernel/nvram_64.c | 8 +-
arch/powerpc/xmon/xmon.c | 6 +-
arch/um/kernel/kmsg_dump.c | 13 +-
drivers/hv/vmbus_drv.c | 4 +-
drivers/mtd/mtdoops.c | 17 +-
fs/pstore/platform.c | 5 +-
include/linux/kmsg_dump.h | 47 ++--
kernel/debug/kdb/kdb_main.c | 10 +-
kernel/printk/internal.h | 4 +-
kernel/printk/printk.c | 464 +++++++++++++++++----------------
kernel/printk/printk_safe.c | 27 +-
11 files changed, 310 insertions(+), 295 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH next v4 11/15] printk: kmsg_dumper: remove @active field
From: John Ogness @ 2021-03-03 10:15 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Douglas Anderson, Paul Mackerras, Kees Cook,
Daniel Thompson, Madhavan Srinivasan, Jordan Niethe, Wei Li,
Ravi Bangoria, Pavel Tatashin, Alistair Popple, Steven Rostedt,
Davidlohr Bueso, Nicholas Piggin, Oleg Nesterov, Thomas Gleixner,
linux-kernel, Sergey Senozhatsky, Jason Wessel, kgdb-bugreport,
linuxppc-dev, Mike Rapoport
In-Reply-To: <20210303101528.29901-1-john.ogness@linutronix.de>
All 6 kmsg_dumpers do not benefit from the @active flag:
(provide their own synchronization)
- arch/powerpc/kernel/nvram_64.c
- arch/um/kernel/kmsg_dump.c
- drivers/mtd/mtdoops.c
- fs/pstore/platform.c
(only dump on KMSG_DUMP_PANIC, which does not require
synchronization)
- arch/powerpc/platforms/powernv/opal-kmsg.c
- drivers/hv/vmbus_drv.c
The other 2 kmsg_dump users also do not rely on @active:
(hard-code @active to always be true)
- arch/powerpc/xmon/xmon.c
- kernel/debug/kdb/kdb_main.c
Therefore, @active can be removed.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
arch/powerpc/xmon/xmon.c | 2 +-
include/linux/kmsg_dump.h | 2 --
kernel/debug/kdb/kdb_main.c | 2 +-
kernel/printk/printk.c | 10 +---------
4 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3fe37495f63d..80ed3e1becf9 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3001,7 +3001,7 @@ print_address(unsigned long addr)
static void
dump_log_buf(void)
{
- struct kmsg_dumper dumper = { .active = 1 };
+ struct kmsg_dumper dumper;
unsigned char buf[128];
size_t len;
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 070c994ff19f..84eaa2090efa 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -36,7 +36,6 @@ enum kmsg_dump_reason {
* through the record iterator
* @max_reason: filter for highest reason number that should be dumped
* @registered: Flag that specifies if this is already registered
- * @active: Flag that specifies if this is currently dumping
* @cur_seq: Points to the oldest message to dump
* @next_seq: Points after the newest message to dump
*/
@@ -44,7 +43,6 @@ struct kmsg_dumper {
struct list_head list;
void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason);
enum kmsg_dump_reason max_reason;
- bool active;
bool registered;
/* private state of the kmsg iterator */
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 930ac1b25ec7..315169d5e119 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2101,7 +2101,7 @@ static int kdb_dmesg(int argc, const char **argv)
int adjust = 0;
int n = 0;
int skip = 0;
- struct kmsg_dumper dumper = { .active = 1 };
+ struct kmsg_dumper dumper;
size_t len;
char buf[201];
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e794a08de00f..ce4cc64ba7c9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3408,8 +3408,6 @@ void kmsg_dump(enum kmsg_dump_reason reason)
continue;
/* initialize iterator with data about the stored records */
- dumper->active = true;
-
logbuf_lock_irqsave(flags);
dumper->cur_seq = latched_seq_read_nolock(&clear_seq);
dumper->next_seq = prb_next_seq(prb);
@@ -3417,9 +3415,6 @@ void kmsg_dump(enum kmsg_dump_reason reason)
/* invoke dumper which will iterate over records */
dumper->dump(dumper, reason);
-
- /* reset iterator */
- dumper->active = false;
}
rcu_read_unlock();
}
@@ -3454,9 +3449,6 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
prb_rec_init_rd(&r, &info, line, size);
- if (!dumper->active)
- goto out;
-
/* Read text or count text lines? */
if (line) {
if (!prb_read_valid(prb, dumper->cur_seq, &r))
@@ -3542,7 +3534,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
bool ret = false;
bool time = printk_time;
- if (!dumper->active || !buf || !size)
+ if (!buf || !size)
goto out;
logbuf_lock_irqsave(flags);
--
2.20.1
^ permalink raw reply related
* [PATCH next v4 12/15] printk: introduce a kmsg_dump iterator
From: John Ogness @ 2021-03-03 10:15 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson,
Paul Mackerras, Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Wei Liu, Madhavan Srinivasan,
Stephen Hemminger, Anton Vorontsov, Jason Wessel, Anton Ivanov,
Wei Li, Haiyang Zhang, Ravi Bangoria, Kees Cook, Alistair Popple,
Jeff Dike, Colin Cross, linux-um, Daniel Thompson, Steven Rostedt,
Davidlohr Bueso, Nicholas Piggin, Oleg Nesterov, Thomas Gleixner,
Andy Shevchenko, Jordan Niethe, Michael Kelley, Christophe Leroy,
Tony Luck, Pavel Tatashin, linux-kernel, Sergey Senozhatsky,
Richard Weinberger, kgdb-bugreport, linux-mtd, linuxppc-dev,
Mike Rapoport
In-Reply-To: <20210303101528.29901-1-john.ogness@linutronix.de>
Rather than storing the iterator information in the registered
kmsg_dumper structure, create a separate iterator structure. The
kmsg_dump_iter structure can reside on the stack of the caller, thus
allowing lockless use of the kmsg_dump functions.
Update code that accesses the kernel logs using the kmsg_dumper
structure to use the new kmsg_dump_iter structure. For kmsg_dumpers,
this also means adding a call to kmsg_dump_rewind() to initialize
the iterator.
All this is in preparation for removal of @logbuf_lock.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Kees Cook <keescook@chromium.org> # pstore
---
arch/powerpc/kernel/nvram_64.c | 8 +++--
arch/powerpc/xmon/xmon.c | 6 ++--
arch/um/kernel/kmsg_dump.c | 5 ++-
drivers/hv/vmbus_drv.c | 4 ++-
drivers/mtd/mtdoops.c | 5 ++-
fs/pstore/platform.c | 5 ++-
include/linux/kmsg_dump.h | 36 ++++++++++---------
kernel/debug/kdb/kdb_main.c | 10 +++---
kernel/printk/printk.c | 63 +++++++++++++++++-----------------
9 files changed, 80 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 532f22637783..3c8d9bbb51cf 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -647,6 +647,7 @@ static void oops_to_nvram(struct kmsg_dumper *dumper,
{
struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
static unsigned int oops_count = 0;
+ static struct kmsg_dump_iter iter;
static bool panicking = false;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
@@ -681,13 +682,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper,
return;
if (big_oops_buf) {
- kmsg_dump_get_buffer(dumper, false,
+ kmsg_dump_rewind(&iter);
+ kmsg_dump_get_buffer(&iter, false,
big_oops_buf, big_oops_buf_sz, &text_len);
rc = zip_oops(text_len);
}
if (rc != 0) {
- kmsg_dump_rewind(dumper);
- kmsg_dump_get_buffer(dumper, false,
+ kmsg_dump_rewind(&iter);
+ kmsg_dump_get_buffer(&iter, false,
oops_data, oops_data_sz, &text_len);
err_type = ERR_TYPE_KERNEL_PANIC;
oops_hdr->version = cpu_to_be16(OOPS_HDR_VERSION);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 80ed3e1becf9..5978b90a885f 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3001,7 +3001,7 @@ print_address(unsigned long addr)
static void
dump_log_buf(void)
{
- struct kmsg_dumper dumper;
+ struct kmsg_dump_iter iter;
unsigned char buf[128];
size_t len;
@@ -3013,9 +3013,9 @@ dump_log_buf(void)
catch_memory_errors = 1;
sync();
- kmsg_dump_rewind_nolock(&dumper);
+ kmsg_dump_rewind_nolock(&iter);
xmon_start_pagination();
- while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) {
+ while (kmsg_dump_get_line_nolock(&iter, false, buf, sizeof(buf), &len)) {
buf[len] = '\0';
printf("%s", buf);
}
diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index a765d235e50e..0224fcb36e22 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -10,6 +10,7 @@
static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
enum kmsg_dump_reason reason)
{
+ static struct kmsg_dump_iter iter;
static DEFINE_SPINLOCK(lock);
static char line[1024];
struct console *con;
@@ -35,8 +36,10 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
if (!spin_trylock_irqsave(&lock, flags))
return;
+ kmsg_dump_rewind(&iter);
+
printf("kmsg_dump:\n");
- while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) {
+ while (kmsg_dump_get_line(&iter, true, line, sizeof(line), &len)) {
line[len] = '\0';
printf("%s", line);
}
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 10dce9f91216..b341b144bde8 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1391,6 +1391,7 @@ static void vmbus_isr(void)
static void hv_kmsg_dump(struct kmsg_dumper *dumper,
enum kmsg_dump_reason reason)
{
+ struct kmsg_dump_iter iter;
size_t bytes_written;
phys_addr_t panic_pa;
@@ -1404,7 +1405,8 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
* Write dump contents to the page. No need to synchronize; panic should
* be single-threaded.
*/
- kmsg_dump_get_buffer(dumper, false, hv_panic_page, HV_HYP_PAGE_SIZE,
+ kmsg_dump_rewind(&iter);
+ kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
&bytes_written);
if (bytes_written)
hyperv_report_panic_msg(panic_pa, bytes_written);
diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 8bbfba40a554..862c4a889234 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -277,14 +277,17 @@ static void mtdoops_do_dump(struct kmsg_dumper *dumper,
{
struct mtdoops_context *cxt = container_of(dumper,
struct mtdoops_context, dump);
+ struct kmsg_dump_iter iter;
/* Only dump oopses if dump_oops is set */
if (reason == KMSG_DUMP_OOPS && !dump_oops)
return;
+ kmsg_dump_rewind(&iter);
+
if (test_and_set_bit(0, &cxt->oops_buf_busy))
return;
- kmsg_dump_get_buffer(dumper, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE,
+ kmsg_dump_get_buffer(&iter, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE,
record_size - MTDOOPS_HEADER_SIZE, NULL);
clear_bit(0, &cxt->oops_buf_busy);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d963ae7902f9..b9614db48b1d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -385,6 +385,7 @@ void pstore_record_init(struct pstore_record *record,
static void pstore_dump(struct kmsg_dumper *dumper,
enum kmsg_dump_reason reason)
{
+ struct kmsg_dump_iter iter;
unsigned long total = 0;
const char *why;
unsigned int part = 1;
@@ -405,6 +406,8 @@ static void pstore_dump(struct kmsg_dumper *dumper,
}
}
+ kmsg_dump_rewind(&iter);
+
oopscount++;
while (total < kmsg_bytes) {
char *dst;
@@ -435,7 +438,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
dst_size -= header_size;
/* Write dump contents. */
- if (!kmsg_dump_get_buffer(dumper, true, dst + header_size,
+ if (!kmsg_dump_get_buffer(&iter, true, dst + header_size,
dst_size, &dump_size))
break;
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 84eaa2090efa..36c8c57e1051 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -29,6 +29,16 @@ enum kmsg_dump_reason {
KMSG_DUMP_MAX
};
+/**
+ * struct kmsg_dump_iter - iterator for retrieving kernel messages
+ * @cur_seq: Points to the oldest message to dump
+ * @next_seq: Points after the newest message to dump
+ */
+struct kmsg_dump_iter {
+ u64 cur_seq;
+ u64 next_seq;
+};
+
/**
* struct kmsg_dumper - kernel crash message dumper structure
* @list: Entry in the dumper list (private)
@@ -36,35 +46,29 @@ enum kmsg_dump_reason {
* through the record iterator
* @max_reason: filter for highest reason number that should be dumped
* @registered: Flag that specifies if this is already registered
- * @cur_seq: Points to the oldest message to dump
- * @next_seq: Points after the newest message to dump
*/
struct kmsg_dumper {
struct list_head list;
void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason);
enum kmsg_dump_reason max_reason;
bool registered;
-
- /* private state of the kmsg iterator */
- u64 cur_seq;
- u64 next_seq;
};
#ifdef CONFIG_PRINTK
void kmsg_dump(enum kmsg_dump_reason reason);
-bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog,
char *line, size_t size, size_t *len);
-bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
char *line, size_t size, size_t *len);
-bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog,
char *buf, size_t size, size_t *len_out);
-void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper);
+void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter);
-void kmsg_dump_rewind(struct kmsg_dumper *dumper);
+void kmsg_dump_rewind(struct kmsg_dump_iter *iter);
int kmsg_dump_register(struct kmsg_dumper *dumper);
@@ -76,30 +80,30 @@ static inline void kmsg_dump(enum kmsg_dump_reason reason)
{
}
-static inline bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper,
+static inline bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter,
bool syslog, const char *line,
size_t size, size_t *len)
{
return false;
}
-static inline bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog,
+static inline bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
const char *line, size_t size, size_t *len)
{
return false;
}
-static inline bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
+static inline bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog,
char *buf, size_t size, size_t *len)
{
return false;
}
-static inline void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper)
+static inline void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter)
{
}
-static inline void kmsg_dump_rewind(struct kmsg_dumper *dumper)
+static inline void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
{
}
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 315169d5e119..8544d7a55a57 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2101,7 +2101,7 @@ static int kdb_dmesg(int argc, const char **argv)
int adjust = 0;
int n = 0;
int skip = 0;
- struct kmsg_dumper dumper;
+ struct kmsg_dump_iter iter;
size_t len;
char buf[201];
@@ -2126,8 +2126,8 @@ static int kdb_dmesg(int argc, const char **argv)
kdb_set(2, setargs);
}
- kmsg_dump_rewind_nolock(&dumper);
- while (kmsg_dump_get_line_nolock(&dumper, 1, NULL, 0, NULL))
+ kmsg_dump_rewind_nolock(&iter);
+ while (kmsg_dump_get_line_nolock(&iter, 1, NULL, 0, NULL))
n++;
if (lines < 0) {
@@ -2159,8 +2159,8 @@ static int kdb_dmesg(int argc, const char **argv)
if (skip >= n || skip < 0)
return 0;
- kmsg_dump_rewind_nolock(&dumper);
- while (kmsg_dump_get_line_nolock(&dumper, 1, buf, sizeof(buf), &len)) {
+ kmsg_dump_rewind_nolock(&iter);
+ while (kmsg_dump_get_line_nolock(&iter, 1, buf, sizeof(buf), &len)) {
if (skip) {
skip--;
continue;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ce4cc64ba7c9..b49dee256947 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3390,7 +3390,6 @@ EXPORT_SYMBOL_GPL(kmsg_dump_reason_str);
void kmsg_dump(enum kmsg_dump_reason reason)
{
struct kmsg_dumper *dumper;
- unsigned long flags;
rcu_read_lock();
list_for_each_entry_rcu(dumper, &dump_list, list) {
@@ -3407,12 +3406,6 @@ void kmsg_dump(enum kmsg_dump_reason reason)
if (reason > max_reason)
continue;
- /* initialize iterator with data about the stored records */
- logbuf_lock_irqsave(flags);
- dumper->cur_seq = latched_seq_read_nolock(&clear_seq);
- dumper->next_seq = prb_next_seq(prb);
- logbuf_unlock_irqrestore(flags);
-
/* invoke dumper which will iterate over records */
dumper->dump(dumper, reason);
}
@@ -3421,7 +3414,7 @@ void kmsg_dump(enum kmsg_dump_reason reason)
/**
* kmsg_dump_get_line_nolock - retrieve one kmsg log line (unlocked version)
- * @dumper: registered kmsg dumper
+ * @iter: kmsg dump iterator
* @syslog: include the "<4>" prefixes
* @line: buffer to copy the line to
* @size: maximum size of the buffer
@@ -3438,24 +3431,28 @@ void kmsg_dump(enum kmsg_dump_reason reason)
*
* The function is similar to kmsg_dump_get_line(), but grabs no locks.
*/
-bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog,
char *line, size_t size, size_t *len)
{
+ u64 min_seq = latched_seq_read_nolock(&clear_seq);
struct printk_info info;
unsigned int line_count;
struct printk_record r;
size_t l = 0;
bool ret = false;
+ if (iter->cur_seq < min_seq)
+ iter->cur_seq = min_seq;
+
prb_rec_init_rd(&r, &info, line, size);
/* Read text or count text lines? */
if (line) {
- if (!prb_read_valid(prb, dumper->cur_seq, &r))
+ if (!prb_read_valid(prb, iter->cur_seq, &r))
goto out;
l = record_print_text(&r, syslog, printk_time);
} else {
- if (!prb_read_valid_info(prb, dumper->cur_seq,
+ if (!prb_read_valid_info(prb, iter->cur_seq,
&info, &line_count)) {
goto out;
}
@@ -3464,7 +3461,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
}
- dumper->cur_seq = r.info->seq + 1;
+ iter->cur_seq = r.info->seq + 1;
ret = true;
out:
if (len)
@@ -3474,7 +3471,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
/**
* kmsg_dump_get_line - retrieve one kmsg log line
- * @dumper: registered kmsg dumper
+ * @iter: kmsg dump iterator
* @syslog: include the "<4>" prefixes
* @line: buffer to copy the line to
* @size: maximum size of the buffer
@@ -3489,14 +3486,14 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
* A return value of FALSE indicates that there are no more records to
* read.
*/
-bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
char *line, size_t size, size_t *len)
{
unsigned long flags;
bool ret;
logbuf_lock_irqsave(flags);
- ret = kmsg_dump_get_line_nolock(dumper, syslog, line, size, len);
+ ret = kmsg_dump_get_line_nolock(iter, syslog, line, size, len);
logbuf_unlock_irqrestore(flags);
return ret;
@@ -3505,7 +3502,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line);
/**
* kmsg_dump_get_buffer - copy kmsg log lines
- * @dumper: registered kmsg dumper
+ * @iter: kmsg dump iterator
* @syslog: include the "<4>" prefixes
* @buf: buffer to copy the line to
* @size: maximum size of the buffer
@@ -3522,9 +3519,10 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line);
* A return value of FALSE indicates that there are no more records to
* read.
*/
-bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
+bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog,
char *buf, size_t size, size_t *len_out)
{
+ u64 min_seq = latched_seq_read_nolock(&clear_seq);
struct printk_info info;
struct printk_record r;
unsigned long flags;
@@ -3537,16 +3535,19 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
if (!buf || !size)
goto out;
+ if (iter->cur_seq < min_seq)
+ iter->cur_seq = min_seq;
+
logbuf_lock_irqsave(flags);
- if (prb_read_valid_info(prb, dumper->cur_seq, &info, NULL)) {
- if (info.seq != dumper->cur_seq) {
+ if (prb_read_valid_info(prb, iter->cur_seq, &info, NULL)) {
+ if (info.seq != iter->cur_seq) {
/* messages are gone, move to first available one */
- dumper->cur_seq = info.seq;
+ iter->cur_seq = info.seq;
}
}
/* last entry */
- if (dumper->cur_seq >= dumper->next_seq) {
+ if (iter->cur_seq >= iter->next_seq) {
logbuf_unlock_irqrestore(flags);
goto out;
}
@@ -3557,7 +3558,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
* because this function (by way of record_print_text()) will
* not write more than size-1 bytes of text into @buf.
*/
- seq = find_first_fitting_seq(dumper->cur_seq, dumper->next_seq,
+ seq = find_first_fitting_seq(iter->cur_seq, iter->next_seq,
size - 1, syslog, time);
/*
@@ -3570,7 +3571,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
len = 0;
prb_for_each_record(seq, prb, seq, &r) {
- if (r.info->seq >= dumper->next_seq)
+ if (r.info->seq >= iter->next_seq)
break;
len += record_print_text(&r, syslog, time);
@@ -3579,7 +3580,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
prb_rec_init_rd(&r, &info, buf + len, size - len);
}
- dumper->next_seq = next_seq;
+ iter->next_seq = next_seq;
ret = true;
logbuf_unlock_irqrestore(flags);
out:
@@ -3591,7 +3592,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer);
/**
* kmsg_dump_rewind_nolock - reset the iterator (unlocked version)
- * @dumper: registered kmsg dumper
+ * @iter: kmsg dump iterator
*
* Reset the dumper's iterator so that kmsg_dump_get_line() and
* kmsg_dump_get_buffer() can be called again and used multiple
@@ -3599,26 +3600,26 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer);
*
* The function is similar to kmsg_dump_rewind(), but grabs no locks.
*/
-void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper)
+void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter)
{
- dumper->cur_seq = latched_seq_read_nolock(&clear_seq);
- dumper->next_seq = prb_next_seq(prb);
+ iter->cur_seq = latched_seq_read_nolock(&clear_seq);
+ iter->next_seq = prb_next_seq(prb);
}
/**
* kmsg_dump_rewind - reset the iterator
- * @dumper: registered kmsg dumper
+ * @iter: kmsg dump iterator
*
* Reset the dumper's iterator so that kmsg_dump_get_line() and
* kmsg_dump_get_buffer() can be called again and used multiple
* times within the same dumper.dump() callback.
*/
-void kmsg_dump_rewind(struct kmsg_dumper *dumper)
+void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
{
unsigned long flags;
logbuf_lock_irqsave(flags);
- kmsg_dump_rewind_nolock(dumper);
+ kmsg_dump_rewind_nolock(iter);
logbuf_unlock_irqrestore(flags);
}
EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
--
2.20.1
^ permalink raw reply related
* [PATCH next v4 14/15] printk: kmsg_dump: remove _nolock() variants
From: John Ogness @ 2021-03-03 10:15 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Douglas Anderson, Paul Mackerras, Kees Cook,
Daniel Thompson, Madhavan Srinivasan, Jordan Niethe, Wei Li,
Ravi Bangoria, Pavel Tatashin, Alistair Popple, Steven Rostedt,
Davidlohr Bueso, Nicholas Piggin, Thomas Gleixner, Sumit Garg,
linux-kernel, Sergey Senozhatsky, Jason Wessel, kgdb-bugreport,
linuxppc-dev, Mike Rapoport
In-Reply-To: <20210303101528.29901-1-john.ogness@linutronix.de>
kmsg_dump_rewind() and kmsg_dump_get_line() are lockless, so there is
no need for _nolock() variants. Remove these functions and switch all
callers of the _nolock() variants.
The functions without _nolock() were chosen because they are already
exported to kernel modules.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
arch/powerpc/xmon/xmon.c | 4 +--
include/linux/kmsg_dump.h | 16 ----------
kernel/debug/kdb/kdb_main.c | 8 ++---
kernel/printk/printk.c | 60 +++++--------------------------------
4 files changed, 14 insertions(+), 74 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 5978b90a885f..bf7d69625a2e 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3013,9 +3013,9 @@ dump_log_buf(void)
catch_memory_errors = 1;
sync();
- kmsg_dump_rewind_nolock(&iter);
+ kmsg_dump_rewind(&iter);
xmon_start_pagination();
- while (kmsg_dump_get_line_nolock(&iter, false, buf, sizeof(buf), &len)) {
+ while (kmsg_dump_get_line(&iter, false, buf, sizeof(buf), &len)) {
buf[len] = '\0';
printf("%s", buf);
}
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 36c8c57e1051..906521c2329c 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -57,17 +57,12 @@ struct kmsg_dumper {
#ifdef CONFIG_PRINTK
void kmsg_dump(enum kmsg_dump_reason reason);
-bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog,
- char *line, size_t size, size_t *len);
-
bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
char *line, size_t size, size_t *len);
bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog,
char *buf, size_t size, size_t *len_out);
-void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter);
-
void kmsg_dump_rewind(struct kmsg_dump_iter *iter);
int kmsg_dump_register(struct kmsg_dumper *dumper);
@@ -80,13 +75,6 @@ static inline void kmsg_dump(enum kmsg_dump_reason reason)
{
}
-static inline bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter,
- bool syslog, const char *line,
- size_t size, size_t *len)
-{
- return false;
-}
-
static inline bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
const char *line, size_t size, size_t *len)
{
@@ -99,10 +87,6 @@ static inline bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog
return false;
}
-static inline void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter)
-{
-}
-
static inline void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
{
}
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 8544d7a55a57..67d9f2403b52 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2126,8 +2126,8 @@ static int kdb_dmesg(int argc, const char **argv)
kdb_set(2, setargs);
}
- kmsg_dump_rewind_nolock(&iter);
- while (kmsg_dump_get_line_nolock(&iter, 1, NULL, 0, NULL))
+ kmsg_dump_rewind(&iter);
+ while (kmsg_dump_get_line(&iter, 1, NULL, 0, NULL))
n++;
if (lines < 0) {
@@ -2159,8 +2159,8 @@ static int kdb_dmesg(int argc, const char **argv)
if (skip >= n || skip < 0)
return 0;
- kmsg_dump_rewind_nolock(&iter);
- while (kmsg_dump_get_line_nolock(&iter, 1, buf, sizeof(buf), &len)) {
+ kmsg_dump_rewind(&iter);
+ while (kmsg_dump_get_line(&iter, 1, buf, sizeof(buf), &len)) {
if (skip) {
skip--;
continue;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8994bc192b88..602de86d4e76 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3373,7 +3373,7 @@ void kmsg_dump(enum kmsg_dump_reason reason)
}
/**
- * kmsg_dump_get_line_nolock - retrieve one kmsg log line (unlocked version)
+ * kmsg_dump_get_line - retrieve one kmsg log line
* @iter: kmsg dump iterator
* @syslog: include the "<4>" prefixes
* @line: buffer to copy the line to
@@ -3388,22 +3388,22 @@ void kmsg_dump(enum kmsg_dump_reason reason)
*
* A return value of FALSE indicates that there are no more records to
* read.
- *
- * The function is similar to kmsg_dump_get_line(), but grabs no locks.
*/
-bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog,
- char *line, size_t size, size_t *len)
+bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
+ char *line, size_t size, size_t *len)
{
u64 min_seq = latched_seq_read_nolock(&clear_seq);
struct printk_info info;
unsigned int line_count;
struct printk_record r;
+ unsigned long flags;
size_t l = 0;
bool ret = false;
if (iter->cur_seq < min_seq)
iter->cur_seq = min_seq;
+ printk_safe_enter_irqsave(flags);
prb_rec_init_rd(&r, &info, line, size);
/* Read text or count text lines? */
@@ -3424,40 +3424,11 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog,
iter->cur_seq = r.info->seq + 1;
ret = true;
out:
+ printk_safe_exit_irqrestore(flags);
if (len)
*len = l;
return ret;
}
-
-/**
- * kmsg_dump_get_line - retrieve one kmsg log line
- * @iter: kmsg dump iterator
- * @syslog: include the "<4>" prefixes
- * @line: buffer to copy the line to
- * @size: maximum size of the buffer
- * @len: length of line placed into buffer
- *
- * Start at the beginning of the kmsg buffer, with the oldest kmsg
- * record, and copy one record into the provided buffer.
- *
- * Consecutive calls will return the next available record moving
- * towards the end of the buffer with the youngest messages.
- *
- * A return value of FALSE indicates that there are no more records to
- * read.
- */
-bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog,
- char *line, size_t size, size_t *len)
-{
- unsigned long flags;
- bool ret;
-
- printk_safe_enter_irqsave(flags);
- ret = kmsg_dump_get_line_nolock(iter, syslog, line, size, len);
- printk_safe_exit_irqrestore(flags);
-
- return ret;
-}
EXPORT_SYMBOL_GPL(kmsg_dump_get_line);
/**
@@ -3550,22 +3521,6 @@ bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog,
}
EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer);
-/**
- * kmsg_dump_rewind_nolock - reset the iterator (unlocked version)
- * @iter: kmsg dump iterator
- *
- * Reset the dumper's iterator so that kmsg_dump_get_line() and
- * kmsg_dump_get_buffer() can be called again and used multiple
- * times within the same dumper.dump() callback.
- *
- * The function is similar to kmsg_dump_rewind(), but grabs no locks.
- */
-void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter)
-{
- iter->cur_seq = latched_seq_read_nolock(&clear_seq);
- iter->next_seq = prb_next_seq(prb);
-}
-
/**
* kmsg_dump_rewind - reset the iterator
* @iter: kmsg dump iterator
@@ -3579,7 +3534,8 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
unsigned long flags;
printk_safe_enter_irqsave(flags);
- kmsg_dump_rewind_nolock(iter);
+ iter->cur_seq = latched_seq_read_nolock(&clear_seq);
+ iter->next_seq = prb_next_seq(prb);
printk_safe_exit_irqrestore(flags);
}
EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
--
2.20.1
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-03 10:28 UTC (permalink / raw)
To: Michael Ellerman, Marco Elver
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <87h7ltss18.fsf@mpe.ellerman.id.au>
Le 02/03/2021 à 12:40, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 02/03/2021 à 10:53, Marco Elver a écrit :
>>> On Tue, 2 Mar 2021 at 10:27, Christophe Leroy
>>> <christophe.leroy@csgroup.eu> wrote:
>>>> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit :
>>>>>> [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>>>> [ 14.998426]
>>>>>> [ 15.007061] Invalid read at 0x(ptrval):
>>>>>> [ 15.010906] finish_task_switch.isra.0+0x54/0x23c
>>>>>> [ 15.015633] kunit_try_run_case+0x5c/0xd0
>>>>>> [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>>>> [ 15.025099] kthread+0x15c/0x174
>>>>>> [ 15.028359] ret_from_kernel_thread+0x14/0x1c
>>>>>> [ 15.032747]
>>>>>> [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>>>> [ 15.045811] ==================================================================
>>>>>> [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>>>> [ 15.053324] Expected report_matches(&expect) to be true, but is false
>>>>>> [ 15.068359] not ok 21 - test_invalid_access
>>>>>
>>>>> The test expects the function name to be test_invalid_access, i. e.
>>>>> the first line should be "BUG: KFENCE: invalid read in
>>>>> test_invalid_access".
>>>>> The error reporting function unwinds the stack, skips a couple of
>>>>> "uninteresting" frames
>>>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43)
>>>>> and uses the first "interesting" one frame to print the report header
>>>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226).
>>>>>
>>>>> It's strange that test_invalid_access is missing altogether from the
>>>>> stack trace - is that expected?
>>>>> Can you try printing the whole stacktrace without skipping any frames
>>>>> to see if that function is there?
>>>>>
>>>>
>>>> Booting with 'no_hash_pointers" I get the following. Does it helps ?
>>>>
>>>> [ 16.837198] ==================================================================
>>>> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.848521]
>>>> [ 16.857158] Invalid read at 0xdf98800a:
>>>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.865731] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.875199] kthread+0x15c/0x174
>>>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.882847]
>>>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
>>>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
>>>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
>>>> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
>>>> [ 16.918153] DAR: df98800a DSISR: 20000000
>>>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
>>>> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
>>>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
>>>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.947292] Call Trace:
>>>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
>>>
>>> The "(unreliable)" might be a clue that it's related to ppc32 stack
>>> unwinding. Any ppc expert know what this is about?
>>>
>>>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
>>>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.981896] Instruction dump:
>>>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
>>>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
>>>> [ 17.000711] ==================================================================
>>>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>> [ 17.008223] Expected report_matches(&expect) to be true, but is false
>>>> [ 17.023243] not ok 21 - test_invalid_access
>>>
>>> On a fault in test_invalid_access, KFENCE prints the stack trace based
>>> on the information in pt_regs. So we do not think there's anything we
>>> can do to improve stack printing pe-se.
>>
>> stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does.
>>
>> IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests,
>> there is some function call being done before the fault, for instance
>> test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the
>> call in the stack. However this is fragile.
>>
>> This works for function calls because in order to call a subfunction, a function has to set up a
>> stack frame in order to same the value in the Link Register, which contains the address of the
>> function's parent and that will be clobbered by the sub-function call.
>>
>> However, it cannot be done by exceptions, because exceptions can happen in a function that has no
>> stack frame (because that function has no need to call a subfunction and doesn't need to same
>> anything on the stack). If the exception handler was writting the caller's address in the stack
>> frame, it would in fact write it in the parent's frame, leading to a mess.
>>
>> But in fact the information is in pt_regs, it is in regs->nip so KFENCE should be able to use that
>> instead of the stack.
>>
>>>
>>> What's confusing is that it's only this test, and none of the others.
>>> Given that, it might be code-gen related, which results in some subtle
>>> issue with stack unwinding. There are a few things to try, if you feel
>>> like it:
>>>
>>> -- Change the unwinder, if it's possible for ppc32.
>>
>> I don't think it is possible.
>
> I think this actually is the solution.
>
> It seems the good architectures have all added support for
> arch_stack_walk(), and we have not.
>
> Looking at some of the implementations of arch_stack_walk() it seems
> it's expected that the first entry emitted includes the PC (or NIP on
> ppc).
I don't see a direct link between arch_stack_walk() and that expectation. Looks like those
architectures where already doing this before being converted to arch_stack_walk().
>
> For us stack_trace_save() calls save_stack_trace() which only emits
> entries from the stack, which doesn't necessarily include the function
> NIP is pointing to.
Yes, as the name save_stack says, it emits the entries from the stack. I think it is correct.
>
> So I think it's probably on us to update to that new API. Or at least
> update our save_stack_trace() to fabricate an entry using the NIP, as it
> seems that's what callers expect.
As mentionned above, that doesn't seem to be directly linked to the new API. That new API only is an
intermediate step anyway, the consumers like KFENCE still use the old API which is serviced by the
generic code now.
For me it looks odd to present a stack trace where entry[0] is not from the stack and where entry[1]
is unreliable (possibly non existing) because we don't know if we are coming from a frameless
function or not and even if the function as a frame we don't know if it saved LR in the parent's
frame or not.
I would deeply prefer if KFENCE could avoid making assumptions on entry[0] of the stack trace.
What about the following change to KFENCE ?
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index ab83d5a59bb1..c2fef4eeb192 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -171,12 +171,15 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r
const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1;
int num_stack_entries;
int skipnr = 0;
+ void *ip;
if (regs) {
num_stack_entries = stack_trace_save_regs(regs, stack_entries, KFENCE_STACK_DEPTH, 0);
+ ip = (void *)instruction_pointer(regs);
} else {
num_stack_entries = stack_trace_save(stack_entries, KFENCE_STACK_DEPTH, 1);
skipnr = get_stack_skipnr(stack_entries, num_stack_entries, &type);
+ ip = (void *)stack_entries[skipnr];
}
/* Require non-NULL meta, except if KFENCE_ERROR_INVALID. */
@@ -202,8 +205,7 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r
case KFENCE_ERROR_OOB: {
const bool left_of_object = address < meta->addr;
- pr_err("BUG: KFENCE: out-of-bounds %s in %pS\n\n", get_access_type(is_write),
- (void *)stack_entries[skipnr]);
+ pr_err("BUG: KFENCE: out-of-bounds %s in %pS\n\n", get_access_type(is_write), ip);
pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%zd):\n",
get_access_type(is_write), (void *)address,
left_of_object ? meta->addr - address : address - meta->addr,
@@ -211,25 +213,23 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r
break;
}
case KFENCE_ERROR_UAF:
- pr_err("BUG: KFENCE: use-after-free %s in %pS\n\n", get_access_type(is_write),
- (void *)stack_entries[skipnr]);
+ pr_err("BUG: KFENCE: use-after-free %s in %pS\n\n", get_access_type(is_write), ip);
pr_err("Use-after-free %s at 0x%p (in kfence-#%zd):\n",
get_access_type(is_write), (void *)address, object_index);
break;
case KFENCE_ERROR_CORRUPTION:
- pr_err("BUG: KFENCE: memory corruption in %pS\n\n", (void *)stack_entries[skipnr]);
+ pr_err("BUG: KFENCE: memory corruption in %pS\n\n", ip);
pr_err("Corrupted memory at 0x%p ", (void *)address);
print_diff_canary(address, 16, meta);
pr_cont(" (in kfence-#%zd):\n", object_index);
break;
case KFENCE_ERROR_INVALID:
- pr_err("BUG: KFENCE: invalid %s in %pS\n\n", get_access_type(is_write),
- (void *)stack_entries[skipnr]);
+ pr_err("BUG: KFENCE: invalid %s in %pS\n\n", get_access_type(is_write), ip);
pr_err("Invalid %s at 0x%p:\n", get_access_type(is_write),
(void *)address);
break;
case KFENCE_ERROR_INVALID_FREE:
- pr_err("BUG: KFENCE: invalid free in %pS\n\n", (void *)stack_entries[skipnr]);
+ pr_err("BUG: KFENCE: invalid free in %pS\n\n", ip);
pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address,
object_index);
break;
---
Christophe
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-03 10:31 UTC (permalink / raw)
To: Marco Elver
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <CANpmjNPGj4C2rr2FbSD+FC-GnWUvJrtdLyX5TYpJE_Um8CGu1Q@mail.gmail.com>
Le 02/03/2021 à 10:53, Marco Elver a écrit :
> On Tue, 2 Mar 2021 at 10:27, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit :
>>>> [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>> [ 14.998426]
>>>> [ 15.007061] Invalid read at 0x(ptrval):
>>>> [ 15.010906] finish_task_switch.isra.0+0x54/0x23c
>>>> [ 15.015633] kunit_try_run_case+0x5c/0xd0
>>>> [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 15.025099] kthread+0x15c/0x174
>>>> [ 15.028359] ret_from_kernel_thread+0x14/0x1c
>>>> [ 15.032747]
>>>> [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>> [ 15.045811] ==================================================================
>>>> [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>> [ 15.053324] Expected report_matches(&expect) to be true, but is false
>>>> [ 15.068359] not ok 21 - test_invalid_access
>>>
>>> The test expects the function name to be test_invalid_access, i. e.
>>> the first line should be "BUG: KFENCE: invalid read in
>>> test_invalid_access".
>>> The error reporting function unwinds the stack, skips a couple of
>>> "uninteresting" frames
>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43)
>>> and uses the first "interesting" one frame to print the report header
>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226).
>>>
>>> It's strange that test_invalid_access is missing altogether from the
>>> stack trace - is that expected?
>>> Can you try printing the whole stacktrace without skipping any frames
>>> to see if that function is there?
>>>
>>
>> Booting with 'no_hash_pointers" I get the following. Does it helps ?
>>
>> [ 16.837198] ==================================================================
>> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>> [ 16.848521]
>> [ 16.857158] Invalid read at 0xdf98800a:
>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
>> [ 16.865731] kunit_try_run_case+0x5c/0xd0
>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
>> [ 16.875199] kthread+0x15c/0x174
>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
>> [ 16.882847]
>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
>> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
>> [ 16.918153] DAR: df98800a DSISR: 20000000
>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
>> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
>> [ 16.947292] Call Trace:
>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
>
> The "(unreliable)" might be a clue that it's related to ppc32 stack
> unwinding. Any ppc expert know what this is about?
>
>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
>> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
>> [ 16.981896] Instruction dump:
>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
>> [ 17.000711] ==================================================================
>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>> [ 17.008223] Expected report_matches(&expect) to be true, but is false
>> [ 17.023243] not ok 21 - test_invalid_access
>
> On a fault in test_invalid_access, KFENCE prints the stack trace based
> on the information in pt_regs. So we do not think there's anything we
> can do to improve stack printing pe-se.
>
> What's confusing is that it's only this test, and none of the others.
> Given that, it might be code-gen related, which results in some subtle
> issue with stack unwinding. There are a few things to try, if you feel
> like it:
>
> -- Change the unwinder, if it's possible for ppc32.
>
> -- Add code to test_invalid_access(), to get the compiler to emit
> different code. E.g. add a bunch (unnecessary) function calls, or add
> barriers, etc.
>
> -- Play with compiler options. We already pass
> -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call
> optimizations that'd hide stack trace entries. But perhaps there's
> something ppc-specific we missed?
>
> Well, the good thing is that KFENCE detects the bad access just fine.
> Since, according to the test, everything works from KFENCE's side, I'd
> be happy to give my Ack:
>
> Acked-by: Marco Elver <elver@google.com>
>
Thanks.
For you information, I've got a pile of warnings from mm/kfence/report.o . Is that expected ?
CC mm/kfence/report.o
In file included from ./include/linux/printk.h:7,
from ./include/linux/kernel.h:16,
from mm/kfence/report.c:10:
mm/kfence/report.c: In function 'kfence_report_error':
./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
but argument 6 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
mm/kfence/report.c:207:3: note: in expansion of macro 'pr_err'
207 | pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%zd):\n",
| ^~~~~~
./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
but argument 4 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
mm/kfence/report.c:216:3: note: in expansion of macro 'pr_err'
216 | pr_err("Use-after-free %s at 0x%p (in kfence-#%zd):\n",
| ^~~~~~
./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
but argument 2 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/kern_levels.h:24:19: note: in expansion of macro 'KERN_SOH'
24 | #define KERN_CONT KERN_SOH "c"
| ^~~~~~~~
./include/linux/printk.h:385:9: note: in expansion of macro 'KERN_CONT'
385 | printk(KERN_CONT fmt, ##__VA_ARGS__)
| ^~~~~~~~~
mm/kfence/report.c:223:3: note: in expansion of macro 'pr_cont'
223 | pr_cont(" (in kfence-#%zd):\n", object_index);
| ^~~~~~~
./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
but argument 3 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
mm/kfence/report.c:233:3: note: in expansion of macro 'pr_err'
233 | pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address,
| ^~~~~~
Christophe
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-03 10:38 UTC (permalink / raw)
To: Marco Elver
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <CANpmjNPYEmLtQEu5G=zJLUzOBaGoqNKwLyipDCxvytdKDKb7mg@mail.gmail.com>
Le 02/03/2021 à 12:39, Marco Elver a écrit :
> On Tue, 2 Mar 2021 at 12:21, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
> [...]
>>>> Booting with 'no_hash_pointers" I get the following. Does it helps ?
>>>>
>>>> [ 16.837198] ==================================================================
>>>> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.848521]
>>>> [ 16.857158] Invalid read at 0xdf98800a:
>>>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.865731] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.875199] kthread+0x15c/0x174
>>>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.882847]
>>>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
>>>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
>>>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
>>>> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
>>>> [ 16.918153] DAR: df98800a DSISR: 20000000
>>>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
>>>> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
>>>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
>>>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.947292] Call Trace:
>>>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
>>>
>>> The "(unreliable)" might be a clue that it's related to ppc32 stack
>>> unwinding. Any ppc expert know what this is about?
>>>
>>>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
>>>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.981896] Instruction dump:
>>>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
>>>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
>>>> [ 17.000711] ==================================================================
>>>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>> [ 17.008223] Expected report_matches(&expect) to be true, but is false
>>>> [ 17.023243] not ok 21 - test_invalid_access
>>>
>>> On a fault in test_invalid_access, KFENCE prints the stack trace based
>>> on the information in pt_regs. So we do not think there's anything we
>>> can do to improve stack printing pe-se.
>>
>> stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does.
>
> We use stack_trace_save_regs() + stack_trace_print().
>
>> IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests,
>> there is some function call being done before the fault, for instance
>> test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the
>> call in the stack. However this is fragile.
>
> Interesting, this might explain it.
>
>> This works for function calls because in order to call a subfunction, a function has to set up a
>> stack frame in order to same the value in the Link Register, which contains the address of the
>> function's parent and that will be clobbered by the sub-function call.
>>
>> However, it cannot be done by exceptions, because exceptions can happen in a function that has no
>> stack frame (because that function has no need to call a subfunction and doesn't need to same
>> anything on the stack). If the exception handler was writting the caller's address in the stack
>> frame, it would in fact write it in the parent's frame, leading to a mess.
>>
>> But in fact the information is in pt_regs, it is in regs->nip so KFENCE should be able to use that
>> instead of the stack.
>
> Perhaps stack_trace_save_regs() needs fixing for ppc32? Although that
> seems to use arch_stack_walk().
>
>>> What's confusing is that it's only this test, and none of the others.
>>> Given that, it might be code-gen related, which results in some subtle
>>> issue with stack unwinding. There are a few things to try, if you feel
>>> like it:
>>>
>>> -- Change the unwinder, if it's possible for ppc32.
>>
>> I don't think it is possible.
>>
>>>
>>> -- Add code to test_invalid_access(), to get the compiler to emit
>>> different code. E.g. add a bunch (unnecessary) function calls, or add
>>> barriers, etc.
>>
>> The following does the trick
>>
>> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
>> index 4acf4251ee04..22550676cd1f 100644
>> --- a/mm/kfence/kfence_test.c
>> +++ b/mm/kfence/kfence_test.c
>> @@ -631,8 +631,11 @@ static void test_invalid_access(struct kunit *test)
>> .addr = &__kfence_pool[10],
>> .is_write = false,
>> };
>> + char *buf;
>>
>> + buf = test_alloc(test, 4, GFP_KERNEL, ALLOCATE_RIGHT);
>> READ_ONCE(__kfence_pool[10]);
>> + test_free(buf);
>> KUNIT_EXPECT_TRUE(test, report_matches(&expect));
>> }
>>
>>
>> But as I said above, this is fragile. If for some reason one day test_alloc() gets inlined, it may
>> not work anymore.
>
> Yeah, obviously that's hack, but interesting nevertheless.
>
> Based on what you say above, however, it seems that
> stack_trace_save_regs()/arch_stack_walk() don't exactly do what they
> should? Can they be fixed for ppc32?
Can we really consider they don't do what they should ?
I have the feeling that excepting entry[0] of the stack trace to match the instruction pointer is
not a valid expectation. That's probably correct on architectures that always have a stack frame for
any function, but for powerpc who can have frameless functions, we can't expect that I think.
I have proposed a change to KFENCE in another response to this mail thread, could it be the solution ?
Thanks
Christophe
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-03 10:39 UTC (permalink / raw)
To: Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <3abbe4c9-16ad-c168-a90f-087978ccd8f7@csgroup.eu>
On Wed, 3 Mar 2021 at 11:32, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 02/03/2021 à 10:53, Marco Elver a écrit :
> > On Tue, 2 Mar 2021 at 10:27, Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> >> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit :
> >>>> [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
> >>>> [ 14.998426]
> >>>> [ 15.007061] Invalid read at 0x(ptrval):
> >>>> [ 15.010906] finish_task_switch.isra.0+0x54/0x23c
> >>>> [ 15.015633] kunit_try_run_case+0x5c/0xd0
> >>>> [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30
> >>>> [ 15.025099] kthread+0x15c/0x174
> >>>> [ 15.028359] ret_from_kernel_thread+0x14/0x1c
> >>>> [ 15.032747]
> >>>> [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
> >>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
> >>>> [ 15.045811] ==================================================================
> >>>> [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
> >>>> [ 15.053324] Expected report_matches(&expect) to be true, but is false
> >>>> [ 15.068359] not ok 21 - test_invalid_access
> >>>
> >>> The test expects the function name to be test_invalid_access, i. e.
> >>> the first line should be "BUG: KFENCE: invalid read in
> >>> test_invalid_access".
> >>> The error reporting function unwinds the stack, skips a couple of
> >>> "uninteresting" frames
> >>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43)
> >>> and uses the first "interesting" one frame to print the report header
> >>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226).
> >>>
> >>> It's strange that test_invalid_access is missing altogether from the
> >>> stack trace - is that expected?
> >>> Can you try printing the whole stacktrace without skipping any frames
> >>> to see if that function is there?
> >>>
> >>
> >> Booting with 'no_hash_pointers" I get the following. Does it helps ?
> >>
> >> [ 16.837198] ==================================================================
> >> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
> >> [ 16.848521]
> >> [ 16.857158] Invalid read at 0xdf98800a:
> >> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
> >> [ 16.865731] kunit_try_run_case+0x5c/0xd0
> >> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
> >> [ 16.875199] kthread+0x15c/0x174
> >> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
> >> [ 16.882847]
> >> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
> >> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
> >> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
> >> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
> >> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
> >> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
> >> [ 16.918153] DAR: df98800a DSISR: 20000000
> >> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
> >> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
> >> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
> >> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
> >> [ 16.947292] Call Trace:
> >> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
> >
> > The "(unreliable)" might be a clue that it's related to ppc32 stack
> > unwinding. Any ppc expert know what this is about?
> >
> >> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
> >> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
> >> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
> >> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
> >> [ 16.981896] Instruction dump:
> >> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
> >> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
> >> [ 17.000711] ==================================================================
> >> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
> >> [ 17.008223] Expected report_matches(&expect) to be true, but is false
> >> [ 17.023243] not ok 21 - test_invalid_access
> >
> > On a fault in test_invalid_access, KFENCE prints the stack trace based
> > on the information in pt_regs. So we do not think there's anything we
> > can do to improve stack printing pe-se.
> >
> > What's confusing is that it's only this test, and none of the others.
> > Given that, it might be code-gen related, which results in some subtle
> > issue with stack unwinding. There are a few things to try, if you feel
> > like it:
> >
> > -- Change the unwinder, if it's possible for ppc32.
> >
> > -- Add code to test_invalid_access(), to get the compiler to emit
> > different code. E.g. add a bunch (unnecessary) function calls, or add
> > barriers, etc.
> >
> > -- Play with compiler options. We already pass
> > -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call
> > optimizations that'd hide stack trace entries. But perhaps there's
> > something ppc-specific we missed?
> >
> > Well, the good thing is that KFENCE detects the bad access just fine.
> > Since, according to the test, everything works from KFENCE's side, I'd
> > be happy to give my Ack:
> >
> > Acked-by: Marco Elver <elver@google.com>
> >
>
> Thanks.
>
> For you information, I've got a pile of warnings from mm/kfence/report.o . Is that expected ?
>
> CC mm/kfence/report.o
> In file included from ./include/linux/printk.h:7,
> from ./include/linux/kernel.h:16,
> from mm/kfence/report.c:10:
> mm/kfence/report.c: In function 'kfence_report_error':
> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
> but argument 6 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
> | ^~~~~~
> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
> | ^~~~~~~~
> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> | ^~~~~~~~
> mm/kfence/report.c:207:3: note: in expansion of macro 'pr_err'
> 207 | pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%zd):\n",
> | ^~~~~~
> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
> but argument 4 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
> | ^~~~~~
> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
> | ^~~~~~~~
> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> | ^~~~~~~~
> mm/kfence/report.c:216:3: note: in expansion of macro 'pr_err'
> 216 | pr_err("Use-after-free %s at 0x%p (in kfence-#%zd):\n",
> | ^~~~~~
> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
> but argument 2 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
> | ^~~~~~
> ./include/linux/kern_levels.h:24:19: note: in expansion of macro 'KERN_SOH'
> 24 | #define KERN_CONT KERN_SOH "c"
> | ^~~~~~~~
> ./include/linux/printk.h:385:9: note: in expansion of macro 'KERN_CONT'
> 385 | printk(KERN_CONT fmt, ##__VA_ARGS__)
> | ^~~~~~~~~
> mm/kfence/report.c:223:3: note: in expansion of macro 'pr_cont'
> 223 | pr_cont(" (in kfence-#%zd):\n", object_index);
> | ^~~~~~~
> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
> but argument 3 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
> | ^~~~~~
> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
> | ^~~~~~~~
> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> | ^~~~~~~~
> mm/kfence/report.c:233:3: note: in expansion of macro 'pr_err'
> 233 | pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address,
> | ^~~~~~
>
> Christophe
No this is not expected. Is 'signed size_t' != 'long int' on ppc32?
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-03 10:56 UTC (permalink / raw)
To: Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <ad61cb3a-2b4a-3754-5761-832a1dd0c34e@csgroup.eu>
On Wed, 3 Mar 2021 at 11:39, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 02/03/2021 à 12:39, Marco Elver a écrit :
> > On Tue, 2 Mar 2021 at 12:21, Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> > [...]
> >>>> Booting with 'no_hash_pointers" I get the following. Does it helps ?
> >>>>
> >>>> [ 16.837198] ==================================================================
> >>>> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
> >>>> [ 16.848521]
> >>>> [ 16.857158] Invalid read at 0xdf98800a:
> >>>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
> >>>> [ 16.865731] kunit_try_run_case+0x5c/0xd0
> >>>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
> >>>> [ 16.875199] kthread+0x15c/0x174
> >>>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
> >>>> [ 16.882847]
> >>>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
> >>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
> >>>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
> >>>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
> >>>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
> >>>> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
> >>>> [ 16.918153] DAR: df98800a DSISR: 20000000
> >>>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
> >>>> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
> >>>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
> >>>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
> >>>> [ 16.947292] Call Trace:
> >>>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
> >>>
> >>> The "(unreliable)" might be a clue that it's related to ppc32 stack
> >>> unwinding. Any ppc expert know what this is about?
> >>>
> >>>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
> >>>> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
> >>>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
> >>>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
> >>>> [ 16.981896] Instruction dump:
> >>>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
> >>>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
> >>>> [ 17.000711] ==================================================================
> >>>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
> >>>> [ 17.008223] Expected report_matches(&expect) to be true, but is false
> >>>> [ 17.023243] not ok 21 - test_invalid_access
> >>>
> >>> On a fault in test_invalid_access, KFENCE prints the stack trace based
> >>> on the information in pt_regs. So we do not think there's anything we
> >>> can do to improve stack printing pe-se.
> >>
> >> stack printing, probably not. Would be good anyway to mark the last level [unreliable] as the ppc does.
> >
> > We use stack_trace_save_regs() + stack_trace_print().
> >
> >> IIUC, on ppc the address in the stack frame of the caller is written by the caller. In most tests,
> >> there is some function call being done before the fault, for instance
> >> test_kmalloc_aligned_oob_read() does a call to kunit_do_assertion which populates the address of the
> >> call in the stack. However this is fragile.
> >
> > Interesting, this might explain it.
> >
> >> This works for function calls because in order to call a subfunction, a function has to set up a
> >> stack frame in order to same the value in the Link Register, which contains the address of the
> >> function's parent and that will be clobbered by the sub-function call.
> >>
> >> However, it cannot be done by exceptions, because exceptions can happen in a function that has no
> >> stack frame (because that function has no need to call a subfunction and doesn't need to same
> >> anything on the stack). If the exception handler was writting the caller's address in the stack
> >> frame, it would in fact write it in the parent's frame, leading to a mess.
> >>
> >> But in fact the information is in pt_regs, it is in regs->nip so KFENCE should be able to use that
> >> instead of the stack.
> >
> > Perhaps stack_trace_save_regs() needs fixing for ppc32? Although that
> > seems to use arch_stack_walk().
> >
> >>> What's confusing is that it's only this test, and none of the others.
> >>> Given that, it might be code-gen related, which results in some subtle
> >>> issue with stack unwinding. There are a few things to try, if you feel
> >>> like it:
> >>>
> >>> -- Change the unwinder, if it's possible for ppc32.
> >>
> >> I don't think it is possible.
> >>
> >>>
> >>> -- Add code to test_invalid_access(), to get the compiler to emit
> >>> different code. E.g. add a bunch (unnecessary) function calls, or add
> >>> barriers, etc.
> >>
> >> The following does the trick
> >>
> >> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> >> index 4acf4251ee04..22550676cd1f 100644
> >> --- a/mm/kfence/kfence_test.c
> >> +++ b/mm/kfence/kfence_test.c
> >> @@ -631,8 +631,11 @@ static void test_invalid_access(struct kunit *test)
> >> .addr = &__kfence_pool[10],
> >> .is_write = false,
> >> };
> >> + char *buf;
> >>
> >> + buf = test_alloc(test, 4, GFP_KERNEL, ALLOCATE_RIGHT);
> >> READ_ONCE(__kfence_pool[10]);
> >> + test_free(buf);
> >> KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> >> }
> >>
> >>
> >> But as I said above, this is fragile. If for some reason one day test_alloc() gets inlined, it may
> >> not work anymore.
> >
> > Yeah, obviously that's hack, but interesting nevertheless.
> >
> > Based on what you say above, however, it seems that
> > stack_trace_save_regs()/arch_stack_walk() don't exactly do what they
> > should? Can they be fixed for ppc32?
>
> Can we really consider they don't do what they should ?
>
> I have the feeling that excepting entry[0] of the stack trace to match the instruction pointer is
> not a valid expectation. That's probably correct on architectures that always have a stack frame for
> any function, but for powerpc who can have frameless functions, we can't expect that I think.
>
> I have proposed a change to KFENCE in another response to this mail thread, could it be the solution ?
You're going to have to change all users of stack_trace_print/save
across the kernel, because the assumption is that the current frame is
included.
It is just bad design if we add special code to all users of the
<linux/stacktrace.h> API just so we can print the current frame at the
top of the trace. Therefore, I'm afraid your proposed patch to KFENCE
is not acceptable.
Instead, we have to either extend the <linux/stacktrace.h> API, or
simply accept that all current users of the API expect the current
frame to be included. If you do not want to include the current frame,
that API even provides a way to skip it already (just pass +1 as
skipnr).
<linux/stacktrace.h> writes this about arch_stack_walk():
* task NULL Stack trace from task (can be current)
* current regs Stack trace starting on regs->stackpointer
This is a bit vague, and unfortunately seems outdated, but I'd assume
that when it says "Stack trace from task" would be the stack trace
including the current function (at IP) being executed.
Somewhat tangentially, I also note that e.g. show_regs(regs) (which
was printed along the KFENCE report above) didn't include the top
frame in the "Call Trace", so this assumption is definitely not
isolated to KFENCE.
Thanks,
-- Marco
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-03 10:56 UTC (permalink / raw)
To: Marco Elver
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <CANpmjNMKEObjf=WyfDQB5vPmR5RuyUMBJyfr6P2ykCd67wyMbA@mail.gmail.com>
Le 03/03/2021 à 11:39, Marco Elver a écrit :
> On Wed, 3 Mar 2021 at 11:32, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>>
>>
>> Le 02/03/2021 à 10:53, Marco Elver a écrit :
>>> On Tue, 2 Mar 2021 at 10:27, Christophe Leroy
>>> <christophe.leroy@csgroup.eu> wrote:
>>>> Le 02/03/2021 à 10:21, Alexander Potapenko a écrit :
>>>>>> [ 14.998426] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>>>> [ 14.998426]
>>>>>> [ 15.007061] Invalid read at 0x(ptrval):
>>>>>> [ 15.010906] finish_task_switch.isra.0+0x54/0x23c
>>>>>> [ 15.015633] kunit_try_run_case+0x5c/0xd0
>>>>>> [ 15.019682] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>>>> [ 15.025099] kthread+0x15c/0x174
>>>>>> [ 15.028359] ret_from_kernel_thread+0x14/0x1c
>>>>>> [ 15.032747]
>>>>>> [ 15.034251] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>>>> [ 15.045811] ==================================================================
>>>>>> [ 15.053324] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>>>> [ 15.053324] Expected report_matches(&expect) to be true, but is false
>>>>>> [ 15.068359] not ok 21 - test_invalid_access
>>>>>
>>>>> The test expects the function name to be test_invalid_access, i. e.
>>>>> the first line should be "BUG: KFENCE: invalid read in
>>>>> test_invalid_access".
>>>>> The error reporting function unwinds the stack, skips a couple of
>>>>> "uninteresting" frames
>>>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L43)
>>>>> and uses the first "interesting" one frame to print the report header
>>>>> (https://elixir.bootlin.com/linux/v5.12-rc1/source/mm/kfence/report.c#L226).
>>>>>
>>>>> It's strange that test_invalid_access is missing altogether from the
>>>>> stack trace - is that expected?
>>>>> Can you try printing the whole stacktrace without skipping any frames
>>>>> to see if that function is there?
>>>>>
>>>>
>>>> Booting with 'no_hash_pointers" I get the following. Does it helps ?
>>>>
>>>> [ 16.837198] ==================================================================
>>>> [ 16.848521] BUG: KFENCE: invalid read in finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.848521]
>>>> [ 16.857158] Invalid read at 0xdf98800a:
>>>> [ 16.861004] finish_task_switch.isra.0+0x54/0x23c
>>>> [ 16.865731] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.869780] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.875199] kthread+0x15c/0x174
>>>> [ 16.878460] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.882847]
>>>> [ 16.884351] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B
>>>> 5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty #4674
>>>> [ 16.895908] NIP: c016eb8c LR: c02f50dc CTR: c016eb38
>>>> [ 16.900963] REGS: e2449d90 TRAP: 0301 Tainted: G B
>>>> (5.12.0-rc1-s3k-dev-01534-g4f14ae75edf0-dirty)
>>>> [ 16.911386] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
>>>> [ 16.918153] DAR: df98800a DSISR: 20000000
>>>> [ 16.918153] GPR00: c02f50dc e2449e50 c1140d00 e100dd24 c084b13c 00000008 c084b32b c016eb38
>>>> [ 16.918153] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
>>>> [ 16.936695] NIP [c016eb8c] test_invalid_access+0x54/0x108
>>>> [ 16.942125] LR [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.947292] Call Trace:
>>>> [ 16.949746] [e2449e50] [c005a5ec] finish_task_switch.isra.0+0x54/0x23c (unreliable)
>>>
>>> The "(unreliable)" might be a clue that it's related to ppc32 stack
>>> unwinding. Any ppc expert know what this is about?
>>>
>>>> [ 16.957443] [e2449eb0] [c02f50dc] kunit_try_run_case+0x5c/0xd0
>>>> [ 16.963319] [e2449ed0] [c02f63ec] kunit_generic_run_threadfn_adapter+0x24/0x30
>>>> [ 16.970574] [e2449ef0] [c004e710] kthread+0x15c/0x174
>>>> [ 16.975670] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
>>>> [ 16.981896] Instruction dump:
>>>> [ 16.984879] 8129d608 38e7eb38 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
>>>> [ 16.992710] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
>>>> [ 17.000711] ==================================================================
>>>> [ 17.008223] # test_invalid_access: EXPECTATION FAILED at mm/kfence/kfence_test.c:636
>>>> [ 17.008223] Expected report_matches(&expect) to be true, but is false
>>>> [ 17.023243] not ok 21 - test_invalid_access
>>>
>>> On a fault in test_invalid_access, KFENCE prints the stack trace based
>>> on the information in pt_regs. So we do not think there's anything we
>>> can do to improve stack printing pe-se.
>>>
>>> What's confusing is that it's only this test, and none of the others.
>>> Given that, it might be code-gen related, which results in some subtle
>>> issue with stack unwinding. There are a few things to try, if you feel
>>> like it:
>>>
>>> -- Change the unwinder, if it's possible for ppc32.
>>>
>>> -- Add code to test_invalid_access(), to get the compiler to emit
>>> different code. E.g. add a bunch (unnecessary) function calls, or add
>>> barriers, etc.
>>>
>>> -- Play with compiler options. We already pass
>>> -fno-optimize-sibling-calls for kfence_test.o to avoid tail-call
>>> optimizations that'd hide stack trace entries. But perhaps there's
>>> something ppc-specific we missed?
>>>
>>> Well, the good thing is that KFENCE detects the bad access just fine.
>>> Since, according to the test, everything works from KFENCE's side, I'd
>>> be happy to give my Ack:
>>>
>>> Acked-by: Marco Elver <elver@google.com>
>>>
>>
>> Thanks.
>>
>> For you information, I've got a pile of warnings from mm/kfence/report.o . Is that expected ?
>>
>> CC mm/kfence/report.o
>> In file included from ./include/linux/printk.h:7,
>> from ./include/linux/kernel.h:16,
>> from mm/kfence/report.c:10:
>> mm/kfence/report.c: In function 'kfence_report_error':
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
>> but argument 6 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>> | ^~~~~~
>> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
>> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
>> | ^~~~~~~~
>> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
>> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> | ^~~~~~~~
>> mm/kfence/report.c:207:3: note: in expansion of macro 'pr_err'
>> 207 | pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%zd):\n",
>> | ^~~~~~
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
>> but argument 4 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>> | ^~~~~~
>> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
>> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
>> | ^~~~~~~~
>> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
>> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> | ^~~~~~~~
>> mm/kfence/report.c:216:3: note: in expansion of macro 'pr_err'
>> 216 | pr_err("Use-after-free %s at 0x%p (in kfence-#%zd):\n",
>> | ^~~~~~
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
>> but argument 2 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>> | ^~~~~~
>> ./include/linux/kern_levels.h:24:19: note: in expansion of macro 'KERN_SOH'
>> 24 | #define KERN_CONT KERN_SOH "c"
>> | ^~~~~~~~
>> ./include/linux/printk.h:385:9: note: in expansion of macro 'KERN_CONT'
>> 385 | printk(KERN_CONT fmt, ##__VA_ARGS__)
>> | ^~~~~~~~~
>> mm/kfence/report.c:223:3: note: in expansion of macro 'pr_cont'
>> 223 | pr_cont(" (in kfence-#%zd):\n", object_index);
>> | ^~~~~~~
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
>> but argument 3 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>> | ^~~~~~
>> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
>> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
>> | ^~~~~~~~
>> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
>> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> | ^~~~~~~~
>> mm/kfence/report.c:233:3: note: in expansion of macro 'pr_err'
>> 233 | pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address,
>> | ^~~~~~
>>
>> Christophe
>
> No this is not expected. Is 'signed size_t' != 'long int' on ppc32?
>
No, it is an 'int' not a 'long int', see arch/powerpc/include/uapi/asm/posix_types.h
#ifdef __powerpc64__
typedef unsigned long __kernel_old_dev_t;
#define __kernel_old_dev_t __kernel_old_dev_t
#else
typedef unsigned int __kernel_size_t;
typedef int __kernel_ssize_t;
typedef long __kernel_ptrdiff_t;
#define __kernel_size_t __kernel_size_t
What is probably specific to powerpc is that ptrdiff_t is not same as ssize_t unlike in
include/uapi/asm-generic/posix_types.h :
/*
* Most 32 bit architectures use "unsigned int" size_t,
* and all 64 bit architectures use "unsigned long" size_t.
*/
#ifndef __kernel_size_t
#if __BITS_PER_LONG != 64
typedef unsigned int __kernel_size_t;
typedef int __kernel_ssize_t;
typedef int __kernel_ptrdiff_t;
#else
typedef __kernel_ulong_t __kernel_size_t;
typedef __kernel_long_t __kernel_ssize_t;
typedef __kernel_long_t __kernel_ptrdiff_t;
#endif
#endif
I have no warning on ppc64.
Christophe
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Andreas Schwab @ 2021-03-03 10:46 UTC (permalink / raw)
To: Marco Elver
Cc: LKML, kasan-dev, Paul Mackerras, Alexander Potapenko,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <CANpmjNMKEObjf=WyfDQB5vPmR5RuyUMBJyfr6P2ykCd67wyMbA__49537.1361424745$1614767987$gmane$org@mail.gmail.com>
On Mär 03 2021, Marco Elver wrote:
> On Wed, 3 Mar 2021 at 11:32, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>> ./include/linux/kern_levels.h:5:18: warning: format '%zd' expects argument of type 'signed size_t',
>> but argument 3 has type 'ptrdiff_t' {aka 'const long int'} [-Wformat=]
>> 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
>> | ^~~~~~
>> ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
>> 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
>> | ^~~~~~~~
>> ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
>> 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> | ^~~~~~~~
>> mm/kfence/report.c:233:3: note: in expansion of macro 'pr_err'
>> 233 | pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address,
>> | ^~~~~~
>>
>> Christophe
>
> No this is not expected. Is 'signed size_t' != 'long int' on ppc32?
If you want to format a ptrdiff_t you should use %td.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* [RESEND 1/1] powerpc: asm: hvconsole: Move 'hvc_vio_init_early's prototype to shared location
From: Lee Jones @ 2021-03-03 12:46 UTC (permalink / raw)
To: lee.jones; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
Fixes the following W=1 kernel build warning(s):
drivers/tty/hvc/hvc_vio.c:385:13: warning: no previous prototype for ‘hvc_vio_init_early’ [-Wmissing-prototypes]
385 | void __init hvc_vio_init_early(void)
| ^~~~~~~~~~~~~~~~~~
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/hvconsole.h | 3 +++
arch/powerpc/platforms/pseries/pseries.h | 3 ---
arch/powerpc/platforms/pseries/setup.c | 1 +
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/hvconsole.h b/arch/powerpc/include/asm/hvconsole.h
index 999ed5ac90531..ccb2034506f0f 100644
--- a/arch/powerpc/include/asm/hvconsole.h
+++ b/arch/powerpc/include/asm/hvconsole.h
@@ -24,5 +24,8 @@
extern int hvc_get_chars(uint32_t vtermno, char *buf, int count);
extern int hvc_put_chars(uint32_t vtermno, const char *buf, int count);
+/* Provided by HVC VIO */
+void hvc_vio_init_early(void);
+
#endif /* __KERNEL__ */
#endif /* _PPC64_HVCONSOLE_H */
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 4fe48c04c6c20..a13438fca10a8 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -43,9 +43,6 @@ extern void pSeries_final_fixup(void);
/* Poweron flag used for enabling auto ups restart */
extern unsigned long rtas_poweron_auto;
-/* Provided by HVC VIO */
-extern void hvc_vio_init_early(void);
-
/* Dynamic logical Partitioning/Mobility */
extern void dlpar_free_cc_nodes(struct device_node *);
extern void dlpar_free_cc_property(struct property *);
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 46e1540abc229..145e3f4c999af 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -71,6 +71,7 @@
#include <asm/swiotlb.h>
#include <asm/svm.h>
#include <asm/dtl.h>
+#include <asm/hvconsole.h>
#include "pseries.h"
#include "../../../../drivers/pci/pci.h"
--
2.27.0
^ permalink raw reply related
* lkml delivery: was: Re: [PATCH next v4 00/15] printk: remove logbuf_lock
From: Petr Mladek @ 2021-03-03 13:18 UTC (permalink / raw)
To: John Ogness, Konstantin Ryabitsev
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson, linux-mtd,
Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Daniel Thompson, Madhavan Srinivasan,
Stephen Hemminger, Richard Weinberger, Anton Vorontsov,
Jordan Niethe, Anton Ivanov, Wei Li, Haiyang Zhang, Ravi Bangoria,
Kees Cook, Alistair Popple, Jeff Dike, Colin Cross, linux-um,
Wei Liu, Steven Rostedt, Davidlohr Bueso, Nicholas Piggin,
Oleg Nesterov, Thomas Gleixner, Andy Shevchenko, Michael Kelley,
Christophe Leroy, Sumit Garg, Tony Luck, Pavel Tatashin,
linux-kernel, Sergey Senozhatsky, Jason Wessel, kgdb-bugreport,
Paul Mackerras, linuxppc-dev, Mike Rapoport
In-Reply-To: <20210303101528.29901-1-john.ogness@linutronix.de>
Hi John,
On Wed 2021-03-03 11:15:13, John Ogness wrote:
> Hello,
>
> Here is v4 of a series to remove @logbuf_lock, exposing the
> ringbuffer locklessly to both readers and writers. v3 is
> here [0].
Have you got some reply from lkml that it has not delivered there,
please?
I am not able to get the patchset using b4 tool:
$> b4 am -o test 20210303101528.29901-1-john.ogness@linutronix.de
Looking up https://lore.kernel.org/r/20210303101528.29901-1-john.ogness%40linutronix.de
Grabbing thread from lore.kernel.org/linux-hyperv
Analyzing 2 messages in the thread
---
Thread incomplete, attempting to backfill
Grabbing thread from lore.kernel.org/lkml
Server returned an error: 404
Grabbing thread from lore.kernel.org/linux-mtd
Server returned an error: 404
Grabbing thread from lore.kernel.org/linuxppc-dev
Loaded 2 messages from https://lore.kernel.org/linuxppc-dev/
---
Writing test/v4_20210303_john_ogness_printk_remove_logbuf_lock.mbx
ERROR: missing [1/15]!
ERROR: missing [2/15]!
ERROR: missing [3/15]!
ERROR: missing [4/15]!
ERROR: missing [5/15]!
ERROR: missing [6/15]!
ERROR: missing [7/15]!
ERROR: missing [8/15]!
ERROR: missing [9/15]!
ERROR: missing [10/15]!
[PATCH next v4 11/15] printk: kmsg_dumper: remove @active field
✓ [PATCH next v4 12/15] printk: introduce a kmsg_dump iterator
ERROR: missing [13/15]!
[PATCH next v4 14/15] printk: kmsg_dump: remove _nolock() variants
ERROR: missing [15/15]!
---
Total patches: 3
---
WARNING: Thread incomplete!
Cover: test/v4_20210303_john_ogness_printk_remove_logbuf_lock.cover
Link: https://lore.kernel.org/r/20210303101528.29901-1-john.ogness@linutronix.de
Base: not found
git am test/v4_20210303_john_ogness_printk_remove_logbuf_lock.mbx
and I do not see it at lore. It has only found copies in linux-hyperv
and linux-ppcdev mailing lists,
see https://lore.kernel.org/lkml/20210303101528.29901-2-john.ogness@linutronix.de/
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] ASoC: hdmi-codec: fix platform_no_drv_owner.cocci warnings
From: Fabio Estevam @ 2021-03-03 13:37 UTC (permalink / raw)
To: Yang Li
Cc: Linux-ALSA, linuxppc-dev, linux-kernel, Timur Tabi, Xiubo Li,
Shawn Guo, Sascha Hauer, Takashi Iwai, Liam Girdwood,
Jaroslav Kysela, Nicolin Chen, Mark Brown, NXP Linux Team,
Sascha Hauer, Shengjiu Wang,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1614761651-86898-1-git-send-email-yang.lee@linux.alibaba.com>
Hi Yang,
On Wed, Mar 3, 2021 at 5:54 AM Yang Li <yang.lee@linux.alibaba.com> wrote:
>
> ./sound/soc/fsl/imx-hdmi.c:226:3-8: No need to set .owner here. The core
> will do it.
>
> Remove .owner field if calls are used which set it automatically
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
The patch looks good, but please send a v2 with the correct Subject.
It should mention imx-hdmi instead of hdmi-codec.
Thanks
^ permalink raw reply
* Re: [PATCH next v4 12/15] printk: introduce a kmsg_dump iterator
From: Petr Mladek @ 2021-03-03 13:48 UTC (permalink / raw)
To: John Ogness
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson,
Paul Mackerras, Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Wei Liu, Madhavan Srinivasan,
Stephen Hemminger, Anton Vorontsov, Jason Wessel, Anton Ivanov,
Wei Li, Haiyang Zhang, Ravi Bangoria, Kees Cook, Alistair Popple,
Jeff Dike, Colin Cross, linux-um, Daniel Thompson, Steven Rostedt,
Davidlohr Bueso, Nicholas Piggin, Oleg Nesterov, Thomas Gleixner,
Andy Shevchenko, Jordan Niethe, Michael Kelley, Christophe Leroy,
Tony Luck, Pavel Tatashin, linux-kernel, Sergey Senozhatsky,
Richard Weinberger, kgdb-bugreport, linux-mtd, linuxppc-dev,
Mike Rapoport
In-Reply-To: <20210303101528.29901-13-john.ogness@linutronix.de>
On Wed 2021-03-03 11:15:25, John Ogness wrote:
> Rather than storing the iterator information in the registered
> kmsg_dumper structure, create a separate iterator structure. The
> kmsg_dump_iter structure can reside on the stack of the caller, thus
> allowing lockless use of the kmsg_dump functions.
>
> Update code that accesses the kernel logs using the kmsg_dumper
> structure to use the new kmsg_dump_iter structure. For kmsg_dumpers,
> this also means adding a call to kmsg_dump_rewind() to initialize
> the iterator.
>
> All this is in preparation for removal of @logbuf_lock.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Kees Cook <keescook@chromium.org> # pstore
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply
* [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Christophe Leroy @ 2021-03-03 14:09 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, elver
Cc: linuxppc-dev, linux-kernel, kasan-dev
It seems like all other sane architectures, namely x86 and arm64
at least, include the running function as top entry when saving
stack trace.
Functionnalities like KFENCE expect it.
Do the same on powerpc, it allows KFENCE to properly identify the faulting
function as depicted below. Before the patch KFENCE was identifying
finish_task_switch.isra as the faulting function.
[ 14.937370] ==================================================================
[ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
[ 14.948692]
[ 14.956814] Invalid read at 0xdf98800a:
[ 14.960664] test_invalid_access+0x54/0x108
[ 14.964876] finish_task_switch.isra.0+0x54/0x23c
[ 14.969606] kunit_try_run_case+0x5c/0xd0
[ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 14.979079] kthread+0x15c/0x174
[ 14.982342] ret_from_kernel_thread+0x14/0x1c
[ 14.986731]
[ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
[ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
[ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
[ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 15.022043] DAR: df98800a DSISR: 20000000
[ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
[ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
[ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
[ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.051181] Call Trace:
[ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
[ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
[ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
[ 15.085798] Instruction dump:
[ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
[ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
[ 15.104612] ==================================================================
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/stacktrace.c | 42 +++++++++++++++++++++-----------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index b6440657ef92..67c2b8488035 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -22,16 +22,32 @@
#include <asm/kprobes.h>
#include <asm/paca.h>
+#include <asm/switch_to.h>
/*
* Save stack-backtrace addresses into a stack_trace buffer.
*/
+static void save_entry(struct stack_trace *trace, unsigned long ip, int savesched)
+{
+ if (savesched || !in_sched_functions(ip)) {
+ if (!trace->skip)
+ trace->entries[trace->nr_entries++] = ip;
+ else
+ trace->skip--;
+ }
+}
+
static void save_context_stack(struct stack_trace *trace, unsigned long sp,
- struct task_struct *tsk, int savesched)
+ unsigned long ip, struct task_struct *tsk, int savesched)
{
+ save_entry(trace, ip, savesched);
+
+ if (trace->nr_entries >= trace->max_entries)
+ return;
+
for (;;) {
unsigned long *stack = (unsigned long *) sp;
- unsigned long newsp, ip;
+ unsigned long newsp;
if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
return;
@@ -39,12 +55,7 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
- if (savesched || !in_sched_functions(ip)) {
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
- }
+ save_entry(trace, ip, savesched);
if (trace->nr_entries >= trace->max_entries)
return;
@@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
sp = current_stack_frame();
- save_context_stack(trace, sp, current, 1);
+ save_context_stack(trace, sp, (unsigned long)save_stack_trace, current, 1);
}
EXPORT_SYMBOL_GPL(save_stack_trace);
void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
{
- unsigned long sp;
+ unsigned long sp, ip;
if (!try_get_task_stack(tsk))
return;
- if (tsk == current)
+ if (tsk == current) {
+ ip = (unsigned long)save_stack_trace_tsk;
sp = current_stack_frame();
- else
+ } else {
+ ip = (unsigned long)_switch;
sp = tsk->thread.ksp;
+ }
- save_context_stack(trace, sp, tsk, 0);
+ save_context_stack(trace, sp, ip, tsk, 0);
put_task_stack(tsk);
}
@@ -84,7 +98,7 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
void
save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
{
- save_context_stack(trace, regs->gpr[1], current, 0);
+ save_context_stack(trace, regs->gpr[1], regs->nip, current, 0);
}
EXPORT_SYMBOL_GPL(save_stack_trace_regs);
--
2.25.0
^ permalink raw reply related
* Re: lkml delivery: was: Re: [PATCH next v4 00/15] printk: remove logbuf_lock
From: Steven Rostedt @ 2021-03-03 14:34 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson, Colin Cross,
linux-mtd, Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Daniel Thompson, Madhavan Srinivasan,
Stephen Hemminger, Richard Weinberger, Anton Vorontsov,
Jordan Niethe, Anton Ivanov, Wei Li, Haiyang Zhang, Ravi Bangoria,
Kees Cook, John Ogness, Alistair Popple, Jeff Dike, Jason Wessel,
linux-um, Wei Liu, Nicholas Piggin, Davidlohr Bueso,
Oleg Nesterov, Thomas Gleixner, Andy Shevchenko,
Konstantin Ryabitsev, Michael Kelley, Christophe Leroy,
Sumit Garg, Tony Luck, Pavel Tatashin, linux-kernel,
Sergey Senozhatsky, kgdb-bugreport, Paul Mackerras, linuxppc-dev,
Mike Rapoport
In-Reply-To: <YD+MpccJp4gX6bOP@alley>
On Wed, 3 Mar 2021 14:18:29 +0100
Petr Mladek <pmladek@suse.com> wrote:
> Hi John,
>
> On Wed 2021-03-03 11:15:13, John Ogness wrote:
> > Hello,
> >
> > Here is v4 of a series to remove @logbuf_lock, exposing the
> > ringbuffer locklessly to both readers and writers. v3 is
> > here [0].
>
> Have you got some reply from lkml that it has not delivered there,
> please?
vger has been having some issues as of late, and emails have been coming in
slowly. I just received emails I sent more than 24 hours a head of time.
Those in charge are trying to work things out.
-- Steve
^ permalink raw reply
* [PATCH v2 00/10] Rid W=1 warnings in Crypto
From: Lee Jones @ 2021-03-03 14:34 UTC (permalink / raw)
To: lee.jones
Cc: Alexandre Belloni, Aymen Sghaier, Kent Yoder, Ayush Sawal,
Joakim Bech, Nicolas Ferre, Paul Mackerras, Andreas Westin,
Breno Leitão, Atul Gupta, Niklas Hernaeus, M R Gowda,
Herbert Xu, Horia Geantă, Rohit Maheshwari, Nayna Jain,
Manoj Malviya, Ludovic Desroches, Jonas Linde, Rob Rice, Zaibo Xu,
Harsh Jain, Declan Murphy, Tudor Ambarus, Vinay Kumar Yadav,
Shujuan Chen, Henrique Cerri, Daniele Alessandrelli,
linux-arm-kernel, Jonathan Cameron, linux-kernel, Berne Hebark,
linux-crypto, Jitendra Lulla, Paulo Flabiano Smorigo,
linuxppc-dev, David S. Miller
This set is part of a larger effort attempting to clean-up W=1
kernel builds, which are currently overwhelmingly riddled with
niggly little warnings.
This is set 1 of 2 sets required to fully clean Crypto.
No functional changes since v1.
Lee Jones (10):
crypto: hisilicon: sec_drv: Supply missing description for
'sec_queue_empty()'s 'queue' param
crypto: bcm: Fix a whole host of kernel-doc misdemeanours
crypto: chelsio: chcr_core: Fix some kernel-doc issues
crypto: ux500: hash: hash_core: Fix worthy kernel-doc headers and
remove others
crypto: keembay: ocs-hcu: Fix incorrectly named functions/structs
crypto: atmel-ecc: Struct headers need to start with keyword 'struct'
crypto: caam: caampkc: Provide the name of the function and provide
missing descriptions
crypto: vmx: Source headers are not good kernel-doc candidates
crypto: nx: nx-aes-cbc: Repair some kernel-doc problems
crypto: cavium: nitrox_isr: Demote non-compliant kernel-doc headers
drivers/crypto/atmel-ecc.c | 2 +-
drivers/crypto/bcm/cipher.c | 7 ++--
drivers/crypto/bcm/spu.c | 16 ++++-----
drivers/crypto/bcm/spu2.c | 43 +++++++++++++----------
drivers/crypto/bcm/util.c | 4 +--
drivers/crypto/caam/caamalg_qi2.c | 2 ++
drivers/crypto/caam/caampkc.c | 3 +-
drivers/crypto/cavium/nitrox/nitrox_isr.c | 4 +--
drivers/crypto/chelsio/chcr_algo.c | 8 ++---
drivers/crypto/chelsio/chcr_core.c | 2 +-
drivers/crypto/hisilicon/sec/sec_drv.c | 1 +
drivers/crypto/keembay/ocs-hcu.c | 6 ++--
drivers/crypto/nx/nx-aes-cbc.c | 2 +-
drivers/crypto/nx/nx.c | 5 +--
drivers/crypto/nx/nx_debugfs.c | 2 +-
drivers/crypto/ux500/cryp/cryp.c | 5 +--
drivers/crypto/ux500/cryp/cryp_core.c | 5 +--
drivers/crypto/ux500/cryp/cryp_irq.c | 2 +-
drivers/crypto/ux500/hash/hash_core.c | 15 +++-----
drivers/crypto/vmx/vmx.c | 2 +-
20 files changed, 71 insertions(+), 65 deletions(-)
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: Atul Gupta <atul.gupta@chelsio.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Ayush Sawal <ayush.sawal@chelsio.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Berne Hebark <berne.herbark@stericsson.com>
Cc: "Breno Leitão" <leitao@debian.org>
Cc: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Declan Murphy <declan.murphy@intel.com>
Cc: Harsh Jain <harsh@chelsio.com>
Cc: Henrique Cerri <mhcerri@br.ibm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Horia Geantă" <horia.geanta@nxp.com>
Cc: Jitendra Lulla <jlulla@chelsio.com>
Cc: Joakim Bech <joakim.xx.bech@stericsson.com>
Cc: Jonas Linde <jonas.linde@stericsson.com>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Kent Yoder <yoder1@us.ibm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Manoj Malviya <manojmalviya@chelsio.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: M R Gowda <yeshaswi@chelsio.com>
Cc: Nayna Jain <nayna@linux.ibm.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Niklas Hernaeus <niklas.hernaeus@stericsson.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Paulo Flabiano Smorigo <pfsmorigo@gmail.com>
Cc: Rob Rice <rob.rice@broadcom.com>
Cc: Rohit Maheshwari <rohitm@chelsio.com>
Cc: Shujuan Chen <shujuan.chen@stericsson.com>
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
Cc: Zaibo Xu <xuzaibo@huawei.com>
--
2.27.0
^ permalink raw reply
* [PATCH 08/10] crypto: vmx: Source headers are not good kernel-doc candidates
From: Lee Jones @ 2021-03-03 14:34 UTC (permalink / raw)
To: lee.jones
Cc: Herbert Xu, Nayna Jain, linux-kernel, Henrique Cerri,
Paulo Flabiano Smorigo, linux-crypto, Breno Leitão,
Paul Mackerras, linuxppc-dev, David S. Miller
In-Reply-To: <20210303143449.3170813-1-lee.jones@linaro.org>
Fixes the following W=1 kernel build warning(s):
drivers/crypto/vmx/vmx.c:23: warning: expecting prototype for Routines supporting VMX instructions on the Power 8(). Prototype was for p8_init() instead
Cc: "Breno Leitão" <leitao@debian.org>
Cc: Nayna Jain <nayna@linux.ibm.com>
Cc: Paulo Flabiano Smorigo <pfsmorigo@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Henrique Cerri <mhcerri@br.ibm.com>
Cc: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/crypto/vmx/vmx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/vmx/vmx.c b/drivers/crypto/vmx/vmx.c
index a40d08e75fc0b..7eb713cc87c8c 100644
--- a/drivers/crypto/vmx/vmx.c
+++ b/drivers/crypto/vmx/vmx.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/**
+/*
* Routines supporting VMX instructions on the Power 8
*
* Copyright (C) 2015 International Business Machines Inc.
--
2.27.0
^ permalink raw reply related
* [PATCH 09/10] crypto: nx: nx-aes-cbc: Repair some kernel-doc problems
From: Lee Jones @ 2021-03-03 14:34 UTC (permalink / raw)
To: lee.jones
Cc: Herbert Xu, Kent Yoder, Nayna Jain, linux-kernel,
Paulo Flabiano Smorigo, linux-crypto, Breno Leitão,
Paul Mackerras, linuxppc-dev, David S. Miller
In-Reply-To: <20210303143449.3170813-1-lee.jones@linaro.org>
Fixes the following W=1 kernel build warning(s):
drivers/crypto/nx/nx-aes-cbc.c:24: warning: Function parameter or member 'tfm' not described in 'cbc_aes_nx_set_key'
drivers/crypto/nx/nx-aes-cbc.c:24: warning: Function parameter or member 'in_key' not described in 'cbc_aes_nx_set_key'
drivers/crypto/nx/nx-aes-cbc.c:24: warning: Function parameter or member 'key_len' not described in 'cbc_aes_nx_set_key'
drivers/crypto/nx/nx-aes-cbc.c:24: warning: expecting prototype for Nest Accelerators driver(). Prototype was for cbc_aes_nx_set_key() instead
drivers/crypto/nx/nx_debugfs.c:34: warning: Function parameter or member 'drv' not described in 'nx_debugfs_init'
drivers/crypto/nx/nx_debugfs.c:34: warning: expecting prototype for Nest Accelerators driver(). Prototype was for nx_debugfs_init() instead
drivers/crypto/nx/nx.c:31: warning: Incorrect use of kernel-doc format: * nx_hcall_sync - make an H_COP_OP hcall for the passed in op structure
drivers/crypto/nx/nx.c:43: warning: Function parameter or member 'nx_ctx' not described in 'nx_hcall_sync'
drivers/crypto/nx/nx.c:43: warning: Function parameter or member 'op' not described in 'nx_hcall_sync'
drivers/crypto/nx/nx.c:43: warning: Function parameter or member 'may_sleep' not described in 'nx_hcall_sync'
drivers/crypto/nx/nx.c:43: warning: expecting prototype for Nest Accelerators driver(). Prototype was for nx_hcall_sync() instead
drivers/crypto/nx/nx.c:209: warning: Function parameter or member 'nbytes' not described in 'trim_sg_list'
Cc: "Breno Leitão" <leitao@debian.org>
Cc: Nayna Jain <nayna@linux.ibm.com>
Cc: Paulo Flabiano Smorigo <pfsmorigo@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Kent Yoder <yoder1@us.ibm.com>
Cc: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/crypto/nx/nx-aes-cbc.c | 2 +-
drivers/crypto/nx/nx.c | 5 +++--
drivers/crypto/nx/nx_debugfs.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/nx/nx-aes-cbc.c b/drivers/crypto/nx/nx-aes-cbc.c
index 92e921eceed75..d6314ea9ae896 100644
--- a/drivers/crypto/nx/nx-aes-cbc.c
+++ b/drivers/crypto/nx/nx-aes-cbc.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/**
+/*
* AES CBC routines supporting the Power 7+ Nest Accelerators driver
*
* Copyright (C) 2011-2012 International Business Machines Inc.
diff --git a/drivers/crypto/nx/nx.c b/drivers/crypto/nx/nx.c
index 1d0e8a1ba1605..010e87d9da36b 100644
--- a/drivers/crypto/nx/nx.c
+++ b/drivers/crypto/nx/nx.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/**
+/*
* Routines supporting the Power 7+ Nest Accelerators driver
*
* Copyright (C) 2011-2012 International Business Machines Inc.
@@ -200,7 +200,8 @@ struct nx_sg *nx_walk_and_build(struct nx_sg *nx_dst,
* @sg: sg list head
* @end: sg lisg end
* @delta: is the amount we need to crop in order to bound the list.
- *
+ * @nbytes: length of data in the scatterlists or data length - whichever
+ * is greater.
*/
static long int trim_sg_list(struct nx_sg *sg,
struct nx_sg *end,
diff --git a/drivers/crypto/nx/nx_debugfs.c b/drivers/crypto/nx/nx_debugfs.c
index 1975bcbee9974..ee7cd88bb10a7 100644
--- a/drivers/crypto/nx/nx_debugfs.c
+++ b/drivers/crypto/nx/nx_debugfs.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/**
+/*
* debugfs routines supporting the Power 7+ Nest Accelerators driver
*
* Copyright (C) 2011-2012 International Business Machines Inc.
--
2.27.0
^ permalink raw reply related
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-03 14:38 UTC (permalink / raw)
To: Christophe Leroy; +Cc: LKML, kasan-dev, Paul Mackerras, linuxppc-dev
In-Reply-To: <e2e8728c4c4553bbac75a64b148e402183699c0c.1614780567.git.christophe.leroy@csgroup.eu>
On Wed, 3 Mar 2021 at 15:09, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> It seems like all other sane architectures, namely x86 and arm64
> at least, include the running function as top entry when saving
> stack trace.
>
> Functionnalities like KFENCE expect it.
>
> Do the same on powerpc, it allows KFENCE to properly identify the faulting
> function as depicted below. Before the patch KFENCE was identifying
> finish_task_switch.isra as the faulting function.
>
> [ 14.937370] ==================================================================
> [ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
> [ 14.948692]
> [ 14.956814] Invalid read at 0xdf98800a:
> [ 14.960664] test_invalid_access+0x54/0x108
> [ 14.964876] finish_task_switch.isra.0+0x54/0x23c
> [ 14.969606] kunit_try_run_case+0x5c/0xd0
> [ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
> [ 14.979079] kthread+0x15c/0x174
> [ 14.982342] ret_from_kernel_thread+0x14/0x1c
> [ 14.986731]
> [ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
> [ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
> [ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
> [ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
> [ 15.022043] DAR: df98800a DSISR: 20000000
> [ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
> [ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
> [ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
> [ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
> [ 15.051181] Call Trace:
> [ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
> [ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
> [ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
> [ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
> [ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
> [ 15.085798] Instruction dump:
> [ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
> [ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
> [ 15.104612] ==================================================================
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Marco Elver <elver@google.com>
Thank you, I think this looks like the right solution. Just a question below:
> ---
> arch/powerpc/kernel/stacktrace.c | 42 +++++++++++++++++++++-----------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index b6440657ef92..67c2b8488035 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -22,16 +22,32 @@
> #include <asm/kprobes.h>
>
> #include <asm/paca.h>
> +#include <asm/switch_to.h>
>
> /*
> * Save stack-backtrace addresses into a stack_trace buffer.
> */
> +static void save_entry(struct stack_trace *trace, unsigned long ip, int savesched)
> +{
> + if (savesched || !in_sched_functions(ip)) {
> + if (!trace->skip)
> + trace->entries[trace->nr_entries++] = ip;
> + else
> + trace->skip--;
> + }
> +}
> +
> static void save_context_stack(struct stack_trace *trace, unsigned long sp,
> - struct task_struct *tsk, int savesched)
> + unsigned long ip, struct task_struct *tsk, int savesched)
> {
> + save_entry(trace, ip, savesched);
> +
> + if (trace->nr_entries >= trace->max_entries)
> + return;
> +
> for (;;) {
> unsigned long *stack = (unsigned long *) sp;
> - unsigned long newsp, ip;
> + unsigned long newsp;
>
> if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
> return;
> @@ -39,12 +55,7 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
> newsp = stack[0];
> ip = stack[STACK_FRAME_LR_SAVE];
>
> - if (savesched || !in_sched_functions(ip)) {
> - if (!trace->skip)
> - trace->entries[trace->nr_entries++] = ip;
> - else
> - trace->skip--;
> - }
> + save_entry(trace, ip, savesched);
>
> if (trace->nr_entries >= trace->max_entries)
> return;
> @@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
>
> sp = current_stack_frame();
>
> - save_context_stack(trace, sp, current, 1);
> + save_context_stack(trace, sp, (unsigned long)save_stack_trace, current, 1);
This causes ip == save_stack_trace and also below for
save_stack_trace_tsk. Does this mean save_stack_trace() is included in
the trace? Looking at kernel/stacktrace.c, I think the library wants
to exclude itself from the trace, as it does '.skip = skipnr + 1' (and
'.skip = skipnr + (current == tsk)' for the _tsk variant).
If the arch-helper here is included, should this use _RET_IP_ instead?
Thanks,
-- Marco
> }
> EXPORT_SYMBOL_GPL(save_stack_trace);
>
> void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> {
> - unsigned long sp;
> + unsigned long sp, ip;
>
> if (!try_get_task_stack(tsk))
> return;
>
> - if (tsk == current)
> + if (tsk == current) {
> + ip = (unsigned long)save_stack_trace_tsk;
> sp = current_stack_frame();
> - else
> + } else {
> + ip = (unsigned long)_switch;
> sp = tsk->thread.ksp;
> + }
>
> - save_context_stack(trace, sp, tsk, 0);
> + save_context_stack(trace, sp, ip, tsk, 0);
>
> put_task_stack(tsk);
> }
> @@ -84,7 +98,7 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
> void
> save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
> {
> - save_context_stack(trace, regs->gpr[1], current, 0);
> + save_context_stack(trace, regs->gpr[1], regs->nip, current, 0);
> }
> EXPORT_SYMBOL_GPL(save_stack_trace_regs);
>
> --
> 2.25.0
>
^ permalink raw reply
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