Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a0f6e131-a649-1731-b096-46313a0460a9@joelfernandes.org>
Date:   Wed, 19 Jul 2023 07:51:58 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU
 Tasks Trace
Content-Language: en-US
To:     paulmck@kernel.org, Joe Perches <joe@perches.com>
Cc:     rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@meta.com, rostedt@goodmis.org,
        Andy Whitcroft <apw@canonical.com>,
        Dwaipayan Ray <dwaipayanray1@gmail.com>,
        Lukas Bulwahn <lukas.bulwahn@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>, bpf@vger.kernel.org
References: <a6fff63c-5930-4918-82a3-a9301309d88d@paulmck-laptop>
 <20230717180454.1097714-5-paulmck@kernel.org>
 <04e74fd214a01bee0fb5ac690730cb386536cced.camel@perches.com>
 <8477fd32-38a5-4d66-8deb-a61b0e290df5@paulmck-laptop>
From:   Joel Fernandes <joel@joelfernandes.org>
In-Reply-To: <8477fd32-38a5-4d66-8deb-a61b0e290df5@paulmck-laptop>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@canonical.com> (maintainer:CHECKPATCH)
>>> Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
>>> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
>>> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: <bpf@vger.kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.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@^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