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

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

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

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?

> +	/* 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)?

> +	{ 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@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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@gmail.com> 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@gmail.com

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

[Richard Weinberger]

On Sat, Oct 19, 2019 at 10:33 AM Konstantin Komarov
<almaz.alexandrovich@paragon-software.com> wrote:
>
> Recently exFAT filesystem specification has been made public by Microsoft (https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification).
> Having decades of expertise in commercial file systems development, we at Paragon Software GmbH are very excited by Microsoft's decision and now want to make our contribution to the Open Source Community by providing our implementation of exFAT Read-Only (yet!) fs implementation for the Linux Kernel.
> We are about to prepare the Read-Write support patch as well.
> 'fs/exfat' is implemented accordingly to standard Linux fs development approach with no use/addition of any custom API's.
> To divide our contribution from 'drivers/staging' submit of Aug'2019, our Kconfig key is "EXFAT_RO_FS"

How is this driver different from the driver in drivers/staging?
With the driver in staging and the upcoming driver from Samsung this
is driver number
three for exfat. ;-\

-- 
Thanks,
//richard

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

[Pali Rohár]

On Sunday 20 October 2019 20:08:20 Richard Weinberger wrote:
> On Sat, Oct 19, 2019 at 10:33 AM Konstantin Komarov
> <almaz.alexandrovich@paragon-software.com> wrote:
> >
> > Recently exFAT filesystem specification has been made public by Microsoft (https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification).
> > Having decades of expertise in commercial file systems development, we at Paragon Software GmbH are very excited by Microsoft's decision and now want to make our contribution to the Open Source Community by providing our implementation of exFAT Read-Only (yet!) fs implementation for the Linux Kernel.
> > We are about to prepare the Read-Write support patch as well.
> > 'fs/exfat' is implemented accordingly to standard Linux fs development approach with no use/addition of any custom API's.
> > To divide our contribution from 'drivers/staging' submit of Aug'2019, our Kconfig key is "EXFAT_RO_FS"
> 
> How is this driver different from the driver in drivers/staging?
> With the driver in staging and the upcoming driver from Samsung this
> is driver number
> three for exfat. ;-\

Hi Richard!

There is vfat+msdos driver for FAT12/16/32 in fs/fat/. Then there is
modified Samsung exfat driver which was recently merged into staging
area and supports FAT12/16/32 and exFAT. Plus there is new version of
this out-of-tree Samsung's exfat driver called sdfat which can be found
in some Android phones. Based on sdfat sources there is out-of-tree
exfat-linux [1] driver which seems to have better performance as
currently merged old modified Samsung's exfat driver into staging. This
list of available exfat drivers is not complete. There is also fuse
implementation widely used [2] and some commercial implementations from
Tuxera [3], Paragon [4], Embedded Access [5] or HCC [6]. As Konstantin
in his email wrote, implementation which he sent should be one used in
their commercial Paragon product.

So we have not 3, but at least 6 open source implementations. Plus more
closed source, commercial.

About that one implementation from Samsung, which was recently merged
into staging tree, more people wrote that code is in horrible state and
probably it should not have been merged. That implementation has
all-one-one driver FAT12, FAT16, FAT32 and exFAT which basically
duplicate current kernel fs/fat code.

Quick look at this Konstantin's patch, it looks like that code is not in
such bad state as staging one. It has only exFAT support (no FAT32) but
there is no write support (yet). For me it looks like that this
Konstantin's implementation is more closer then one in staging to be
"primary" exfat implementation for kernel (if write support would be
provided).

[1] - https://github.com/cryptomilk/kernel-sdfat
[2] - https://github.com/relan/exfat
[3] - https://www.tuxera.com/products/tuxera-exfat-embedded/
[4] - https://www.paragon-software.com/technologies/
[5] - http://embedded-access.com/exfat-file-system/
[6] - https://www.hcc-embedded.com/exfat/

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Maurizio Lombardi]

Dne 21.10.2019 v 12:54 Pali Rohár napsal(a):
> Plus there is new version of
> this out-of-tree Samsung's exfat driver called sdfat which can be found
> in some Android phones. 

[...]

> 
> About that one implementation from Samsung, which was recently merged
> into staging tree, more people wrote that code is in horrible state and
> probably it should not have been merged. That implementation has
> all-one-one driver FAT12, FAT16, FAT32 and exFAT which basically
> duplicate current kernel fs/fat code.
> 
> Quick look at this Konstantin's patch, it looks like that code is not in
> such bad state as staging one. It has only exFAT support (no FAT32) but
> there is no write support (yet).

But, AFAIK, Samsung is preparing a patch that will replace the current
staging driver with their newer sdfat driver that also has write support.

https://marc.info/?l=linux-fsdevel&m=156985252507812&w=2

Maurizio

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

[Pali Rohár]

On Monday 21 October 2019 13:08:07 Maurizio Lombardi wrote:
> Dne 21.10.2019 v 12:54 Pali Rohár napsal(a):
> > Plus there is new version of
> > this out-of-tree Samsung's exfat driver called sdfat which can be found
> > in some Android phones. 
> 
> [...]
> 
> > 
> > About that one implementation from Samsung, which was recently merged
> > into staging tree, more people wrote that code is in horrible state and
> > probably it should not have been merged. That implementation has
> > all-one-one driver FAT12, FAT16, FAT32 and exFAT which basically
> > duplicate current kernel fs/fat code.
> > 
> > Quick look at this Konstantin's patch, it looks like that code is not in
> > such bad state as staging one. It has only exFAT support (no FAT32) but
> > there is no write support (yet).
> 
> But, AFAIK, Samsung is preparing a patch that will replace the current
> staging driver with their newer sdfat driver that also has write support.
> 
> https://marc.info/?l=linux-fsdevel&m=156985252507812&w=2

Maurizio, thank you for reference! I have not caught this Samsung
activity yet! So we now we have +1 for count of exFAT drivers.

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Richard Weinberger]

On Mon, Oct 21, 2019 at 1:13 PM Pali Rohár <pali.rohar@gmail.com> wrote:
> On Monday 21 October 2019 13:08:07 Maurizio Lombardi wrote:
> > Dne 21.10.2019 v 12:54 Pali Rohár napsal(a):
> Maurizio, thank you for reference! I have not caught this Samsung
> activity yet! So we now we have +1 for count of exFAT drivers.

This is how I counted three exfat drivers.
1. staging/exfat (old samsung driver)
2. sdfat (new samsung dirver)
3. paragon read-only

-- 
Thanks,
//richard

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

[Pali Rohár]

On Friday 18 October 2019 15:18:39 Konstantin Komarov wrote:
> Recently exFAT filesystem specification has been made public by Microsoft (https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification).
> Having decades of expertise in commercial file systems development, we at Paragon Software GmbH are very excited by Microsoft's decision and now want to make our contribution to the Open Source Community by providing our implementation of exFAT Read-Only (yet!) fs implementation for the Linux Kernel.
> We are about to prepare the Read-Write support patch as well.

Hi Konstantin! Do you have any plan when you provide also R/W support?

> 'fs/exfat' is implemented accordingly to standard Linux fs development approach with no use/addition of any custom API's.
> To divide our contribution from 'drivers/staging' submit of Aug'2019, our Kconfig key is "EXFAT_RO_FS"
>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
> MAINTAINERS         |    6 +
> fs/Kconfig          |    3 +-
> fs/exfat/Kconfig    |   31 ++
> fs/exfat/Makefile   |    9 +
> fs/exfat/bitmap.c   |  117 +++++
> fs/exfat/cache.c    |  483 ++++++++++++++++++
> fs/exfat/debug.h    |   69 +++
> fs/exfat/dir.c      |  610 +++++++++++++++++++++++
> fs/exfat/exfat.h    |  248 ++++++++++
> fs/exfat/exfat_fs.h |  388 +++++++++++++++
> fs/exfat/fatent.c   |   79 +++
> fs/exfat/file.c     |   93 ++++
> fs/exfat/inode.c    |  317 ++++++++++++
> fs/exfat/namei.c    |  154 ++++++
> fs/exfat/super.c    | 1145 +++++++++++++++++++++++++++++++++++++++++++
> fs/exfat/upcase.c   |  344 +++++++++++++
> 16 files changed, 4095 insertions(+), 1 deletion(-)
> create mode 100644 fs/exfat/Kconfig
> create mode 100644 fs/exfat/Makefile
> create mode 100644 fs/exfat/bitmap.c
> create mode 100644 fs/exfat/cache.c
> create mode 100644 fs/exfat/debug.h
> create mode 100644 fs/exfat/dir.c
> create mode 100644 fs/exfat/exfat.h
> create mode 100644 fs/exfat/exfat_fs.h
> create mode 100644 fs/exfat/fatent.c
> create mode 100644 fs/exfat/file.c
> create mode 100644 fs/exfat/inode.c
> create mode 100644 fs/exfat/namei.c
> create mode 100644 fs/exfat/super.c
> create mode 100644 fs/exfat/upcase.c

Also have you considered to to re-use fs/fat sources instead? It is
possible or there is nothing in fs/fat which could be reused or
refactored/extracted?

> 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
...
> +/* inits internal info from on-disk boot sector*/
> +static int exfat_init_from_boot(struct super_block *sb, struct exfat_boot *boot,
> +				u64 bytes_per_volume, u32 *root_lcn)
> +{
...
> +	if (boot->fats != 1) {
> +		hint = "This version of exfat driver does not support TexFat";
> +		goto out;
> +	}

Are you going to add support also for TexFAT? Or at least for more two
FAT tables (like is used in FAT32)?

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Maurizio Lombardi]

