linux-kernel - Re: False positive for 'Possible unnecessary KERN

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]
Message-ID: <faefcecf38e0c108be86f05f79f290e918016c13.camel@perches.com>
Date:   Wed, 15 Sep 2021 20:46:22 -0700
From:   Joe Perches <joe@...ches.com>
To:     Niklas Söderlund 
        <niklas.soderlund@...igine.com>
Cc:     Andy Whitcroft <apw@...onical.com>, linux-kernel@...r.kernel.org
Subject: Re: False positive for 'Possible unnecessary KERN_ERR' warning in
 checkpatch

On Tue, 2021-09-14 at 18:05 +0200, Niklas Söderlund wrote:
> Hi Joe,
> 
> Maybe you are already aware of this, but in case you are not.
> 
> The issue is the checkpatch check for unnecessary KERN_<LEVEL> for log 
> functions. If a single line contains a statement that match a log 
> function name from $logFunctions, such as foo_err() and the same line 
> contains a KERN_<LEVEL> statement the 'WARNING: Possible unnecessary 
> KERN_ERR' is triggered. This is true even if the KERN_<LEVEL> statement 
> is not part of the arguments to the foo_err() definition that triggers 
> the first part of the check.
> 
> This can be demonstrated by,
> 
>     ./scripts/checkpatch.pl --mailback --git c821e617896e99b8
> 
> Where we get (among others) the warning,
> 
>     WARNING: Possible unnecessary KERN_ERR
>     #38: FILE: drivers/net/ethernet/netronome/nfp/nfp_net.h:63:
>     +#define nn_err(nn, fmt, args...)	nn_pr(nn, KERN_ERR, fmt, ## args)
> 
> Looking at the code in checkpatch.pl we have,
> 
> our $logFunctions = qr{(?x:
>         printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
>         (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
>         TP_printk|
>         WARN(?:_RATELIMIT|_ONCE|)|
>         panic|
>         MODULE_[A-Z_]+|
>         seq_vprintf|seq_printf|seq_puts
> )};
> 
> ...
> 
> # check for logging functions with KERN_<LEVEL>
>                 if ($line !~ /printk(?:_ratelimited|_once)?\s*\(/ &&
>                     $line =~ /\b$logFunctions\s*\(.*\b(KERN_[A-Z]+)\b/) {
>                         my $level = $1;
>                         if (WARN("UNNECESSARY_KERN_LEVEL",
>                                  "Possible unnecessary $level\n" . $herecurr) &&
>                             $fix) {
>                                 $fixed[$fixlinenr] =~ s/\s*$level\s*//;
>                         }
>                 }
> 
> Looking at the line from above that triggers the warning,
> 
> 	#define nn_err(nn, fmt, args...)   nn_pr(nn, KERN_ERR, fmt, ## args)
> 
> We see that the warning is triggers by the regexp but that it matches 
> the first part on nn_err( and then the second part of on the second 
> argument to nn_pr, KERN_ERR. I believe this to be a false positive.
> 
> Unfortunately my Perl skills are not good enough to fix the check to only 
> look for KERN_[A-Z]+ inside the argument list to the log function name 
> that matches the first part of the regexp.
> 

I would have avoided it and gotten dynamic debug support at the
same time with:
---
 drivers/net/ethernet/netronome/nfp/nfp_net.h | 48 ++++++++++++++--------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
index df203738511bf..46178a7244ad8 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -25,32 +25,32 @@
 
 #include "nfp_net_ctrl.h"
 
-#define nn_pr(nn, lvl, fmt, args...)					\
-	({								\
-		struct nfp_net *__nn = (nn);				\
+#define nn_pr(nn, lvl, fmt, ...)					\
+({									\
+	struct nfp_net *__nn = (nn);					\
 									\
-		if (__nn->dp.netdev)					\
-			netdev_printk(lvl, __nn->dp.netdev, fmt, ## args); \
-		else							\
-			dev_printk(lvl, __nn->dp.dev, "ctrl: " fmt, ## args); \
-	})
-
-#define nn_err(nn, fmt, args...)	nn_pr(nn, KERN_ERR, fmt, ## args)
-#define nn_warn(nn, fmt, args...)	nn_pr(nn, KERN_WARNING, fmt, ## args)
-#define nn_info(nn, fmt, args...)	nn_pr(nn, KERN_INFO, fmt, ## args)
-#define nn_dbg(nn, fmt, args...)	nn_pr(nn, KERN_DEBUG, fmt, ## args)
-
-#define nn_dp_warn(dp, fmt, args...)					\
-	({								\
-		struct nfp_net_dp *__dp = (dp);				\
+	if (__nn->dp.netdev)						\
+		netdev_##lvl(__nn->dp.netdev, fmt, ##__VA_ARGS__);	\
+	else								\
+		dev_##lvl(__nn->dp.dev, "ctrl: " fmt, ##__VA_ARGS__);	\
+})
+
+#define nn_err(nn, fmt, ...)	nn_pr(nn, err, fmt, ##__VA_ARGS__)
+#define nn_warn(nn, fmt, ...)	nn_pr(nn, warn, fmt, ##__VA_ARGS__)
+#define nn_info(nn, fmt, ...)	nn_pr(nn, info, fmt, ##__VA_ARGS__)
+#define nn_dbg(nn, fmt, ...)	nn_pr(nn, dbg, fmt, ##__VA_ARGS__)
+
+#define nn_dp_warn(dp, fmt, ...)					\
+({									\
+	struct nfp_net_dp *__dp = (dp);					\
 									\
-		if (unlikely(net_ratelimit())) {			\
-			if (__dp->netdev)				\
-				netdev_warn(__dp->netdev, fmt, ## args); \
-			else						\
-				dev_warn(__dp->dev, fmt, ## args);	\
-		}							\
-	})
+	if (unlikely(net_ratelimit())) {				\
+		if (__dp->netdev)					\
+			netdev_warn(__dp->netdev, fmt, ##__VA_ARGS__);	\
+		else							\
+			dev_warn(__dp->dev, fmt, ##__VA_ARGS__);	\
+	}								\
+})
 
 /* Max time to wait for NFP to respond on updates (in seconds) */
 #define NFP_NET_POLL_TIMEOUT	5