[PATCH] Added CONFIG_VFAT_FS_DUALNAMES option

[Jamie Lokier]

Jan Engelhardt wrote:

>
> On Sunday 2009-06-28 23:57, Jamie Lokier wrote:
> >For those of us who don't want to omit the code, because we like
> >compatibility and we're not in affected countries, or for research, it
> >would be useful to have it as a mount option.
> >
> >So there should be these compile-time options:
> >    2. CONFIG_VFAT_FS_DUALNAMES enabled: Create shortnames, unless
> >       mount option "dualnames=no" is given, in which case that mount
> >       behaves as if CONFIG_VFAT_FS_DUALNAMES is disabled.
>
> If you are not in an affected country, why would you even want
> to disable dualnames? [ on a per-mount basis...]

   1. To test it's behaviour, without changing the whole running system.

   2. To produce disk images which are the same as would be produced
      in an affected country, or would be produced by a Linux distro
      adopting the behaviour.

-- Jamie

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Jamie,

 > For those of us who don't want to omit the code, because we like
 > compatibility and we're not in affected countries, or for research, it
 > would be useful to have it as a mount option.

For research I think having to recompile the module is not a big
burden.

The compatibility argument is more debatable, and if the patch
involved the loss of a large degree of functionality then I would
probably agree. As it is, the patch loses very little functionality -
it is hard to find real scenarios where not storing a 8.3 name along
with the long name will actually cause any problems.

I also don't think it is worth having a different kernel in different
countries for this change, again because the loss of functionality is
so small. The relevant patents exist in several countries (in various
forms), and while the US might be the country of choice for enforcing
patents it is not the only country where patents are a concern.

I would rather see us err on the side of caution and ensure that
Microsoft is not left with an opportunity to continue to spread FUD
about Linux with regards to these patents.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Hirofumi-san,

 > If you want, I have no problem to do. However, I'm not thinking that
 > part is a bug.  And -stable rule is also "a real bug that bothers
 > people", but there is even a no bug reporter which tell actual problem.

ok, then what about pushing the whole patch into -stable?

The 'bug' in this case is like a security hole, with Microsoft
demonstrating an active exploit. This falls outside the normal range
of bug fixes for -stable, but I think you'd have to agree that the
whole situation is unusual.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Greg KH]

On Mon, Jun 29, 2009 at 11:30:39AM +1000, tridge@... wrote:

> Hi Hirofumi-san,
>
>  > If you want, I have no problem to do. However, I'm not thinking that
>  > part is a bug.  And -stable rule is also "a real bug that bothers
>  > people", but there is even a no bug reporter which tell actual problem.
>
> ok, then what about pushing the whole patch into -stable?
>
> The 'bug' in this case is like a security hole, with Microsoft
> demonstrating an active exploit. This falls outside the normal range
> of bug fixes for -stable, but I think you'd have to agree that the
> whole situation is unusual.

I have no objection to take this patch for the -stable releases when it
goes into Linus's tree.  Just forward the git commit ids to
stable@... and I can take care of it.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 > I have no objection to take this patch for the -stable releases when it
 > goes into Linus's tree.  Just forward the git commit ids to
 > stable@... and I can take care of it.

ok, thanks Greg.

Hirofumi-san tells me we've missed the merge window for 2.6.31, so it
seems that this will go first into the fatfs tree, then go into
Linus's tree via -next for 2.6.32. So hopefully the patch will get
some more testing (perhaps in -mm?) before it needs to be considered
for -stable.

Meanwhile some of the distros and other vendors might decide to apply
it sooner depending on their level of legal concern.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Greg KH]

On Tue, Jun 30, 2009 at 08:42:42AM +1000, tridge@... wrote:

>  > I have no objection to take this patch for the -stable releases when it
>  > goes into Linus's tree.  Just forward the git commit ids to
>  > stable@... and I can take care of it.
>
> ok, thanks Greg.
>
> Hirofumi-san tells me we've missed the merge window for 2.6.31, so it
> seems that this will go first into the fatfs tree, then go into
> Linus's tree via -next for 2.6.32. So hopefully the patch will get
> some more testing (perhaps in -mm?) before it needs to be considered
> for -stable.

That sounds reasonable.  Just add a:
        Cc: stable <stable@...>
in the "signed-off-by:" area of the patch, and when it goes into Linus's
tree, the -stable team will be automatically notified of it and we will
pick it up.

It needs to be in Linus's tree before we can add it to the -stable
releases.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[OGAWA Hirofumi]

tridge@... writes:

