linux-kernel - Re: [PATCH 3/8] uprobes: protected uprobe lifetime with SRCU

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: <175e2c47-ad81-cc1d-18b9-d9644bc15925@huawei.com>
Date: Fri, 2 Aug 2024 09:30:52 +0800
From: "Liao, Chang" <liaochang1@...wei.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
CC: Andrii Nakryiko <andrii@...nel.org>, <linux-trace-kernel@...r.kernel.org>,
	<peterz@...radead.org>, <oleg@...hat.com>, <rostedt@...dmis.org>,
	<mhiramat@...nel.org>, <bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<jolsa@...nel.org>, <paulmck@...nel.org>
Subject: Re: [PATCH 3/8] uprobes: protected uprobe lifetime with SRCU



在 2024/8/2 0:49, Andrii Nakryiko 写道:
> On Thu, Aug 1, 2024 at 5:23 AM Liao, Chang <liaochang1@...wei.com> wrote:
>>
>>
>>
>> 在 2024/8/1 5:42, Andrii Nakryiko 写道:
>>> To avoid unnecessarily taking a (brief) refcount on uprobe during
>>> breakpoint handling in handle_swbp for entry uprobes, make find_uprobe()
>>> not take refcount, but protect the lifetime of a uprobe instance with
>>> RCU. This improves scalability, as refcount gets quite expensive due to
>>> cache line bouncing between multiple CPUs.
>>>
>>> Specifically, we utilize our own uprobe-specific SRCU instance for this
>>> RCU protection. put_uprobe() will delay actual kfree() using call_srcu().
>>>
>>> For now, uretprobe and single-stepping handling will still acquire
>>> refcount as necessary. We'll address these issues in follow up patches
>>> by making them use SRCU with timeout.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
>>> ---
>>>  kernel/events/uprobes.c | 93 ++++++++++++++++++++++++-----------------
>>>  1 file changed, 55 insertions(+), 38 deletions(-)
>>>
> 
> [...]
> 
>>>
>>> @@ -2258,12 +2275,12 @@ static void handle_swbp(struct pt_regs *regs)
>>>       if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
>>>               goto out;
>>>
>>> -     if (!pre_ssout(uprobe, regs, bp_vaddr))
>>> -             return;
>>> +     if (pre_ssout(uprobe, regs, bp_vaddr))
>>> +             goto out;
>>>
>>
>> Regardless what pre_ssout() returns, it always reach the label 'out', so the
>> if block is unnecessary.
> 
> yep, I know, but I felt like
> 
> if (something is wrong)
>     goto out;
> 
> pattern was important to keep for each possible failing step for consistency.
> 
> so unless this is a big deal, I'd keep it as is, as in the future
> there might be some other steps after pre_ssout() before returning, so
> this is a bit more "composable"
> 
OK, I would say this conditional-check pattern is likely to be optimized away by
modern compiler.

Thanks.

> 
>>
>>
>>> -     /* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */
>>>  out:
>>> -     put_uprobe(uprobe);
>>> +     /* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */
>>> +     srcu_read_unlock(&uprobes_srcu, srcu_idx);
>>>  }
>>>
>>>  /*
>>
>> --
>> BR
>> Liao, Chang

-- 
BR
Liao, Chang