On Wed, Jul 3, 2024 at 3:39 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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/20240508212605.4012172-4-andrii@xxxxxxxxxx/
[1] https://lore.kernel.org/linux-trace-kernel/20240522013845.1631305-4-andrii@xxxxxxxxxx/
>
> --
> Josh