>  > I have no objection to take this patch for the -stable releases when it
>  > goes into Linus's tree.  Just forward the git commit ids to
>  > stable@... and I can take care of it.
>
> ok, thanks Greg.
>
> Hirofumi-san tells me we've missed the merge window for 2.6.31, so it
> seems that this will go first into the fatfs tree, then go into
> Linus's tree via -next for 2.6.32. So hopefully the patch will get
> some more testing (perhaps in -mm?) before it needs to be considered
> for -stable.

Yes. And I will not drop this patch even if it's not perfect on test. I
can understand it would not be the option for some users. The default
may be arguable though, like I said at first.

My choice is to provide the option of those to users.

> Meanwhile some of the distros and other vendors might decide to apply
> it sooner depending on their level of legal concern.

Yes, it's good.

Thanks.
--
OGAWA Hirofumi <hirofumi@...>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hirofumi-san writes:
 > Yes. And I will not drop this patch even if it's not perfect on test. I
 > can understand it would not be the option for some users.

ok, good

 > The default may be arguable though, like I said at first.

I suggest we leave the default as "no dual names" for now to let it
get some more testing, then if any problems come up then we can look
at the choice of default again. If no problems come up we can keep the
default as it is in the patch now.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[OGAWA Hirofumi]

tridge@... writes:

>  > The default may be arguable though, like I said at first.
>
> I suggest we leave the default as "no dual names" for now to let it
> get some more testing, then if any problems come up then we can look
> at the choice of default again. If no problems come up we can keep the
> default as it is in the patch now.

Ok, I've applied the patch + Cc: to fatfs-2.6.git.
--
OGAWA Hirofumi <hirofumi@...>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Pavel Machek]

> When VFAT_FS_DUALNAMES is disabled we avoid the creation of 8.3 short
> filenames for files on VFAT filesystems that require a long name. The
> patch uses a pattern of 11 bytes in the directory entry which contains
> invalid characters such that it cannot be considered to be a valid short
> filename.
>
> Signed-off-by: Andrew Tridgell <tridge@...>
> Acked-by: Dave Kleikamp <shaggy@...>
> Acked-by: Paul E. McKenney <paulmck@...>
> ---
>  fs/fat/Kconfig      |   20 +++++++++++++++++
>  fs/fat/dir.c        |   15 ++++++-------
>  fs/fat/namei_vfat.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig
> index 182f9ff..907a5de 100644
> --- a/fs/fat/Kconfig
> +++ b/fs/fat/Kconfig
> @@ -74,6 +74,26 @@ config VFAT_FS
>    To compile this as a module, choose M here: the module will be called
>    vfat.
>  
> +config VFAT_FS_DUALNAMES
> + bool "VFAT dual names support"
> + depends on VFAT_FS

Defaults should be back-compatible.

> +
> +  Users considering enabling this option should consider the implications
> +  of any patents that may exist on dual filenames in VFAT.
> +
> +  If unsure, say N

Say Y.

Users considering disabling this should understand that filesystem
they write to will not be valid vfat filesystems, and may trigger bugs
in  some devices.

> +#ifndef CONFIG_VFAT_FS_DUALNAMES
> +/*
> + * build a 11 byte 8.3 buffer which is not a short filename. We want 11
> + * bytes which:
> + *    - will be seen as a constant string to all APIs on Linux and Windows
> + *    - cannot be matched with wildcard patterns
> + *    - cannot be used to access the file
> + *    - has a low probability of collision within a directory
> + *    - has an invalid 3 byte extension
> + *    - contains at least one non-space and non-nul byte
> + */

What happens on collision? With 60000 entries in directory, there will
be 50% chance of collision. BAD.

Why not use something like position in directory instead of random
number?
                                                                        Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

> What happens on collision? With 60000 entries in directory, there will
> be 50% chance of collision. BAD.

Far more surely - its a birthday paradox.

> Why not use something like position in directory instead of random
> number?

Agreed 100%. I'm also not sure it should be called "vfat" when operating
in this mode as it's not vfat any more - it needs a new name.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Rusty Russell]

On Tue, 30 Jun 2009 04:01:03 pm Pavel Machek wrote:
> > +config VFAT_FS_DUALNAMES
> > + bool "VFAT dual names support"
> > + depends on VFAT_FS
>
> Defaults should be back-compatible.

Hi Pavel,

I disagree with this: given there's been testing with no known issues, it's
not a back compat question.

I'd actually prefer to see the code ripped out and no config option available;
it would the clearest avoidance case possible.

I really don't want us to have to do this more than once :(
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Pavel,

 > Defaults should be back-compatible.

I would usually agree, but I think we have an unusual situation here,
in some ways similar to a demonstrated security hole. The previous
behaviour exposed a lot of Linux vendors to the possibility of an
expensive legal fight.

