On Wed, Jul 3, 2024 at 3:39 PM Josh Poimboeuf <[email protected]> wrote: > > On Tue, Jul 02, 2024 at 09:02:03PM -0700, Andrii Nakryiko wrote: > > @@ -2833,6 +2858,18 @@ perf_callchain_user32(struct pt_regs *regs, struct > > perf_callchain_entry_ctx *ent > > > > fp = compat_ptr(ss_base + regs->bp); > > pagefault_disable(); > > + > > +#ifdef CONFIG_UPROBES > > + /* see perf_callchain_user() below for why we do this */ > > + if (current->utask) { > > + u32 ret_addr; > > + > > + if (is_uprobe_at_func_entry(regs, current->utask->auprobe) && > > + !__get_user(ret_addr, (const u32 __user *)regs->sp)) > > Shouldn't the regs->sp value be checked with __access_ok() before > calling __get_user()?
Ah, it's __get_user vs get_user quirk, right? Should I just use get_user() here? It seems like existing code is trying to avoid two __access_ok() checks for two fields of stack_frame, but here we don't have that optimization opportunity anyways. > > Also, instead of littering functions with ifdefs it would be better to > abstract this out into a separate function which has an "always return > false" version for !CONFIG_UPROBES. Then the above could be simplified to > something like: Sure, can do. > > ... > pagefault_disable(); But I'd leave pagefault_disable() outside of that function, because caller has to do it either way. > > if (is_uprobe_at_func_entry(regs, current) && > __access_ok(regs->sp, 4) && > !__get_user(ret_addr, (const u32 __user *)regs->sp)) > perf_callchain_store(entry, ret_addr); > ... > > Also it's good practice to wait at least several days before posting new > versions to avoid spamming reviewers and to give them time to digest > what you've already sent. I'm not sure about "at least several days", tbh. I generally try to not post more often than once a day, and that only if I received some meaningful reviewing feedback (like in your case). I do wait a few days for reviews before pinging the mailing list again, though. Would I get this feedback if I haven't posted v3? Or we'd just be delaying the inevitable for a few more idle days? This particular change (in it's initial version before yours and recent Peter's comments) has been sitting under review since May 8th ([0], and then posted without changes on May 21st, [1]), so I'm not exactly rushing the things here. Either way, I won't get to this until next week, so I won't spam you too much anymore, sorry. [0] https://lore.kernel.org/linux-trace-kernel/[email protected]/ [1] https://lore.kernel.org/linux-trace-kernel/[email protected]/ > > -- > Josh
