linux-kernel - Re: [PATCH] uprobes: Improve scalability by reducing the contention on siglock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240807101746.GA27715@redhat.com>
Date: Wed, 7 Aug 2024 12:17:47 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: "Liao, Chang" <liaochang1@...wei.com>
Cc: mhiramat@...nel.org, peterz@...radead.org, mingo@...hat.com,
	acme@...nel.org, namhyung@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] uprobes: Improve scalability by reducing the contention
 on siglock

So. Liao, I am sorry, but I dislike your patch/approach in any case.

UTASK_SSTEP_DENY_SIGNAL complicates the state machine. And I don't like the fact
that set_thread_flag(TIF_SIGPENDING) is called twice, from handle_singlestep()
and uprobe_post_sstep_notifier(), this complicates the logic even more.

We need a flag, not the new state.

And if I read this patch correctly it is wrong:

	- uprobe_deny_signal() clears TIF_SIGPENDING and sets UTASK_SSTEP_DENY_SIGNAL

	- another signal cames after that and sets TIF_SIGPENDING again

	- in this case the task won't return to user-space and execute the probed
	  insn, exit_to_user_mode_loop() will notice another TIF_SIGPENDING and
	  call arch_do_signal_or_restart()->get_signal() again.

	- get_signal() will call uprobe_deny_signal() again hit

		WARN_ON_ONCE(utask->state != UTASK_SSTEP);


And no, we shouldn't change this check into UTASK_SSTEP || UTASK_SSTEP_DENY_SIGNAL.
Again, the fact that uprobe_deny_signal() cleared TIF_SIGPENDING must not be the
new state.

Oleg.

On 08/06, Oleg Nesterov wrote:
>
> On 08/06, Liao, Chang wrote:
> >
> > You're absolutely right. handle_signlestep() has chance to handle _DENY_SIGANL
> > unless it followed by setting TIF_UPROBE in uprobe_deny_signal(). This means
> > _DENY_SIGNAL is likey replaced during next uprobe single-stepping.
> >
> > I believe introducing _DENY_SIGNAL as the immediate state between UTASK_SSTEP
> > and UTASK_SSTEP_ACK is still necessary. This allow uprobe_post_sstep_notifier()
> > to correctly restore TIF_SIGPENDING upon the completion of single-step.
> >
> > A revised implementation would look like this:
>
> Still looks "obviously wrong" to me... even the approach itself.
>
> Perhaps I am wrong, yet another day when I can't even read emails on lkml
> carefully, sorry.
>
> But can you please send the patch which I could actually apply? This one
> looks white-space damaged...
>
> I'll try to reply with more details as soon I convince myself I fully
> understand what does your patch actually do, but most probably not tomorrow.
>
> Thanks,
>
> Oleg.
>
> > ------------------%<------------------
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1980,6 +1980,7 @@ bool uprobe_deny_signal(void)
> >
> >         if (task_sigpending(t)) {
> >                 clear_tsk_thread_flag(t, TIF_SIGPENDING);
> > +               utask->state = UTASK_SSTEP_DENY_SIGNAL;
> >
> >                 if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
> >                         utask->state = UTASK_SSTEP_TRAPPED;
> > @@ -2276,22 +2277,23 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
> >         int err = 0;
> >
> >         uprobe = utask->active_uprobe;
> > -       if (utask->state == UTASK_SSTEP_ACK)
> > +       switch (utask->state) {
> > +       case UTASK_SSTEP_ACK:
> >                 err = arch_uprobe_post_xol(&uprobe->arch, regs);
> > -       else if (utask->state == UTASK_SSTEP_TRAPPED)
> > +               break;
> > +       case UTASK_SSTEP_TRAPPED:
> >                 arch_uprobe_abort_xol(&uprobe->arch, regs);
> > -       else
> > +               set_thread_flag(TIF_SIGPENDING);
> > +               break;
> > +       default:
> >                 WARN_ON_ONCE(1);
> > +       }
> >
> >         put_uprobe(uprobe);
> >         utask->active_uprobe = NULL;
> >         utask->state = UTASK_RUNNING;
> >         xol_free_insn_slot(current);
> >
> > -       spin_lock_irq(&current->sighand->siglock);
> > -       recalc_sigpending(); /* see uprobe_deny_signal() */
> > -       spin_unlock_irq(&current->sighand->siglock);
> > -
> >         if (unlikely(err)) {
> >                 uprobe_warn(current, "execute the probed insn, sending SIGILL.");
> >                 force_sig(SIGILL);
> > @@ -2351,6 +2353,8 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
> >                 /* task is currently not uprobed */
> >                 return 0;
> >
> > +       if (utask->state == UTASK_SSTEP_DENY_SIGNAL)
> > +               set_thread_flag(TIF_SIGPENDING);
> >         utask->state = UTASK_SSTEP_ACK;
> >         set_thread_flag(TIF_UPROBE);
> >         return 1;
> >
> > ------------------>%------------------
> >
> > >
> > > Oleg.
> > >
> > >
> >
> > --
> > BR
> > Liao, Chang
> >