linux-kernel - Re: [PATCH v2 0/4] dedupe smb unicode files

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: <d1f7fbe9-8fe2-e3e3-d6ff-1544204202ff@talpey.com>
Date:   Thu, 20 Jul 2023 10:25:38 -0400
From:   Tom Talpey <tom@...pey.com>
To:     Dave Kleikamp <dave.kleikamp@...cle.com>,
        "Dr. David Alan Gilbert" <linux@...blig.org>,
        Steve French <smfrench@...il.com>
Cc:     linkinjeon@...nel.org, shaggy@...nel.org,
        linux-cifs@...r.kernel.org, krisman@...labora.com,
        jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] dedupe smb unicode files

On 7/19/2023 6:06 PM, Dave Kleikamp wrote:
> On 7/19/23 4:58PM, Dr. David Alan Gilbert wrote:
>> * Steve French (smfrench@...il.com) wrote:
>>> The related question is which tree to send it from, if no problems
>>> reported (presumably mine since it mostly affect cifs.ko and ksmbd.ko,
>>> and because there hasn't been activity in fs/nls for years)
>>
>> That was my hope, given that ~half of the patches are directly on that
>> code, and it's the only very active tree this touches as far as I can
>> tell.
>>
>>> On Wed, Jul 19, 2023 at 12:56 PM Steve French <smfrench@...il.com> 
>>> wrote:
>>>>
>>>> No objections to this on my part.  If Shaggy is ok with the JFS
>>>> change, we could target it for 6.6-rc1 if it tests out ok
> 
> For the series:
> Reviewed-by: Dave Kleikamp <dave.kleikamp@...cle.com>
> 
> Steve,
> Feel free to pull in even the 4th patch into your tree with my consent. 
> Or if you're more comfortable, I could submit it after yours hits mainline.
> 
> Shaggy

The changes look good to me but there is one quirk with the
copyrights and SPDX in patch 2.

In the new fs/nls/nls_ucs2_utils.c, the SPDX line changes from
a "/* ... */" form to "// ...", which may be a proper update, but
then partway down, adds the same SPDX in "/* ... */ form. These
should at least be consistent.

> +++ b/fs/nls/nls_ucs2_utils.c
> @@ -1,19 +1,25 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> +// SPDX-License-Identifier: GPL-2.0-or-later

vs

> +++ b/fs/nls/nls_ucs2_utils.h
> @@ -0,0 +1,297 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

Second, the copyright in fs/nls/nls_ucs2_utils.c is a bit of
a mash-up (adding 2009 especially).

I think it's better to keep the exact text of both copyrights,
perhaps with a note as to which files had them previously, and
adding some new note/blank line to separate the recent contributions
from Namjae and you from the ancient history.

> +++ b/fs/nls/nls_ucs2_utils.c
> ...
> - *   Some of the source code in this file came from fs/cifs/uniupr.h
> - *   Copyright (c) International Business Machines  Corp., 2000,2002
> - *
> - * uniupr.h - Unicode compressed case ranges
> + *   Some of the source code in this file came from fs/cifs/cifs_unicode.c
> + *   via fs/smb/unicode.c and fs/smb/uniupr.h and fs/cifs/uniupr.h
> + *   Copyright (c) International Business Machines  Corp., 2000,2002,2009
> + *   Modified by Steve French (sfrench@...ibm.com)
> + *   Modified by Namjae Jeon (linkinjeon@...nel.org)
> + *   Modified by Dr. David Alan Gilbert <linux@...blig.org>

Apart from considering these:

Reviewed-by: Tom Talpey <tom@...pey.com>

Nice work!

>>
>> Thanks.
>>
>> Dave
>>
>>>> On Wed, Jul 12, 2023 at 6:28 PM Dr. David Alan Gilbert 
>>>> <dave@...blig.org> wrote:
>>>>>
>>>>> * linux@...blig.org (linux@...blig.org) wrote:
>>>>>> From: "Dr. David Alan Gilbert" <linux@...blig.org>
>>>>>>
>>>>>> The smb client and server code have (mostly) duplicated code
>>>>>> for unicode manipulation, in particular upper case handling.
>>>>>>
>>>>>> Flatten this lot into shared code.
>>>>>
>>>>> Gentle two week ping on this please.
>>>>>
>>>>> Dave
>>>>>
>>>>> (Apologies to the 3 of you who already got a copy of this ping,
>>>>> recent due to a missing header ',' )
>>>>>
>>>>>> There's some code that's slightly different between the two, and
>>>>>> I've not attempted to share that - this should be strictly a no
>>>>>> behaviour change set.
>>>>>>
>>>>>> In addition, the same tables and code are shared in jfs, however
>>>>>> there's very little testing available for the unicode in there,
>>>>>> so just share the raw data tables.
>>>>>>
>>>>>> I suspect there's more UCS-2 code that can be shared, in the NLS code
>>>>>> and in the UCS-2 code used by the EFI interfaces.
>>>>>>
>>>>>> Lightly tested with a module and a monolithic build, and just 
>>>>>> mounting
>>>>>> itself.
>>>>>>
>>>>>> This dupe was found using PMD:
>>>>>>    https://pmd.github.io/pmd/pmd_userdocs_cpd.html
>>>>>>
>>>>>> Dave
>>>>>>
>>>>>> Version 2
>>>>>>    Moved the shared code to fs/nls after v1 feedback.
>>>>>>    Renamed shared tables from Smb to Nls prefix
>>>>>>    Move UniStrcat as well
>>>>>>    Share the JFS tables
>>>>>>
>>>>>> Dr. David Alan Gilbert (4):
>>>>>>    fs/smb: Remove unicode 'lower' tables
>>>>>>    fs/smb: Swing unicode common code from smb->NLS
>>>>>>    fs/smb/client: Use common code in client
>>>>>>    fs/jfs: Use common ucs2 upper case table
>>>>>>
>>>>>>   fs/jfs/Kconfig               |   1 +
>>>>>>   fs/jfs/Makefile              |   2 +-
>>>>>>   fs/jfs/jfs_unicode.h         |  17 +-
>>>>>>   fs/jfs/jfs_uniupr.c          | 121 -------------
>>>>>>   fs/nls/Kconfig               |   8 +
>>>>>>   fs/nls/Makefile              |   1 +
>>>>>>   fs/nls/nls_ucs2_data.h       |  15 ++
>>>>>>   fs/nls/nls_ucs2_utils.c      | 144 +++++++++++++++
>>>>>>   fs/nls/nls_ucs2_utils.h      | 285 ++++++++++++++++++++++++++++++
>>>>>>   fs/smb/client/Kconfig        |   1 +
>>>>>>   fs/smb/client/cifs_unicode.c |   1 -
>>>>>>   fs/smb/client/cifs_unicode.h | 330 
>>>>>> +----------------------------------
>>>>>>   fs/smb/client/cifs_uniupr.h  | 239 -------------------------
>>>>>>   fs/smb/server/Kconfig        |   1 +
>>>>>>   fs/smb/server/unicode.c      |   1 -
>>>>>>   fs/smb/server/unicode.h      | 325 
>>>>>> +---------------------------------
>>>>>>   fs/smb/server/uniupr.h       | 268 ----------------------------
>>>>>>   17 files changed, 467 insertions(+), 1293 deletions(-)
>>>>>>   delete mode 100644 fs/jfs/jfs_uniupr.c
>>>>>>   create mode 100644 fs/nls/nls_ucs2_data.h
>>>>>>   create mode 100644 fs/nls/nls_ucs2_utils.c
>>>>>>   create mode 100644 fs/nls/nls_ucs2_utils.h
>>>>>>   delete mode 100644 fs/smb/client/cifs_uniupr.h
>>>>>>   delete mode 100644 fs/smb/server/uniupr.h
>>>>>>
>>>>>> -- 
>>>>>> 2.41.0
>>>>>>
>>>>> -- 
>>>>>   -----Open up your eyes, open up your mind, open up your code -------
>>>>> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
>>>>> \        dave @ treblig.org |                               | In Hex /
>>>>>   \ _________________________|_____ http://www.treblig.org   |_______/
>>>>
>>>>
>>>>
>>>> -- 
>>>> Thanks,
>>>>
>>>> Steve
>>>
>>>
>>>
>>> -- 
>>> Thanks,
>>>
>>> Steve
>