[rcu,5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

Commit Message

Paul E. McKenney July 17, 2023, 6:04 p.m. UTC

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 <[e-mail]> (maintainer:CHECKPATCH)
Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
Cc: Lukas Bulwahn <[e-mail]>
Cc: Alexei Starovoitov <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
Cc: John Fastabend <[e-mail]>
Cc: <[e-mail]>
Signed-off-by: Paul E. McKenney <[e-mail]>
---
 scripts/checkpatch.pl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Joe Perches July 17, 2023, 10:34 p.m. UTC | #1

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 <[e-mail]> (maintainer:CHECKPATCH)
> Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> Cc: Lukas Bulwahn <[e-mail]>
> Cc: Alexei Starovoitov <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> Cc: John Fastabend <[e-mail]>
> Cc: <[e-mail]>
> Signed-off-by: Paul E. McKenney <[e-mail]>
> ---
>  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
)};

...
	
		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
		    $realfile !~ m{^$rcu_trace_paths}) {
			WARN("RCU_TASKS_TRACE",
			     "use of RCU tasks trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);			}
		}

Paul E. McKenney July 17, 2023, 11:34 p.m. UTC | #2

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 <[e-mail]> (maintainer:CHECKPATCH)
> > Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> > Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> > Cc: Lukas Bulwahn <[e-mail]>
> > Cc: Alexei Starovoitov <[e-mail]>
> > Cc: Daniel Borkmann <[e-mail]>
> > Cc: John Fastabend <[e-mail]>
> > Cc: <[e-mail]>
> > Signed-off-by: Paul E. McKenney <[e-mail]>
> > ---
> >  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.

Help?

							Thanx, Paul

Joel Fernandes July 19, 2023, 11:51 a.m. UTC | #3

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 <[e-mail]> (maintainer:CHECKPATCH)
>>> Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
>>> Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
>>> Cc: Lukas Bulwahn <[e-mail]>
>>> Cc: Alexei Starovoitov <[e-mail]>
>>> Cc: Daniel Borkmann <[e-mail]>
>>> Cc: John Fastabend <[e-mail]>
>>> Cc: <[e-mail]>
>>> Signed-off-by: Paul E. McKenney <[e-mail]>
>>> ---
>>>   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

Paul E. McKenney July 19, 2023, 6:27 p.m. UTC | #4

On Wed, Jul 19, 2023 at 07:51:58AM -0400, Joel Fernandes wrote:
> 
> 
> 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 <[e-mail]> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <[e-mail]>
> > > > Cc: Alexei Starovoitov <[e-mail]>
> > > > Cc: Daniel Borkmann <[e-mail]>
> > > > Cc: John Fastabend <[e-mail]>
> > > > Cc: <[e-mail]>
> > > > Signed-off-by: Paul E. McKenney <[e-mail]>
> > > > ---
> > > >   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)).

Given perl's tendency to have corner cases in its corner cases, I
am guessing that the "^" character combined with the "/" character is
causing trouble here.  Especially given that I don't see any use of such
a pattern anywhere in checkpatch.pl except directly in a "~" expression,
and there are a lot of those.

So I will keep it as is unless I hear otherwise from Joe Perches.

							Thanx, Paul

Joe Perches July 19, 2023, 6:44 p.m. UTC | #5

On Wed, 2023-07-19 at 11:27 -0700, Paul E. McKenney wrote:
[]
> > > > 
> Given perl's tendency to have corner cases in its corner cases, I
> am guessing that the "^" character combined with the "/" character is
> causing trouble here.  Especially given that I don't see any use of such
> a pattern anywhere in checkpatch.pl except directly in a "~" expression,
> and there are a lot of those.
> 
> So I will keep it as is unless I hear otherwise from Joe Perches.

I played with it a bit and can't think of anything better.

Code is always something that can be improved later so the
way Paul has it now is fine with me.

Paul E. McKenney July 19, 2023, 7:02 p.m. UTC | #6

On Wed, Jul 19, 2023 at 11:44:17AM -0700, Joe Perches wrote:
> On Wed, 2023-07-19 at 11:27 -0700, Paul E. McKenney wrote:
> []
> > > > > 
> > Given perl's tendency to have corner cases in its corner cases, I
> > am guessing that the "^" character combined with the "/" character is
> > causing trouble here.  Especially given that I don't see any use of such
> > a pattern anywhere in checkpatch.pl except directly in a "~" expression,
> > and there are a lot of those.
> > 
> > So I will keep it as is unless I hear otherwise from Joe Perches.
> 
> I played with it a bit and can't think of anything better.
> 
> Code is always something that can be improved later so the
> way Paul has it now is fine with me.

Then I don't feel so bad not finding anything better myself.

Thank you, and if nothing else this exercise refreshed a few of my
perl neurons.  ;-)

							Thanx, Paul

Joe Perches July 20, 2023, 7:29 a.m. UTC | #7

On Mon, 2023-07-17 at 16:34 -0700, 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 <[e-mail]> (maintainer:CHECKPATCH)
> > > Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> > > Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> > > Cc: Lukas Bulwahn <[e-mail]>
> > > Cc: Alexei Starovoitov <[e-mail]>
> > > Cc: Daniel Borkmann <[e-mail]>
> > > Cc: John Fastabend <[e-mail]>
> > > Cc: <[e-mail]>
> > > Signed-off-by: Paul E. McKenney <[e-mail]>
> > > ---
> > >  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/ |
		^^
	kernel/bfp/ |

(umm, oops...)
I think my original suggestion works better when I don't typo the path.

> > 	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.
> 
> Help?
> 
> 							Thanx, Paul

Paul E. McKenney July 20, 2023, 7:43 p.m. UTC | #8

