On Tue, 8 Jul 2025 08:53:56 -0700
Linus Torvalds <torva...@linux-foundation.org> wrote:
> On Tue, 8 Jul 2025 at 07:41, Steven Rostedt <rost...@goodmis.org> wrote:
> >
> > 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).
>
> I really think you should just not use 'user_access_begin()" AT ALL if
> you need to play these kinds of games.
>
Looking at the code a bit deeper, I don't think we need to play these games
and still keep the user_read_access_begin().
The places that are more performance critical (where it reads the sframe
during normal stack walking during profiling) has no debug output, and
there's nothing there that needs to take it out of the user_read_access
area.
It's the validator that adds these hacks. I don't think it needs to. It can
just wrap the calls to the code that requires user_read_access and then
check the return value. The validator is just a debugging feature and
performance should not be an issue.
But I do think performance is something to care about during normal
operations where the one big user_read_access_begin() can help.
What about something like this? It adds "safe" versions of the user space
access functions and uses them only in the slow (we don't care about
performance) validator:
-- Steve
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 0060cc576776..79ff3c0fc11f 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -321,7 +321,34 @@ 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)
+static int safe_read_fde(struct sframe_section *sec,
+ unsigned int fde_num, struct sframe_fde *fde)
+{
+ int ret;
+
+ if (!user_read_access_begin((void __user *)sec->sframe_start,
+ sec->sframe_end - sec->sframe_start))
+ return -EFAULT;
+ ret = __read_fde(sec, fde_num, fde);
+ user_read_access_end();
+ return ret;
+}
+
+static int safe_read_fre(struct sframe_section *sec,
+ struct sframe_fde *fde, unsigned long fre_addr,
+ struct sframe_fre *fre)
+{
+ int ret;
+
+ if (!user_read_access_begin((void __user *)sec->sframe_start,
+ sec->sframe_end - sec->sframe_start))
+ return -EFAULT;
+ ret = __read_fre(sec, fde, fre_addr, fre);
+ user_read_access_end();
+ return ret;
+}
+
+static int sframe_validate_section(struct sframe_section *sec)
{
unsigned long prev_ip = 0;
unsigned int i;
@@ -335,13 +362,13 @@ static __always_inline int
__sframe_validate_section(struct sframe_section *sec)
unsigned int j;
int ret;
- ret = __read_fde(sec, i, &fde);
+ ret = safe_read_fde(sec, i, &fde);
if (ret)
return ret;
ip = sec->sframe_start + fde.start_addr;
if (ip <= prev_ip) {
- dbg_sec_uaccess("fde %u not sorted\n", i);
+ dbg_sec("fde %u not sorted\n", i);
return -EFAULT;
}
prev_ip = ip;
@@ -353,17 +380,20 @@ static __always_inline int
__sframe_validate_section(struct sframe_section *sec)
fre = which ? fres : fres + 1;
which = !which;
- ret = __read_fre(sec, &fde, fre_addr, fre);
+ ret = safe_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);
+ dbg_sec("fde %u: __read_fre(%u) failed\n", i,
j);
+ dbg_sec("FDE: start_addr:0x%x func_size:0x%x
fres_off:0x%x fres_num:%d info:%u rep_size:%u\n",
+ fde.start_addr, fde.func_size,
+ fde.fres_off, fde.fres_num,
+ fde.info, fde.rep_size);
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);
+ dbg_sec("fde %u: fre %u not sorted\n", i, j);
return -EFAULT;
}
@@ -374,21 +404,6 @@ static __always_inline int
__sframe_validate_section(struct sframe_section *sec)
return 0;
}
-static int sframe_validate_section(struct sframe_section *sec)
-{
- int ret;
-
- if (!user_read_access_begin((void __user *)sec->sframe_start,
- sec->sframe_end - sec->sframe_start)) {
- dbg_sec("section usercopy failed\n");
- return -EFAULT;
- }
-
- ret = __sframe_validate_section(sec);
- user_read_access_end();
- return ret;
-}
-
#else /* !CONFIG_SFRAME_VALIDATION */
static int sframe_validate_section(struct sframe_section *sec) { return 0; }
