Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace
From: Joel Fernandes
Date: Wed Jul 19 2023 - 07:52:07 EST
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@xxxxxxxxxxxxx> (maintainer:CHECKPATCH)
Cc: Joe Perches <joe@xxxxxxxxxxx> (maintainer:CHECKPATCH)
Cc: Dwaipayan Ray <dwaipayanray1@xxxxxxxxx> (reviewer:CHECKPATCH)
Cc: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>
Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
Cc: John Fastabend <john.fastabend@xxxxxxxxx>
Cc: <bpf@xxxxxxxxxxxxxxx>
Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
---
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@^kernel/bpf@ &&
+ $realfile !~ m@^include/linux/bpf@ &&
+ $realfile !~ m@^net/bpf@ &&
+ $realfile !~ m@^kernel/rcu@ &&
+ $realfile !~ m@^include/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@^$rcu_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@^kernel/bpf@ &&
$realfile !~ m@^include/linux/bpf@ &&
$realfile !~ m@^net/bpf@ &&
$realfile !~ m@^kernel/rcu@ &&
$realfile !~ m@^include/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@$rcu_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