linux-kernel - Re: checkpatch false positon on EXPORT

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Apr 2016 13:59:04 +0100
From:	Andy Whitcroft <apw@...onical.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Daniel Walker <danielwa@...co.com>,
	open list <linux-kernel@...r.kernel.org>,
	Daniel Walker <dwalker@...o99.com>,
	"xe-kernel@...ernal.cisco.com" <xe-kernel@...ernal.cisco.com>
Subject: Re: checkpatch false positon on EXPORT_SYMBOL

On Mon, Apr 11, 2016 at 03:09:42PM -0700, Joe Perches wrote:
> On Mon, 2016-04-11 at 14:51 -0700, Daniel Walker wrote:
> > On 03/31/2016 12:21 PM, Joe Perches wrote:
> > > 
> > > On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
> > > > 
> > > > The below looks like normal code but the last export symbol gets the
> > > > warning,
> > > > 
> > > > 
> > > > WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its
> > > > function/variable
> > > > #16: FILE: kernel/acct.c:70:
> > > > +EXPORT_SYMBOL(test_export);    /* Error ! */
> > > > 
> > > > It seems to have to do with the comments at the end of the line. The
> > > > first two examples don't have warnings because I removed the comments on
> > > > different lines. comments on the variable and export symbol lines gets
> > > > the error tho.
> > > That looks like a false positive I'll leave for Andy.
> > > 
> > > $ cat ~/export_symbol.c
> > > int test_export_no_comment;
> > > EXPORT_SYMBOL(test_export_no_comment);
> > > int test_export_comment_int;		/* comment int */
> > > EXPORT_SYMBOL(test_export_int);
> > > int test_export_comment_symbol;
> > > EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
> > > int test_export_both;			/* comment both 1 */
> > > EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
> > > $
> > > 
> > > Something's a bit off with the $stat variable:
> > > 
> > > test_export_int doesn't match the EXPORT_SYMBOL test.
> > > test_export_symbol and test_export_both get warnings.
> > > 
> > 
> > Did this get solved? I haven't see anything else on it.
> 
> Not by me.
> 
> I punted to Andy and I haven't heard from him.
> 
> There aren't many cases of this defect in the current
> kernel tree, so I don't know how much he might care.

After some debugging it seems we are essentially not finding the
appropriate "next line" when we are parsing either of the second or
third entries.  This leads us to not check the second one at all, and to
check the third one only when think we are parsing the comment.

This all stems from us thinking there are two statements on the same line
as the trailing ; is not actually at the end of line so the next statement
is still on this same line.  Basically inline comments should be considered
as spaces for the purposes of determining the next line for this purpose.

The following patch appears to sort this out.  A quick scan says this
entire next line calculation is still only used for the EXPORT* check so
this should be low risk for other tests.

This works for me on your example, if you have a real world one could
you test it there and let us know.

Thanks.

-apw

>From 223fc7ef4ca0134bf64af0a107532dc3e4010c87 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@...onical.com>
Date: Tue, 12 Apr 2016 13:43:46 +0100
Subject: [PATCH] checkpatch: comments are whitespace for the purposes of
 finding the next line

While parsing statements we are recording the nominal next line for the
purposes of checking that EXPORT* follows exactly on below an appropriate
statement.  Where there is whitespace after a statement end marker (such
as ;) we will move to the next line.  This also needs to apply to inline
comments at the end of a line.

Allows us to more correctly parse:

    +int test_export;
    +EXPORT_SYMBOL(test_export);    /* No Error ! */
    +
    +int test_export2;    /* No Error below */
    +EXPORT_SYMBOL(test_export2);
    +
    +int test_export3;    /* Error below */
    +EXPORT_SYMBOL(test_export3);    /* Error ! */
    +

Reported-by: Daniel Walker <danielwa@...co.com>
Signed-off-by: Andy Whitcroft <apw@...onical.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..b581529f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3000,7 +3000,7 @@ sub process {
 			$realline_next = $line_nr_next;
 			if (defined $realline_next &&
 			    (!defined $lines[$realline_next - 1] ||
-			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
+			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
 				$realline_next++;
 			}
 
-- 
2.7.4