Re: [PATCH v5 08/10] fs/ntfs3: Add Kconfig, Makefile and doc

[Pali Rohár]

On Friday 09 October 2020 15:31:10 Konstantin Komarov wrote:
> From: Pali Rohár <pali@xxxxxxxxxx>
> Sent: Saturday, September 26, 2020 11:23 AM
> > To: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> > Cc: linux-fsdevel@xxxxxxxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dsterba@xxxxxxx; aaptel@xxxxxxxx;
> > willy@xxxxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx; joe@xxxxxxxxxxx; mark@xxxxxxxxxxxxx; nborisov@xxxxxxxx
> > Subject: Re: [PATCH v5 08/10] fs/ntfs3: Add Kconfig, Makefile and doc
> > 
> > On Friday 25 September 2020 16:30:19 Konstantin Komarov wrote:
> > > From: Pali Rohár <pali@xxxxxxxxxx>
> > > Sent: Monday, September 21, 2020 4:27 PM
> > > > To: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> > > > Cc: linux-fsdevel@xxxxxxxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dsterba@xxxxxxx; aaptel@xxxxxxxx;
> > > > willy@xxxxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx; joe@xxxxxxxxxxx; mark@xxxxxxxxxxxxx; nborisov@xxxxxxxx
> > > > Subject: Re: [PATCH v5 08/10] fs/ntfs3: Add Kconfig, Makefile and doc
> > > >
> > > > On Friday 11 September 2020 17:10:16 Konstantin Komarov wrote:
> > > > > +Mount Options
> > > > > +=============
> > > > > +
> > > > > +The list below describes mount options supported by NTFS3 driver in addition to
> > > > > +generic ones.
> > > > > +
> > > > > +===============================================================================
> > > > > +
> > > > > +nls=name		This option informs the driver how to interpret path
> > > > > +			strings and translate them to Unicode and back. If
> > > > > +			this option is not set, the default codepage will be
> > > > > +			used (CONFIG_NLS_DEFAULT).
> > > > > +			Examples:
> > > > > +				'nls=utf8'
> > > > > +
> > > > > +nls_alt=name		This option extends "nls". It will be used to translate
> > > > > +			path string to Unicode if primary nls failed.
> > > > > +			Examples:
> > > > > +				'nls_alt=cp1251'
> > > >
> > > > Hello! I'm looking at other filesystem drivers and no other with UNICODE
> > > > semantic (vfat, udf, isofs) has something like nls_alt option.
> > > >
> > > > So do we really need it? And if yes, it should be added to all other
> > > > UNICODE filesystem drivers for consistency.
> > > >
> > > > But I'm very sceptical if such thing is really needed. nls= option just
> > > > said how to convert UNICODE code points for userpace. This option is
> > > > passed by userspace (when mounting disk), so userspace already know what
> > > > it wanted. And it should really use this encoding for filenames (e.g.
> > > > utf8 or cp1251) which already told to kernel.
> > >
> > > Hi Pali! Thanks for the feedback. We do not consider the nls_alt option as the must have
> > > one. But it is very nice "QOL-type" mount option, which may help some amount of
> > > dual-booters/Windows users to avoid tricky fails with files originated on non-English
> > > Windows systems. One of the cases where this one may be useful is the case of zipping
> > > files with non-English names (e.g. Polish etc) under Windows and then unzipping the archive
> > > under Linux. In this case unzip will likely to fail on those files, as archive stores filenames not
> > > in utf.
> > 
> > Hello!
> > 
> > Thank you for providing example. Now I can imagine the problem which
> > this option is trying to "workaround".
> > 
> > Personally, I think that this is the issue of the program which is
> > unzipping content of the archive. If files are in archive are stored in
> > different encoding, then user needs to provide information in which it
> > is stored. Otherwise it would be broken.
> > 
> 
> Hi Pali! Partially it is the issue of the program. But such issue may affect
> a lot of programs, esp. given that this case is somehwat niche for the Linux,
> because it origins in Windows. We may assume it is unlikely a lot of programs
> will try/are trying to support this case. The mount option, on the other hand,
> gives this ability without relying on the application itself.

Hello! I understand this issue. But what you have described is basically
filesystem independent, this may happen also on ext4 with UNICODE
support and also on fat32 with VFAT support.

