On Tue, 8 Jul 2025 07:34:36 -0700
Josh Poimboeuf <jpoim...@kernel.org> wrote:
> I had found those debug printks really useful for debugging
> corrupt/missing .sframe data, but yeah, this patch is ridiculously bad.
> Sorry for putting that out into the world ;-)
>
> And those are all error paths, so it's rather pointless to do that whole
> uaccess disable/enable/disable dance.
>
> So yeah, drop it for now and I can replace it with something better
> later on.
Would something like this work? If someone enables the config to enable the
validation, I don't think we need dynamic printk to do it (as that requires
passing in the format directly and not via a pointer).
-- Steve
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 0060cc576776..524738e2b823 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -321,11 +321,24 @@ int sframe_find(unsigned long ip, struct
unwind_user_frame *frame)
#ifdef CONFIG_SFRAME_VALIDATION
-static __always_inline int __sframe_validate_section(struct sframe_section
*sec)
+/* Used to save error messages in uaccess sections */
+struct err_msg {
+ const char *fmt;
+ int param1;
+ int param2;
+ long param3;
+ long param4;
+};
+
+static __always_inline
+int __sframe_validate_section(struct sframe_section *sec, struct err_msg *err)
{
unsigned long prev_ip = 0;
unsigned int i;
+/* current->comm, current->pid, sec->filename */
+#define ERR_HDR KERN_WARNING "%s (%d) %s: "
+
for (i = 0; i < sec->num_fdes; i++) {
struct sframe_fre *fre, *prev_fre = NULL;
unsigned long ip, fre_addr;
@@ -341,7 +354,8 @@ static __always_inline int __sframe_validate_section(struct
sframe_section *sec)
ip = sec->sframe_start + fde.start_addr;
if (ip <= prev_ip) {
- dbg_sec_uaccess("fde %u not sorted\n", i);
+ err->fmt = ERR_HDR "fde %u not sorted\n";
+ err->param1 = i;
return -EFAULT;
}
prev_ip = ip;
@@ -355,15 +369,23 @@ static __always_inline int
__sframe_validate_section(struct sframe_section *sec)
ret = __read_fre(sec, &fde, fre_addr, fre);
if (ret) {
- dbg_sec_uaccess("fde %u: __read_fre(%u)
failed\n", i, j);
- dbg_print_fde_uaccess(sec, &fde);
+ err->fmt = ERR_HDR
+ "fde %u: __read_fre(%u) failed\n"
+ " frame_start=%lx frame_end=%lx\n";
+ err->param1 = i;
+ err->param2 = j;
+ err->param3 = (long)sec->sframe_start;
+ err->param4 = (long)sec->sframe_end;
return ret;
}
fre_addr += fre->size;
if (prev_fre && fre->ip_off <= prev_fre->ip_off) {
- dbg_sec_uaccess("fde %u: fre %u not sorted\n",
i, j);
+ err->fmt = ERR_HDR
+ "fde %u: fre %u not sorted\n";
+ err->param1 = i;
+ err->param2 = j;
return -EFAULT;
}
@@ -376,16 +398,26 @@ static __always_inline int
__sframe_validate_section(struct sframe_section *sec)
static int sframe_validate_section(struct sframe_section *sec)
{
+ struct err_msg err;
int ret;
+ memset(&err, 0, sizeof(err));
+
if (!user_read_access_begin((void __user *)sec->sframe_start,
sec->sframe_end - sec->sframe_start)) {
- dbg_sec("section usercopy failed\n");
+ pr_warn("%s (%d): section usercopy failed\n",
+ current->comm, current->pid);
return -EFAULT;
}
- ret = __sframe_validate_section(sec);
+ ret = __sframe_validate_section(sec, &err);
user_read_access_end();
+
+ if (ret) {
+ printk(err.fmt, current->comm, current->pid,
+ sec->filename, err.param1, err.param2,
+ err.param3, err.param4);
+ }
return ret;
}
