| 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
| ||
|
Message-ID: <a0f6e131-a649-1731-b096-46313a0460a9@joelfernandes.org>
Date: Wed, 19 Jul 2023 07:51:58 -0400
From: Joel Fernandes <joel@...lfernandes.org>
To: paulmck@...nel.org, Joe Perches <joe@...ches.com>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-team@...a.com, rostedt@...dmis.org,
Andy Whitcroft <apw@...onical.com>,
Dwaipayan Ray <dwaipayanray1@...il.com>,
Lukas Bulwahn <lukas.bulwahn@...il.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org
Subject: Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU
Tasks Trace
On 7/17/23 19:34, Paul E. McKenney wrote:
> On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
>> On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
>>> RCU Tasks Trace is quite specialized, having been created specifically
>>> for sleepable BPF programs. Because it allows general blocking within
>>> readers, any new use of RCU Tasks Trace must take current use cases into
>>> account. Therefore, update checkpatch.pl to complain about use of any of
>>> the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
>>>
>>> Cc: Andy Whitcroft <apw@...onical.com> (maintainer:CHECKPATCH)
>>> Cc: Joe Perches <joe@...ches.com> (maintainer:CHECKPATCH)
>>> Cc: Dwaipayan Ray <dwaipayanray1@...il.com> (reviewer:CHECKPATCH)
>>> Cc: Lukas Bulwahn <lukas.bulwahn@...il.com>
>>> Cc: Alexei Starovoitov <ast@...nel.org>
>>> Cc: Daniel Borkmann <daniel@...earbox.net>
>>> Cc: John Fastabend <john.fastabend@...il.com>
>>> Cc: <bpf@...r.kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
>>> ---
>>> scripts/checkpatch.pl | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> []
>>> @@ -7457,6 +7457,24 @@ sub process {
>>> }
>>> }
>>>
>>> +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
>>> + if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
>>> + $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
>>> + $line =~ /\brcu_read_unlock_trace\s*\(/ ||
>>> + $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
>>> + $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
>>> + $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
>>> + $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
>>> + if ($realfile !~ m@...rnel/bpf@ &&
>>> + $realfile !~ m@...clude/linux/bpf@ &&
>>> + $realfile !~ m@...t/bpf@ &&
>>> + $realfile !~ m@...rnel/rcu@ &&
>>> + $realfile !~ m@...clude/linux/rcu@) {
>>
>> Functions and paths like these tend to be accreted.
>>
>> Please use a variable or 2 like:
>>
>> our $rcu_trace_funcs = qr{(?x:
>> rcu_read_lock_trace |
>> rcu_read_lock_trace_held |
>> rcu_read_unlock_trace |
>> call_rcu_tasks_trace |
>> synchronize_rcu_tasks_trace |
>> rcu_barrier_tasks_trace |
>> rcu_request_urgent_qs_task
>> )};
>> our $rcu_trace_paths = qr{(?x:
>> kernel/bfp/ |
>> include/linux/bpf |
>> net/bpf/ |
>> kernel/rcu/ |
>> include/linux/rcu
>> )};
>
> Like this?
>
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> our $rcu_trace_funcs = qr{(?x:
> rcu_read_lock_trace |
> rcu_read_lock_trace_held |
> rcu_read_unlock_trace |
> call_rcu_tasks_trace |
> synchronize_rcu_tasks_trace |
> rcu_barrier_tasks_trace |
> rcu_request_urgent_qs_task
> )};
> our $rcu_trace_paths = qr{(?x:
> kernel/bfp/ |
> include/linux/bpf |
> net/bpf/ |
> kernel/rcu/ |
> include/linux/rcu
> )};
> if ($line =~ /$rcu_trace_funcs/) {
> if ($realfile !~ m@...cu_trace_paths@) {
> WARN("RCU_TASKS_TRACE",
> "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> }
> }
>
> No, that is definitely wrong. It has lost track of the list of pathnames,
> thus complaining about uses of those functions in files where their use
> is permitted.
>
> But this seems to work:
>
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> our $rcu_trace_funcs = qr{(?x:
> rcu_read_lock_trace |
> rcu_read_lock_trace_held |
> rcu_read_unlock_trace |
> call_rcu_tasks_trace |
> synchronize_rcu_tasks_trace |
> rcu_barrier_tasks_trace |
> rcu_request_urgent_qs_task
> )};
> if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> if ($realfile !~ m@...rnel/bpf@ &&
> $realfile !~ m@...clude/linux/bpf@ &&
> $realfile !~ m@...t/bpf@ &&
> $realfile !~ m@...rnel/rcu@ &&
> $realfile !~ m@...clude/linux/rcu@) {
> WARN("RCU_TASKS_TRACE",
> "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> }
> }
>
> Maybe the "^" needs to be distributed into $rcu_trace_paths?
>
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> our $rcu_trace_funcs = qr{(?x:
> rcu_read_lock_trace |
> rcu_read_lock_trace_held |
> rcu_read_unlock_trace |
> call_rcu_tasks_trace |
> synchronize_rcu_tasks_trace |
> rcu_barrier_tasks_trace |
> rcu_request_urgent_qs_task
> )};
> our $rcu_trace_paths = qr{(?x:
> ^kernel/bfp/ |
> ^include/linux/bpf |
> ^net/bpf/ |
> ^kernel/rcu/ |
> ^include/linux/rcu
> )};
> if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> if ($realfile !~ m@...u_trace_paths@) {
> WARN("RCU_TASKS_TRACE",
> "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> }
> }
>
> But no joy here, either. Which is no surprise, given that perl is
> happy to distribute the "\b" and the "\s*\(" across the elements of
> $rcu_trace_funcs. I tried a number of other variations, including
> inverting the "if" condition "(!(... =~ ...))", inverting the "if"
> condition via an empty "then" clause, replacing the "m@" with "/",
> replacing the "|" in the "qr{}" with "&", and a few others.
>
> Again, listing the pathnames explicitly in the second "if" condition
> works just fine.
>
Not a perl expert but I wonder if the following are any options at all:
1. Instead of having a complex list of strings in a regex variable, it
might be easier to hold the strings as a perl array, and then iterate
over that array checking each element of the array on every iteration,
against the line.
2. Roll the "\b" and/or "^" in into the regex variable itself than
trying make them play with the variable later.
3. Use parentheses around the variable? Not sure if that will work but I
wonder if it has something to do with operator precedence.
4. Instead of a list of paths, maybe it is better to look for "rcu" or
"bpf" in the regex itself? That way new paths don't require script
updates (at the expense though of false-positives (highly unlikely, IMHO)).
thanks,
- Joel
Powered by blists - more mailing lists