Therefore I do not think it is good idea to have e.g. nls_alt=cp1251
option directly in just one filesystem driver.

This would just make ntfs driver inconsistent with other Linux UNICODE
filesystem drivers and it would cause more problems that application
unzip is working with ntfs driver, but does not work with vfat or ext4
driver.

I would really suggest to not include this nls_alt option into
filesystem driver. And rather come up with some universal solution for
all UNICODE filesystem drivers. There are multiple solutions, e.g.
implement option in VFS layer and propagate it to UNICODE fs drivers, OR
implement NLS codepage which would handle it...

Inconsistency of one FS driver with all other would really cause
problems if not now, then in future.

This issue which you described is already there, also without ntfs
driver. And still adding something for ntfs driver looks like a
workaround or hack for that issue. Not a solution.

> > Also this your approach with nls=utf-8 and nls_alt=cp1251 is broken. I
> > can provide you string encoded in cp1251 which is also valid UTF-8
> > sequence.
> > 
> > For example: sequence of bytes "d0 93".
> > 
> > In cp1251 it is Р“, but also it is valid UTF-8 sequence for Г (CYRILLIC
> > CAPITAL LETTER GHE).
> > 
> > Because cp1251 is set as nls_alt, you would get UTF-8 interpretation.
> > And for all other invalid UTF-8 sequences you would get cp1251.
> > 
> > For me it looks like you are trying to implement workaround based on
> > some heuristic in kernel for userspace application which handles
> > encoding incorrectly. And because all CP???? encodings are defined at
> > full 8bit space and UTF-8 is subset of 8bit space, it would never work
> > correctly.
> > 
> 
> In this case, the whole string will be treated as UTF-8 string, no matter of
> what's the value of "nls_alt" option. The option's related logic starts to be applied
> only for those strings which contain "invalid" UTF-8 character. And it works not in
> symbol-by-symbol way, but applies to the whole string. So if a string does not
> contain invalid UTF-8, the "nls_alt" won't be applied, even if set. And if the string
> contains invalid UTF-8 AND "nls_alt" is set, then the logic will be applied with assumption
> that user, when set the "nls_alt" had in mind that he may be in such situation.
> 
> When it is coming to valid UTF-8 sequences, which are actually meant to represent another
> encoding, there is ambiguity, which seems unable to be resolved both with and without
> the "nls_alt". But at least those cases, when the sequence is incorrect UTF-8, but correct
> e.g. CP-1251, may be solved with the "nls_alt", but not without it. 
> 
> It is not covering all the cases, but covers at least those, which otherwise lead to inability
> operating with the file (e.g. error during unzip). Also it is set only explicitly by the user.
> 
> We'll follow the community opinion in our implementation, just want to make sure the
> solution is understood correctly. Regarding the "nls_alt", it doesn't seem
> to be harmful in any way, but may help some amount of users to overcome interoperability
> issues.
> 
> > Also I do not think that kernel is correct place for workarounding
> > userspace applications which handles encoding incorrectly.
> > 
> > > Windows have that "Language for non-Unicode programs" setting, which controls the
> > > encoding used for the described (and similar) cases.
> > 
> > This windows setting is something different. It is system wide option
> > which affects -A WINAPI functions and defines one fixed 8bit encoding
> > (ACP) which should be used for converting UTF-16 strings (wchar_t*) into
> > 8bit (char*) ACP encoding.
> > 
> > It is something similar to Unix CODESET set in LC_CTYPE from locale. But
> > not the same.
> > 
> > > Overall, it's kinda niche mount option, but we suppose it's legit for Windows-originated filesystems.
> > > What do you think on this, Pali?
> > 
> > I think this is not only for Windows-orientated FS, but rather for all
> > filesystems which store filenames in UNICODE (as opposite of sequence of
> > bytes).
> > 
> > E.g. ext4 now has extension for storing (and validating) that filenames
> > are also in UNICODE (on disk it is in UTF-8).
> > 
> > Same for Beos FS or UDF fs (on DVD/BD-R). In most cases these fs are
> > mounted with nls=utf-8 to interpret UNICODE as utf-8.
> > 
> > And none of these fs have such nls_alt option as I show above, it cannot
> > work reliable.
> 
> Thanks.