Received-SPF: pass (google.com: domain of linux-kernel+bounces-132689-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Jiri Olsa <olsajiri@gmail.com>
Date: Fri, 5 Apr 2024 10:56:15 +0200
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Jiri Olsa <olsajiri@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, linux-kernel@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Borislav Petkov (AMD)" <bp@alien8.de>, x86@kernel.org,
	linux-api@vger.kernel.org
Subject: Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return
 probe
Message-ID: <Zg-8r63tPSkuhN7p@krava>
References: <20240402093302.2416467-2-jolsa@kernel.org>
 <20240403100708.233575a8ac2a5bac2192d180@kernel.org>
 <Zg0lvUIB4WdRUGw_@krava>
 <20240403230937.c3bd47ee47c102cd89713ee8@kernel.org>
 <CAEf4BzZ2RFfz8PNgJ4ENZ0us4uX=DWhYFimXdtWms-VvGXOjgQ@mail.gmail.com>
 <20240404095829.ec5db177f29cd29e849169fa@kernel.org>
 <CAEf4BzYH60TwvBipHWB_kUqZZ6D-iUVnnFsBv06imRikK3o-bg@mail.gmail.com>
 <20240405005405.9bcbe5072d2f32967501edb3@kernel.org>
 <20240404161108.GG7153@redhat.com>
 <20240405102203.825c4a2e9d1c2be5b2bffe96@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405102203.825c4a2e9d1c2be5b2bffe96@kernel.org>

On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> On Thu, 4 Apr 2024 18:11:09 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > On 04/05, Masami Hiramatsu wrote:
> > >
> > > Can we make this syscall and uprobe behavior clearer? As you said, if
> > > the application use sigreturn or longjump, it may skip returns and
> > > shadow stack entries are left in the kernel. In such cases, can uretprobe
> > > detect it properly, or just crash the process (or process runs wrongly)?
> > 
> > Please see the comment in handle_trampoline(), it tries to detect this case.
> > This patch should not make any difference.
> 
> I think you mean this loop will skip and discard the stacked return_instance
> to find the valid one.
> 
> ----
>         do {
>                 /*
>                  * We should throw out the frames invalidated by longjmp().
>                  * If this chain is valid, then the next one should be alive
>                  * or NULL; the latter case means that nobody but ri->func
>                  * could hit this trampoline on return. TODO: sigaltstack().
>                  */
>                 next = find_next_ret_chain(ri);
>                 valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);
> 
>                 instruction_pointer_set(regs, ri->orig_ret_vaddr);
>                 do {
>                         if (valid)
>                                 handle_uretprobe_chain(ri, regs);
>                         ri = free_ret_instance(ri);
>                         utask->depth--;
>                 } while (ri != next);
>         } while (!valid);
> ----
> 
> I think this expects setjmp/longjmp as below
> 
> foo() { <- retprobe1
> 	setjmp()
> 	bar() { <- retprobe2
> 		longjmp()
> 	}
> } <- return to trampoline
> 
> In this case, we need to skip retprobe2's instance.
> My concern is, if we can not find appropriate return instance, what happen?
> e.g.
> 
> foo() { <-- retprobe1
>    bar() { # sp is decremented
>        sys_uretprobe() <-- ??
>     }
> }
> 
> It seems sys_uretprobe() will handle retprobe1 at that point instead of
> SIGILL.

yes, and I think it's fine, you get the consumer called in wrong place,
but it's your fault and kernel won't crash

this can be fixed by checking the syscall is called from the trampoline
and prevent handle_trampoline call if it's not

> 
> Can we avoid this with below strict check?
> 
> if (ri->stack != regs->sp + expected_offset)
> 	goto sigill;

hm the current uprobe 'alive' check makes sure the return_instance is above
or at the same stack address, not sure we can match it exactly, need to think
about that more

> 
> expected_offset should be 16 (push * 3 - ret) on x64 if we ri->stack is the
> regs->sp right after call.

the syscall trampoline already updates the regs->sp before calling
handle_trampoline

        regs->sp += sizeof(r11_cx_ax);

jirka