If we had some way to be completely backwards compatible while
avoiding the legal issues then of course we should take that. I tried
hard to find a varient that achieved that, but the best I could manage
was what I have posted. It may be that someone else can find a better
varient that avoids the legal issues while remaining fully backward
compatible.

 > Users considering disabling this should understand that filesystem
 > they write to will not be valid vfat filesystems, and may trigger bugs
 > in some devices.

If we find any devices that exhibit any problems with this patch while
it is in linux-next (and maybe linux-mm?) then this warning would
indeed be appropriate. It no such devices are known then I think the
warning is going a bit far.

Do you have a specific device in mind with regard to this warning?

 > What happens on collision? With 60000 entries in directory, there will
 > be 50% chance of collision. BAD.

Why do you say it is bad? You need to remember that those 11 bytes
cannot be used as a filename, so it isn't a filename collision. As I
mentioned in the reply to Eric's question, a quite good choice is to
use 11 spaces for every file, and that only falls down because
WindowsXP has a bug that causes that varient to bluescreen.

The only risks with collisions that I am aware of are:

  - there is a very small chance (much, _much_ less than 50% with 60k
    files) of WindowsXP bluescreening. I've never in fact managed to
    reproduce this with the current patch, despite many days of
    continuous randomised testing. The reason the probability is much
    smaller than 50% at 60k is that it doesn't bluescreen just because
    there is a duplicate 8.3 entry - it bluescreens with a particular
    access pattern that involves 2 entries with the same 11 bytes,
    that access pattern is very hard to produce.

  - chkdsk.exe will complain about duplicates, and will rename one of
    the two files. That is a 50% chance of 1 file being renamed for a
    single directory containing 60k files. Given it isn't all that
    common to run chkdsk on removable media that is shared between
    Linux and Windows, I thought that this is not a terribly large
    concern.

 > Why not use something like position in directory instead of random
 > number?

We did of course consider that, and the changes to the patch to
implement collision avoidance are relatively simple. We didn't do it
as it would weaken the legal basis behind the patch. I'll leave it to
John Lanza (the LF patent attorney) to expand on that if you want more
information.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Alan,

 > > What happens on collision? With 60000 entries in directory, there will
 > > be 50% chance of collision. BAD.
 >
 > Far more surely - its a birthday paradox.

If you want to do it accurately, the maximum number of long filenames
in a VFAT directory is actually 32767. (it isn't 65536, as each long
filename consumes at least two 8.3 entries, plus you lose the . and
.. entries).

With the patch I've posted there are 30 bits of randomness in each
entry. You could do an accurate binomial expansion to get the exact
probability, but a very good approximation using exponentiation comes
out as a 39.3% chance of a single duplicate appearing in a directory
that is fully populated.

As I mentioned to Pavel, this isn't the whole story though. To cause
the bluescreen the duplicate entries need to be accessed by WindowsXP
in quick succession in a particular pattern. This lowers the
probability a lot. Exactly how much is hard to estimate, but
experiments I've done with deliberately higher probabilities (ie. less
bits of randomness) show that the probability of the bluescreen is
_very_ low.

 > Agreed 100%. I'm also not sure it should be called "vfat" when operating
 > in this mode as it's not vfat any more - it needs a new name.

If the code differed significantly between the two implementations I'd
probably agree, but as the two are extremely close I think maintaining
a separate filesystem isn't worth it.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

> I'd actually prefer to see the code ripped out and no config option available;
> it would the clearest avoidance case possible.

What about the free world - why should we suffer because Americans have
a broken patent system ? That just leads to a Farenheit 451 model where
the kernel sources end up containing no code, text or image that can
offend, harm or be found wanting in any possible legal jurisdiction.

If we put in VFAT american fixes presumably we need to put in monitoring
features required by dubious governments ?

If you want to rip stuff out of your copy feel free. I am quite sure many
US based vendors will do that (because they do so already with things
like video codecs rather than just disabling the build option).

Also please stop calling it VFAT. With the changes made it isn't VFAT and
it's misleading to an end user to advertise it as vfat and users
shouldn't accidentally be able to specify -o vfat and get non-vfat. Thats
asking for incompatibility, data loss and unpleasant unwarned of suprises.