Dne 21.10.2019 v 13:11 Pali Rohár napsal(a):
> Are you going to add support also for TexFAT? Or at least for more two
> FAT tables (like is used in FAT32)?
> 

Just a small note here, differences between FAT and exFAT:

1) Contiguous files get a special treatment by exFAT: they do not use the FAT cluster chain.
2) exFAT doesn't use the FAT to track free space, it uses a bitmap.

So, 2 FAT tables are probably not sufficient for recovery, 2 bitmaps are needed too.[1]
Btw, only Windows CE supported this.

[1] http://www.ntfs.com/exfat-allocation-bitmap.htm

Maurizio

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

[Pali Rohár]

On Monday 21 October 2019 13:37:13 Maurizio Lombardi wrote:
> So, 2 FAT tables are probably not sufficient for recovery, 2 bitmaps are needed too.

Yes, I know. But code which I referred check both number of fat tables
and number of allocation bitmaps (as they are represented by one member
in boot sector structure).

> Btw, only Windows CE supported this.

Is this information based on some real tests? Or just from marketing or
Microsoft's information? (I would really like to know definite answer in
this area).

Because Microsoft says one thing in their FAT32 specification, second
thing described in their FAT implementation and thing thing is how it is
really implemented (in fatfast.sys kernel driver which is open source).

So I would be really careful about how MS's exfat.sys implementation is
working.

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Maurizio Lombardi]

Dne 21.10.2019 v 13:45 Pali Rohár napsal(a):
> They are represented by one member
> in boot sector structure).
> 
>> Btw, only Windows CE supported this.
> 
> Is this information based on some real tests? Or just from marketing or
> Microsoft's information? (I would really like to know definite answer in
> this area).

I admit I have read it on Microsoft's exFAT documentation, unfortunately
I don't have a WinCE device to test if it's really true.


Maurizio