Re: [PATCH] exFAT read-only driver GPL implementation by Paragon Software.

[Konstantin Komarov]

Hello! 
Thanks for your findings.

> On 20 Oct 2019, at 02:34, Pali RohÃr <pali.rohar@xxxxxxxxx> wrote:
> 
> Hello! I have not read deeply whole implementation, just spotted
> suspicious options. See below.
> 
> On Friday 18 October 2019 15:18:39 Konstantin Komarov wrote:
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> new file mode 100644
>> index 000000000000..5f8713fe1b0c
>> --- /dev/null
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -0,0 +1,388 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  linux/fs/exfat/super.c
>> + *
>> + * Copyright (c) 2010-2019 Paragon Software GmbH, All rights reserved.
>> + *
>> + */
>> +
>> +#include <linux/buffer_head.h>
>> +#include <linux/hash.h>
>> +#include <linux/nls.h>
>> +#include <linux/ratelimit.h>
>> +
>> +struct exfat_mount_options {
>> +	kuid_t fs_uid;
>> +	kgid_t fs_gid;
>> +	u16 fs_fmask;
>> +	u16 fs_dmask;
>> +	u16 codepage; /* Codepage for shortname conversions */
> 
> According to exFAT specification, section 7.7.3 FileName Field there is
> no 8.3 shortname support with DOS/OEM codepage.
> 
> https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#773-filename-field
> 
> Plus it looks like that this member codepage is only set and never
> accessed in whole driver.
> 
> So it can be clean it up and removed?
Yes, this one should be removed as not used actually through the code. One thing to do about the implementation, besides RW support and a little clean-up of such places like this one, is nls/codepage support. Currently, utf-8 only is supported.

> 
>> +	/* minutes bias= UTC - local time. Eastern time zone: +300, */
>> +	/*Paris,Berlin: -60, Moscow: -180*/
>> +	int bias;
>> +	u16 allow_utime; /* permission for setting the [am]time */
>> +	unsigned quiet : 1, /* set = fake successful chmods and chowns */
>> +		showexec : 1, /* set = only set x bit for com/exe/bat */
>> +		sys_immutable : 1, /* set = system files are immutable */
>> +		utf8 : 1, /* Use of UTF-8 character set (Default) */
>> +		/* create escape sequences for unhandled Unicode */
>> +		unicode_xlate : 1, flush : 1, /* write things quickly */
>> +		tz_set : 1, /* Filesystem timestamps' offset set */
>> +		discard : 1 /* Issue discard requests on deletions */
>> +		;
>> +};
> 
> ...
> 
>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>> new file mode 100644
>> index 000000000000..0705dab3c3fc
>> --- /dev/null
>> +++ b/fs/exfat/super.c
> ...
>> +enum {
>> +	Opt_uid, Opt_gid, Opt_umask, Opt_dmask, Opt_fmask, Opt_allow_utime,
>> +	Opt_codepage, Opt_quiet, Opt_showexec, Opt_debug, Opt_immutable,
>> +	Opt_utf8_no, Opt_utf8_yes, Opt_uni_xl_no, Opt_uni_xl_yes, Opt_flush,
>> +	Opt_tz_utc, Opt_discard, Opt_nfs, Opt_bias, Opt_err,
>> +};
>> +
>> +static const match_table_t fat_tokens = {
>> +	{ Opt_uid, "uid=%u" },
>> +	{ Opt_gid, "gid=%u" },
>> +	{ Opt_umask, "umask=%o" },
>> +	{ Opt_dmask, "dmask=%o" },
>> +	{ Opt_fmask, "fmask=%o" },
>> +	{ Opt_allow_utime, "allow_utime=%o" },
>> +	{ Opt_codepage, "codepage=%u" },
>> +	{ Opt_quiet, "quiet" },
>> +	{ Opt_showexec, "showexec" },
>> +	{ Opt_debug, "debug" },
>> +	{ Opt_immutable, "sys_immutable" },
>> +	{ Opt_flush, "flush" },
>> +	{ Opt_tz_utc, "tz=UTC" },
>> +	{ Opt_bias, "bias=%d" },
>> +	{ Opt_discard, "discard" },
>> +	{ Opt_utf8_no, "utf8=0" }, /* 0 or no or false */
>> +	{ Opt_utf8_no, "utf8=no" },
>> +	{ Opt_utf8_no, "utf8=false" },
>> +	{ Opt_utf8_yes, "utf8=1" }, /* empty or 1 or yes or true */
>> +	{ Opt_utf8_yes, "utf8=yes" },
>> +	{ Opt_utf8_yes, "utf8=true" },
>> +	{ Opt_utf8_yes, "utf8" },
> 
> There are lot of utf8 mount options. Are they really needed?
> 
> Would not it be better to use just one "iocharset" mount option like
> other Unicode based filesystem have it (e.g. vfat, jfs, iso9660, udf or
> ntfs)?

It seems reasonable as well. "iocharset" may be more handy way to handle such mount options.

> 
>> +	{ Opt_uni_xl_no, "uni_xlate=0" }, /* 0 or no or false */
>> +	{ Opt_uni_xl_no, "uni_xlate=no" },
>> +	{ Opt_uni_xl_no, "uni_xlate=false" },
>> +	{ Opt_uni_xl_yes, "uni_xlate=1" }, /* empty or 1 or yes or true */
>> +	{ Opt_uni_xl_yes, "uni_xlate=yes" },
>> +	{ Opt_uni_xl_yes, "uni_xlate=true" },
>> +	{ Opt_uni_xl_yes, "uni_xlate" },
>> +	{ Opt_err, NULL }
>> +};
> 
> -- 
> Pali RohÃr
> pali.rohar@xxxxxxxxx