linux-kernel - Re: [PATCH v1] checkpatch: add missing bindings license check

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] [day] [month] [year] [list]
Date:   Mon, 20 Mar 2023 11:31:49 +0300
From:   Dmitry Rokosov <ddrokosov@...rdevices.ru>
To:     Joe Perches <joe@...ches.com>
CC:     <krzysztof.kozlowski@...aro.org>, <robh@...nel.org>,
        <apw@...onical.com>, <dwaipayanray1@...il.com>,
        <lukas.bulwahn@...il.com>, <kernel@...rdevices.ru>,
        <linux-kernel@...r.kernel.org>, <rockosov@...il.com>
Subject: Re: [PATCH v1] checkpatch: add missing bindings license check

Hello Joe,

Thank you for quick review. Please find my comments below.

On Fri, Mar 17, 2023 at 03:28:16PM -0700, Joe Perches wrote:
> On Fri, 2023-03-17 at 23:16 +0300, Dmitry Rokosov wrote:
> > All headers from 'include/dt-bindings/' must be verified by checkpatch
> > together with Documentation bindings, because all of them are part of
> > the whole DT bindings system.
> > 
> > The requirement is dual licensed and matching string:
> >     'GPL-2.0-only OR BSD-2-Clause'
> > 
> > The issue was found during patch review:
> > https://lore.kernel.org/all/20230313201259.19998-4-ddrokosov@sberdevices.ru/
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@...rdevices.ru>
> > ---
> >  scripts/checkpatch.pl | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3709,7 +3709,8 @@ sub process {
> >  						WARN("SPDX_LICENSE_TAG",
> >  						     "'$spdx_license' is not supported in LICENSES/...\n" . $herecurr);
> >  					}
> > -					if ($realfile =~ m@...cumentation/devicetree/bindings/@ &&
> > +					if (($realfile =~ m@...cumentation/devicetree/bindings/@ ||
> > +					    $realfile =~ m@...clude/dt-bindings/@) &&
> 
> I prefer aligning to open parens
> 
> >  					    not $spdx_license =~ /GPL-2\.0.*BSD-2-Clause/) {

Okay, no problem, will send new version today.
> 
> And if it's really a strict bit about the required license,
> why not make it match exactly?
> 
>  					    $spdx_license !~ /GPL-2\.0(?:-only|-or-later|\+)? OR BSD-2-Clause/) {
> 

I think, it's a good idea.

> >  						my $msg_level = \&WARN;
> >  						$msg_level = \&CHK if ($file);
> 
> $ git grep -oh 'SPDX-License.*$' -- Documentation/devicetree/bindings/ include/dt-bindings/ | \
>   sort | uniq -c | sort -rn
>    1597 SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>     611 SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>     540 SPDX-License-Identifier: GPL-2.0
>     355 SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>     285 SPDX-License-Identifier: GPL-2.0 */
>     179 SPDX-License-Identifier: GPL-2.0-only */
>     102 SPDX-License-Identifier: GPL-2.0-only
>      93 SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>      56 SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>      47 SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>      36 SPDX-License-Identifier: GPL-2.0+ */
>      34 SPDX-License-Identifier: GPL-2.0-or-later */
>      33 SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>      28 SPDX-License-Identifier: GPL-2.0+
>      21 SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>      19 SPDX-License-Identifier: (GPL-2.0+ or MIT) */
>      17 SPDX-License-Identifier: (GPL-2.0 OR MIT) */
>      12 SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
>      11 SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>       9 SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>       8 SPDX-License-Identifier: GPL-2.0 OR MIT */
>       8 SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
>       7 SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause
>       7 SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>       7 SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
>       6 SPDX-License-Identifier: (GPL-2.0)
>       5 SPDX-License-Identifier: GPL-2.0+ OR MIT */
>       5 SPDX-License-Identifier: (GPL-2.0 OR MIT)
>       5 SPDX-License-Identifier: (GPL-2.0 or MIT) */
>       4 SPDX-License-Identifier: GPL-2.0-or-later
>       3 SPDX-License-Identifier: (GPL-2.0+ OR X11)
>       3 SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
>       3 SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>       3 SPDX-License-Identifier: GPL-2.0 or BSD-3-Clause */
>       3 SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
>       3 SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */
>       3 SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) */
>       2 SPDX-License-Identifier: (GPL-2.0+ or MIT)
>       2 SPDX-License-Identifier: GPL-2.0-or-later OR MIT */
>       2 SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
>       2 SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */
>       2 SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)*/
>       1 SPDX-License-Identifier: (GPL-2.0-or-later OR MIT) */
>       1 SPDX-License-Identifier: (GPL-2.0-or-later or MIT) */
>       1 SPDX-License-Identifier: GPL-2.0-or-later or BSD-2-Clause */
>       1 SPDX-License-Identifier: (GPL-2.0-or-later)
>       1 SPDX-License-Identifier: GPL-2.0+ or BSD-3-Clause */
>       1 SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
>       1 SPDX-License-Identifier: GPL-2.0-only or X11 */
>       1 SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>       1 SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause */
>       1 SPDX-License-Identifier: (GPL-2.0-only)
>       1 SPDX-License-Identifier: BSD-2-Clause
> 

I've noticed it too, asked Krzysztof about this situation in the below
review:

https://lore.kernel.org/all/9d176288-cd7c-7107-e180-761e372a2b6e@linaro.org/

Krzysztof mentioned, that he checked the purpose to have different
license each time manually. But by default, it should be strict.

-- 
Thank you,
Dmitry