Its "linfat" or something which when you've fixed the bugs Pavel has
pointed out might be semi-compatible with other products (most *FAT using
products don't use Microsofts implementation). Testing it versus Windows
and saying it works is not really adequate. Thats what ACPI and BIOS
people do that *we* moan about all the time.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

> I would usually agree, but I think we have an unusual situation here,
> in some ways similar to a demonstrated security hole. The previous
> behaviour exposed a lot of Linux vendors to the possibility of an
> expensive legal fight.

They don't have to ship the code. They can rip it out. They deal with
video players the same way for the USSA market and have done for years.

>   - chkdsk.exe will complain about duplicates, and will rename one of
>     the two files. That is a 50% chance of 1 file being renamed for a
>     single directory containing 60k files. Given it isn't all that
>     common to run chkdsk on removable media that is shared between

Its a standard usage pattern for some people. Think about Linux based
commodity devices such as the N770 and plugging it into the users general
purpose PC box. Whenever it got pulled out wrongly it *will* get a chkdsk
in Windows.

>     Linux and Windows, I thought that this is not a terribly large
>     concern.

Disagree. Its a rapidly growing market segment.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

>  > Agreed 100%. I'm also not sure it should be called "vfat" when operating
>  > in this mode as it's not vfat any more - it needs a new name.
>
> If the code differed significantly between the two implementations I'd
> probably agree, but as the two are extremely close I think maintaining
> a separate filesystem isn't worth it.

It needs a different name to the user. If the new fs isn't vfat (which it
isn't) and doubly so if it can crash Windows XP at random on very rare
occasions then users need to know its different.

Imagine someone sticks a Linux written disk into a mission critical
windows server - I think they have a right to know and not accidentally
wander into a situation where they bring that box down ?

mount -o vfat should fail for this non-vfat.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Boaz Harrosh-5]

On 07/01/2009 01:50 PM, tridge@... wrote:
> Hi Pavel,
>
> We did of course consider that, and the changes to the patch to
> implement collision avoidance are relatively simple. We didn't do it
> as it would weaken the legal basis behind the patch. I'll leave it to
> John Lanza (the LF patent attorney) to expand on that if you want more
> information.
>

You completely lost me here. And I thought I did understand the patent
and the fix.

what is the difference between.

short_name = rand(sid);
and
short_name = sid++;

Now if you would do
short_name = MD5(long_name);

That I understand since short_name is some function of long_name
but if I'm just inventing the short_name out of my hat. In what legal
system does it matter what is my random function I use?

> Cheers, Tridge

Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Boaz,

 > That I understand since short_name is some function of long_name
 > but if I'm just inventing the short_name out of my hat. In what legal
 > system does it matter what is my random function I use?

I like to defer to John Lanza for a more complete answer to this, but
I can tell you that it is not related to the choice of random
function.

I'd also suggest that if anyone wants to go into this in any depth
then private email with John Lanza is the best option.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Alan,

 > They don't have to ship the code. They can rip it out. They deal with
 > video players the same way for the USSA market and have done for years.

yes, vendors can make the patch unconditional of course. I thought
that we were not trying to encourage divergance between the kernel.org
tree and the vendor trees though? I know some divergance is always
going to happen, but it seems counter productive to be encouraging it.

 > Its a standard usage pattern for some people. Think about Linux based
 > commodity devices such as the N770 and plugging it into the users general
 > purpose PC box. Whenever it got pulled out wrongly it *will* get a chkdsk
 > in Windows.

really? I haven't noticed that behaviour for removable devices in
windows. You can manually set a drive to be checked on reboot, but I
wasn't aware of any automatic chkdsk mechanism for VFAT removable
media in windows. Have you seen this yourself?

In testing this patch I've pulled USB keys in and out of WinXP, Vista
and Windows7 hundreds of time, and done it programatically millions of
times via scripting on virtual machines, but I never noticed this
feature. Perhaps it happens only under some specific circumstances I
haven't seen?

 > >     Linux and Windows, I thought that this is not a terribly large
 > >     concern.
 >
 > Disagree. Its a rapidly growing market segment.

you chopped off a word or two in the quote :-)

Of course I care about Linux/Windows interop, as I'm sure you know!
What I'm saying is that running chkdsk on a shared removable media
which contains 30k files in a single directory is not a common event,
and even when it does happen the consequences are that approximately
one in 3 times you do this, one of those 30k files gets renamed.

As evidence for this I'd like to point out that windows chkdsk has
complained about the Linux VFAT implementation for a long time, even
without my patch. If you create a filesystem with mformat and then
chkdsk it on windows you get:

  Bad links in lost chains at cluster 2 corrected
  Convert lost chains to files (Y/N)

yet I can't find a single instance of anyone complaining about this
with a google search. If a chkdsk was automated for removable media
then a lot of peoples machines would be asking them this question a
lot of the time. If people manually ran windows chkdsk on removable
VFAT media created on Linux then they also would have hit this and I
would have expected at least someone to have mentioned it.

Cheers, Tridge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/