linux-kernel - Re: [PATCH] checkpatch: add new warning when lookup_symbol

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <f650d87a5c65e3da44a129297c3254b7da48767c.camel@perches.com>
Date:   Thu, 17 Dec 2020 10:15:32 -0800
From:   Joe Perches <joe@...ches.com>
To:     Helge Deller <deller@....de>, Andy Whitcroft <apw@...onical.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] checkpatch: add new warning when lookup_symbol_name()
 is used

On Thu, 2020-12-17 at 18:42 +0100, Helge Deller wrote:
> On 12/17/20 6:27 PM, Joe Perches wrote:
> > On Thu, 2020-12-17 at 18:11 +0100, Helge Deller wrote:
> > > In most cases people use lookup_symbol_name() to resolve a kernel symbol
> > > and then print it via printk().
> > > 
> > > In such cases using the %ps, %pS, %pSR or %pB printk formats are easier
> > > to use and thus should be preferred.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -4317,6 +4317,12 @@ sub process {
> > >  			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
> > >  		}
> > > 
> > > +# avoid lookup_symbol_name()
> > > +		if ($line =~ /\blookup_symbol_name\b/) {
> > > +			WARN("PREFER_PRINTK_FORMAT",
> > > +			     "If possible prefer %ps or %pS printk format string to print symbol name instead of using lookup_symbol_name()\n" . $herecurr);
> > > +		}
> > > +
> > >  # check for uses of printk_ratelimit
> > >  		if ($line =~ /\bprintk_ratelimit\s*\(/) {
> > >  			WARN("PRINTK_RATELIMITED",
> > 
> > Huh?  nak.
> > 
> > lookup_symbol_name is used in the kernel a grand total of 3 times.
> 
> Yes, there were much more in the past which got fixed by patches I submitted.

Hi Helge.

Much more may be a bit of an overstatement.

I found 3 instances of lookup_symbol_name removals in 2 patches.

commit 36dbca148bf8e3b8658982aa2256bdc7ef040256
-		lookup_symbol_name((ulong)pm_power_off, symname);
-		lookup_symbol_name((ulong)pm_power_off, symname);
commit da88f9b3113620dcd30fc203236aa53d5430ee98
-	if (lookup_symbol_name((unsigned long)sym, symname) < 0)

There's a tension between adding tests and newbies that consider
checkpatch warnings as dicta that must be followed so there would
be patches submitted eventually against the existing correct uses.

So thanks, but given the very few existing all correct uses of
this function and the low probability of new uses I'd prefer not
to apply this.