On Thu, Jul 20, 2023 at 12:29:42AM -0700, Joe Perches wrote:
> On Mon, 2023-07-17 at 16:34 -0700, 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 <[e-mail]> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <[e-mail]>
> > > > Cc: Alexei Starovoitov <[e-mail]>
> > > > Cc: Daniel Borkmann <[e-mail]>
> > > > Cc: John Fastabend <[e-mail]>
> > > > Cc: <[e-mail]>
> > > > Signed-off-by: Paul E. McKenney <[e-mail]>
> > > > ---
> > > >  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/ |
> 		^^
> 	kernel/bfp/ |
> 
> (umm, oops...)
> I think my original suggestion works better when I don't typo the path.

Color me blind!  ;-)

That works much better, thank you!  I will update the patch on my
next rebase.

							Thanx, Paul

> > > 	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.
> > 
> > Help?
> > 
> > 							Thanx, Paul
>

Paul E. McKenney July 21, 2023, 3:56 a.m. UTC | #9

On Thu, Jul 20, 2023 at 12:43:55PM -0700, Paul E. McKenney wrote:
> On Thu, Jul 20, 2023 at 12:29:42AM -0700, Joe Perches wrote:
> > On Mon, 2023-07-17 at 16:34 -0700, 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 <[e-mail]> (maintainer:CHECKPATCH)
> > > > > Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
> > > > > Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
> > > > > Cc: Lukas Bulwahn <[e-mail]>
> > > > > Cc: Alexei Starovoitov <[e-mail]>
> > > > > Cc: Daniel Borkmann <[e-mail]>
> > > > > Cc: John Fastabend <[e-mail]>
> > > > > Cc: <[e-mail]>
> > > > > Signed-off-by: Paul E. McKenney <[e-mail]>
> > > > > ---
> > > > >  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/ |
> > 		^^
> > 	kernel/bfp/ |
> > 
> > (umm, oops...)
> > I think my original suggestion works better when I don't typo the path.
> 
> Color me blind!  ;-)
> 
> That works much better, thank you!  I will update the patch on my
> next rebase.

As shown below.  Is this what you had in mind?

							Thanx, Paul

------------------------------------------------------------------------

commit 496aa3821b40459b107f4bbc14ca867daad21fb6
Author: Paul E. McKenney <[e-mail]>
Date:   Thu Jul 6 11:48:07 2023 -0700

    checkpatch: Complain about unexpected uses of RCU Tasks Trace
    
    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.
    
    [ paulmck: Apply Joe Perches feedback. ]
    
    Cc: Andy Whitcroft <[e-mail]> (maintainer:CHECKPATCH)
    Cc: Joe Perches <[e-mail]> (maintainer:CHECKPATCH)
    Cc: Dwaipayan Ray <[e-mail]> (reviewer:CHECKPATCH)
    Cc: Lukas Bulwahn <[e-mail]>
    Cc: Alexei Starovoitov <[e-mail]>
    Cc: Daniel Borkmann <[e-mail]>
    Cc: John Fastabend <[e-mail]>
    Cc: <[e-mail]>
    Signed-off-by: Paul E. McKenney <[e-mail]>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 880fde13d9b8..a67e682e896c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7457,6 +7457,30 @@ sub process {
 			}
 		}
 
+# 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/bpf/ |
+			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);
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {

Joe Perches July 21, 2023, 4:38 a.m. UTC | #10

On Thu, 2023-07-20 at 20:56 -0700, Paul E. McKenney wrote:

> 
> > That works much better, thank you!  I will update the patch on my
> > next rebase.
> 
> As shown below.  Is this what you had in mind?
[]
> commit 496aa3821b40459b107f4bbc14ca867daad21fb6
> Author: Paul E. McKenney <[e-mail]>
> Date:   Thu Jul 6 11:48:07 2023 -0700
> 
>     checkpatch: Complain about unexpected uses of RCU Tasks Trace
>     
>     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.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7457,6 +7457,30 @@ sub process {
>  			}
>  		}
>  
> +# 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/bpf/ |
> +			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);

Exactly yes.

(though I still suggest a capture group to show the function like below)

		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
		    $realfile !~ m{^$rcu_trace_paths}) {
			WARN("RCU_TASKS_TRACE",
			     "use of RCU task trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);
		}

Paul E. McKenney July 21, 2023, 10:34 p.m. UTC | #11

On Thu, Jul 20, 2023 at 09:38:51PM -0700, Joe Perches wrote:
> On Thu, 2023-07-20 at 20:56 -0700, Paul E. McKenney wrote:
> 
> > 
> > > That works much better, thank you!  I will update the patch on my
> > > next rebase.
> > 
> > As shown below.  Is this what you had in mind?
> []
> > commit 496aa3821b40459b107f4bbc14ca867daad21fb6
> > Author: Paul E. McKenney <[e-mail]>
> > Date:   Thu Jul 6 11:48:07 2023 -0700
> > 
> >     checkpatch: Complain about unexpected uses of RCU Tasks Trace
> >     
> >     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.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -7457,6 +7457,30 @@ sub process {
> >  			}
> >  		}
> >  
> > +# 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/bpf/ |
> > +			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);
> 
> Exactly yes.
> 
> (though I still suggest a capture group to show the function like below)
> 
> 		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
> 		    $realfile !~ m{^$rcu_trace_paths}) {
> 			WARN("RCU_TASKS_TRACE",
> 			     "use of RCU task trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);
> 		}

That does seem to work!

I will fold this change in on my next rebase.

							Thanx, Paul

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 880fde13d9b8..24bab980bc6f 100755
--- 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@) {
+				WARN("RCU_TASKS_TRACE",
+				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {