drivers/staging/exfat - by default, prohibit mount of fat/vfat
diff mbox series

Message ID 245727.1567183359@turing-police
State New
Headers show
Series
  • drivers/staging/exfat - by default, prohibit mount of fat/vfat
Related show

Commit Message

Valdis Klētnieks Aug. 30, 2019, 4:42 p.m. UTC
Concerns have been raised about the exfat driver accidentally mounting
fat/vfat file systems.  Add an extra configure option to help prevent that.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>

Comments

Christoph Hellwig Aug. 30, 2019, 4:45 p.m. UTC | #1
On Fri, Aug 30, 2019 at 12:42:39PM -0400, Valdis Klētnieks wrote:
> Concerns have been raised about the exfat driver accidentally mounting
> fat/vfat file systems.  Add an extra configure option to help prevent that.

Just remove that code.  This is exactly what I fear about this staging
crap, all kinds of half-a***ed patches instead of trying to get anything
done.  Given that you signed up as the maintainer for this what is your
plan forward on it?  What development did you on the code and what are
your next steps?
Christoph Hellwig Aug. 31, 2019, 6:46 a.m. UTC | #2
On Fri, Aug 30, 2019 at 08:48:36PM -0400, Valdis Klētnieks wrote:
> Explain how it's half-a**ed.  You worry about accidental mounting, meanwhile
> down in the embedded space there are memory-constrained machines that
> don't want separate vfat and exfat drivers sitting around in memory. If you
> have a better patch that addresses both concerns, feel free to submit it.

Since when did Linux kernel submissions become "show me a better patch"
to reject something obviously bad?

As I said the right approach is to probably (pending comments from the
actual fat maintainer) to merge exfat support into the existing fs/fat/
codebase.  You obviously seem to disagree (and at the same time not).

But using this as a pre-text of adding a non-disabled second fat16/32
implementation and actually allowing that to build with no reason is
just not how it works.

> > done.  Given that you signed up as the maintainer for this what is your
> > plan forward on it?  What development did you on the code and what are
> > your next steps?
> 
> Well, the *original* plan was to get it into the tree someplace so it can get
> review and updates from others.

In other words you have no actual plan and no idea what to do and want to
rely on "others" to do anything, but at the same time reject the
comments from others how to do things right?

> Given the amount of press the Microsoft
> announcement had, we were *hoping* there would be some momentum and
> people actually looking at the code and feeding me patches. I've gotten a
> half dozen already today....

And all of that you can easily do by just sending out a patch series.
And maybe actually listening to comments.

> Although if you prefer, it can just sit out-of-tree until I've got a perfect driver
> without input or review from anybody.  But I can't think of *any* instance where
> that model has actually worked.

You generally get a lot of review and comments by posting to the mailing
list.  But what really helps is to just do the very basic homework
beforehand.  And it also really helps to have a plan what you want to
do with a codebase you just copy and pasted from somewhere.
Andy Shevchenko Aug. 31, 2019, 2:24 p.m. UTC | #3
On Sat, Aug 31, 2019 at 1:26 PM Valdis Klētnieks
<valdis.kletnieks@vt.edu> wrote:

> [/usr/src/linux-next] git log -- fs/ext4 | awk '/^commit/ {merge=0} /^Merge: / {merge=1} /^Author:/ { if (!merge) {print $0}}' | sort | uniq -c | sort -t: -k 1,1nr -k 2

Side note:

% git shortlog -n --no-merges -- fs/ext4 | grep '^[^ ]'

kinda faster and groups by name.
Valdis Klētnieks Aug. 31, 2019, 2:51 p.m. UTC | #4
On Sat, 31 Aug 2019 17:24:47 +0300, Andy Shevchenko said:

> Side note:
>
> % git shortlog -n --no-merges -- fs/ext4 | grep '^[^ ]'
>
> kinda faster and groups by name.

Thanks... I rarely do statistical analyses of this sort of thing, so 'git
shortlog' isn't on my list of most-used git subcommands...
Dave Chinner Sept. 1, 2019, 1:07 a.m. UTC | #5
On Sat, Aug 31, 2019 at 06:25:21AM -0400, Valdis Klētnieks wrote:
> On Fri, 30 Aug 2019 23:46:16 -0700, Christoph Hellwig said:
> > Since when did Linux kernel submissions become "show me a better patch"
> > to reject something obviously bad?
> 
> Well, do you even have a *suggestion* for a better idea?  Other than "just rip
> it out"?  Keeping in mind that:
> 
> > As I said the right approach is to probably (pending comments from the
> > actual fat maintainer) to merge exfat support into the existing fs/fat/
> > codebase.  You obviously seem to disagree (and at the same time not).
> 
> At this point, there isn't any true consensus on whether that's the best
> approach at the current.

Which, quite frankly, means it has been merged prematurely.

Valdis - the model for getting a new filesystem merged is the one
taken by Orangefs. That was not merged through the staging tree,
it was reviewd via patches to linux-fsdevel that were iterated far
faster than the stable merge cycle allows, allowed all the cleanups
to be done independently of the feature work needed, the structural
changes we easy to discuss, quote, etc.

These are the sorts of problems we are having with EROFS right now,
even though it's been in staging for some time, and it's clear we
are already having them with exfat - fundamental architecture issues
have not yet been decided, and so there's likely major structural
change yet to be done.

That's stuff that is much more easily done and reveiwed by patches
on a mailing list. You don't need the code in the staging tree to
get this sort of thing done and, really, having it already merged
gets in the way of doing major structural change as it cannot be
rapidly iterated independently of the kernel dev cycle...

So when Christoph say:

> "Just rip it out"

what he is really saying is that Greg has jumped the jump and is -
yet again - fucking over filesystem developers because he's
taken the review process for a new filesystem out of hands _yet
again_.

He did this with POHMELFS, then Lustre, then EROFS - they all got
merged into stable over the objections of senior filesystem
developers.  The first two were an utter disaster, the latter is
rapidly turning into one.

You wanted a "show me a better patch" response from Christoph. What
he actually is saying is "we've got a better process for reviewing
and merging filesystems". That is, we need to reboot the exfat
process from the start - sort out the fundamental implementation
direction and then implement via the normal out-of-tree patch series
iteration process.

That's the fundamental problem here - we have a rogue maintainer
that is abusing their power by subverting our normal review and
merge process. I don't know (or care) what motive that maintainer
has for expedited merging of this filesystem, but history tells us
it _will_ result in a total clusterfuck in the near future. In fact,
I'd argue that the clusterfuck has already arrived....

> And by the way, it seems like other filesystems rely on "others" to help out.
> Let's look at the non-merge commit for fs/ext4. And these are actual patches,
> not just reviewer comments....

Totally irrelevant to the issue at hand. You can easily co-ordinate
out of tree contributions through a github tree, or a tree on
kernel.org, etc.

Being in the staging tree does not get you more robust review -
it'll just delay it until someone wants it out of the staging tree,
and then you'll get all the same issues with experienced filesystem
developer review as you are getting now.

That's the choice you have to make now: listen to the reviewers
saying "resolve the fundamental issues before goign any further",
or you can ignore that and have it rejected after another year of
work because the fundamental issues haven't been resolved while it
sits in staging....

Cheers,

Dave.
Gao Xiang Sept. 1, 2019, 1:37 a.m. UTC | #6
Hi Dave,

On Sun, Sep 01, 2019 at 11:07:21AM +1000, Dave Chinner wrote:
> On Sat, Aug 31, 2019 at 06:25:21AM -0400, Valdis Kl??tnieks wrote:
> > On Fri, 30 Aug 2019 23:46:16 -0700, Christoph Hellwig said:
> > > Since when did Linux kernel submissions become "show me a better patch"
> > > to reject something obviously bad?
> > 
> > Well, do you even have a *suggestion* for a better idea?  Other than "just rip
> > it out"?  Keeping in mind that:
> > 
> > > As I said the right approach is to probably (pending comments from the
> > > actual fat maintainer) to merge exfat support into the existing fs/fat/
> > > codebase.  You obviously seem to disagree (and at the same time not).
> > 
> > At this point, there isn't any true consensus on whether that's the best
> > approach at the current.
> 
> Which, quite frankly, means it has been merged prematurely.
> 
> Valdis - the model for getting a new filesystem merged is the one
> taken by Orangefs. That was not merged through the staging tree,
> it was reviewd via patches to linux-fsdevel that were iterated far
> faster than the stable merge cycle allows, allowed all the cleanups
> to be done independently of the feature work needed, the structural
> changes we easy to discuss, quote, etc.

fs/orangefs/dir.c
 61 static int do_readdir(struct orangefs_inode_s *oi,
 62     struct orangefs_dir *od, struct dentry *dentry,
 63     struct orangefs_kernel_op_s *op)

131 static int parse_readdir(struct orangefs_dir *od,
132     struct orangefs_kernel_op_s *op)

189 static int fill_from_part(struct orangefs_dir_part *part,
190     struct dir_context *ctx)

fs/orangefs/file.c
 19 static int flush_racache(struct inode *inode)

 48 ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
 49     loff_t *offset, struct iov_iter *iter, size_t total_size,
 50     loff_t readahead_size, struct orangefs_write_range *wr, int *index_return)

fs/orangefs/super.c
304 
305 int fsid_key_table_initialize(void)
306 {
307         return 0;
308 }
309 
310 void fsid_key_table_finalize(void)
311 {
312 }

----> no prefix and empty functions

fs/orangefs/xattr.c
 31 static int is_reserved_key(const char *key, size_t size)
 40 static inline int convert_to_internal_xattr_flags(int setxattr_flags)
 54 static unsigned int xattr_key(const char *key)

> 
> These are the sorts of problems we are having with EROFS right now,
> even though it's been in staging for some time, and it's clear we
> are already having them with exfat - fundamental architecture issues
> have not yet been decided, and so there's likely major structural
> change yet to be done.
> 
> That's stuff that is much more easily done and reveiwed by patches
> on a mailing list. You don't need the code in the staging tree to
> get this sort of thing done and, really, having it already merged
> gets in the way of doing major structural change as it cannot be
> rapidly iterated independently of the kernel dev cycle...
> 
> So when Christoph say:
> 
> > "Just rip it out"
> 
> what he is really saying is that Greg has jumped the jump and is -
> yet again - fucking over filesystem developers because he's
> taken the review process for a new filesystem out of hands _yet
> again_.
> 
> He did this with POHMELFS, then Lustre, then EROFS - they all got
> merged into stable over the objections of senior filesystem
> developers.  The first two were an utter disaster, the latter is
> rapidly turning into one.

I don't know what "rapidly turning" means:
 This round I am working on all suggestions from fs developers;
 and I have been addressing what Christoph said for days, he hope that
 all functions should be prefixed with "erofs_" and I am doing.

That is all.

Thanks,
Gao Xiang
Al Viro Sept. 1, 2019, 3:05 a.m. UTC | #7
On Sun, Sep 01, 2019 at 09:37:19AM +0800, Gao Xiang wrote:

> fs/orangefs/file.c
>  19 static int flush_racache(struct inode *inode)

Just why the hell would _that_ one be a problem?  It's static in
file; it can't pollute the namespace even if linked into the
kernel.

Folks, let's keep at least some degree of sanity - this is sinking
to the level of certain killfile denizens...
Gao Xiang Sept. 1, 2019, 3:26 a.m. UTC | #8
Hi Al,

On Sun, Sep 01, 2019 at 04:05:14AM +0100, Al Viro wrote:
> On Sun, Sep 01, 2019 at 09:37:19AM +0800, Gao Xiang wrote:
> 
> > fs/orangefs/file.c
> >  19 static int flush_racache(struct inode *inode)
> 
> Just why the hell would _that_ one be a problem?  It's static in
> file; it can't pollute the namespace even if linked into the
> kernel.
> 
> Folks, let's keep at least some degree of sanity - this is sinking
> to the level of certain killfile denizens...

Thanks for your kind reply. I think in the same way.
And Christoph did many great suggestions for erofs, thanks him
for erofs, and I'm already fixed most of them, and some
suggestions I have no idea to do....
  1) add "erofs_" to all functions [1] [2];
  2) avoid sb_bread and use read_mapping_page, actually
      read_mapping_page will call block_read_full_page and
      buffer_heads still there;

and I don't know what erofs "rapidly turning" means, all great
suggestions I can fix them all, I have no idea it's a bad thing.

[1] https://lore.kernel.org/linux-fsdevel/20190830163910.GB29603@infradead.org/
[2] https://lore.kernel.org/linux-fsdevel/20190831064853.GA162401@architecture4/

Thanks,
Gao Xiang
Valdis Klētnieks Sept. 1, 2019, 3:37 a.m. UTC | #9
On Sun, 01 Sep 2019 11:07:21 +1000, Dave Chinner said:
> Totally irrelevant to the issue at hand. You can easily co-ordinate
> out of tree contributions through a github tree, or a tree on
> kernel.org, etc.

Well.. I'm not personally wedded to the staging tree.  I'm just interested in
getting a driver done and upstreamed with as little pain as possible. :)

Is there any preference for github versus kernel.org?  I can set up a github
tree on my own, no idea who needs to do what for a kernel.org tree.

Also, this (from another email of yours) was (at least to me) the most useful
thing said so far:

> look at what other people have raised w.r.t. to that filesystem -
> on-disk format validation, re-implementation of largely generic
> code, lack of namespacing of functions leading to conflicts with
> generic/VFS functionality, etc.

All of which are now on the to-do list, thanks.

Now one big question:

Should I heave all the vfat stuff overboard and make a module that
*only* does exfat, or is there enough interest in an extended FAT module
that does vfat and extfat, in which case the direction should be to re-align
this module's code with vfat?

> That's the choice you have to make now: listen to the reviewers
> saying "resolve the fundamental issues before goign any further",

Well... *getting* a laundry list of what the reviewers see as the fundamental
issues is the first step in resolving them ;)
Dave Chinner Sept. 1, 2019, 10:43 p.m. UTC | #10
On Sat, Aug 31, 2019 at 11:37:27PM -0400, Valdis Klētnieks wrote:
> On Sun, 01 Sep 2019 11:07:21 +1000, Dave Chinner said:
> > Totally irrelevant to the issue at hand. You can easily co-ordinate
> > out of tree contributions through a github tree, or a tree on
> > kernel.org, etc.
> 
> Well.. I'm not personally wedded to the staging tree.  I'm just interested in
> getting a driver done and upstreamed with as little pain as possible. :)

Understood - I'm trying to head off you guys getting delayed
by sitting for a year in the staging tree polishing a turd and not
addressing the things that really need to be done first...

> Is there any preference for github versus kernel.org?  I can set up a github
> tree on my own, no idea who needs to do what for a kernel.org tree.

What ever is most convenient for you to manage and co-ordinate. :P

> Also, this (from another email of yours) was (at least to me) the most useful
> thing said so far:
> 
> > look at what other people have raised w.r.t. to that filesystem -
> > on-disk format validation, re-implementation of largely generic
> > code, lack of namespacing of functions leading to conflicts with
> > generic/VFS functionality, etc.
> 
> All of which are now on the to-do list, thanks.
> 
> Now one big question:
> 
> Should I heave all the vfat stuff overboard and make a module that
> *only* does exfat, or is there enough interest in an extended FAT module
> that does vfat and extfat, in which case the direction should be to re-align
> this module's code with vfat?

I don't know the details of the exfat spec or the code to know what
the best approach is. I've worked fairly closely with Christoph for
more than a decade - you need to think about what he says rather
than /how he says it/ because there's a lot of thought and knowledge
behind his reasoning. Hence if I were implementing exfat and
Christoph was saying "throw it away and extend fs/fat"
then that's what I'd be doing.

A lot of this is largely risk management - there's a good chance
that the people developing the code move on after the project is
done and merged, which leaves the people like Christoph with the
burden of long term code maintenance for that filesystem. There's
enough crusty, old, largely unmaintained filesystem code already,
and we don't want more. Implementing exfat on top of fs/fat kills
two birds with one stone - it modernises the fs/fat code base and
brings new functionality that will have more developers interested
in maintaining it over the long term. So from an overall filesystem
maintenance perspective, building on top of fs/fat makes a lot of
sense to a kernel filesystem developer...

This is not a judgement on the quality of the existing exfat code
or it's developers - it's just that there are very good reasons for
building on existing codebase rather than creating a whole new code
base that has very similar functionality.

That's my total involvement in this - I don't really care about
exfat at all - my stake in this is to improve the development,
review and merge process being undertaken. We have a history of lax
review, poor processes and really shitty code being merged into the
kernel and I've been on the cleanup squad for a few of them over the
past couple of years. That's a burnout vector, so it's in the
interests of my own sanity (and fs developers) to set standards and
precedence to prevent more trainwrecks from happening before the
train even leaves the station...

> > That's the choice you have to make now: listen to the reviewers
> > saying "resolve the fundamental issues before goign any further",
> 
> Well... *getting* a laundry list of what the reviewers see as the fundamental
> issues is the first step in resolving them ;)

You won't get them all at once. You'll get something new every round
of review as the bigger issues are worked out, the reviewers become
more familiar with the code and notice more detailed/subtle
issues. Most filesystem reviews start with developers trying to
understand the on-disk structure and architecture rather that focus
on whitespace and code cleanliness. Cleaning up the code can be done
after we work through all the structural issues...

Cheers,

Dave.
Valdis Klētnieks Sept. 1, 2019, 11:13 p.m. UTC | #11
On Mon, 02 Sep 2019 08:43:29 +1000, Dave Chinner said:

> I don't know the details of the exfat spec or the code to know what
> the best approach is. I've worked fairly closely with Christoph for
> more than a decade - you need to think about what he says rather
> than /how he says it/ because there's a lot of thought and knowledge
> behind his reasoning. Hence if I were implementing exfat and
> Christoph was saying "throw it away and extend fs/fat"
> then that's what I'd be doing.

Again, I'm not ruling that out if that's the consensus direction. After all,
the goal is to merge a working driver - and for that, I need to produce
something that the file system maintainers will be willing to merge, which
means doing it in a way they want it...

Hopefully next week a few other people will weigh in with what they prefer as
far as approach goes.  Only definite statement I've heard so far was
Christoph's...

> and we don't want more. Implementing exfat on top of fs/fat kills
> two birds with one stone - it modernises the fs/fat code base and
> brings new functionality that will have more developers interested
> in maintaining it over the long term.

Any recommendations on how to approach that?   Clone the current fs/fat code
and develop on top of that, or create a branch of it and on occasion do the
merging needed to track further fs/fat development?

Mostly asking for workflow suggestions - what's known to work well for this
sort of situation, where we know we won't be merging until we have several
thousand lines of new code?  And any "don't do <this> or you'll regret it
later" advice is also appreciated. :)
Christoph Hellwig Sept. 2, 2019, 7:35 a.m. UTC | #12
On Sat, Aug 31, 2019 at 06:25:21AM -0400, Valdis Klētnieks wrote:
> On Fri, 30 Aug 2019 23:46:16 -0700, Christoph Hellwig said:
> 
> > Since when did Linux kernel submissions become "show me a better patch"
> > to reject something obviously bad?
> 
> Well, do you even have a *suggestion* for a better idea?  Other than "just rip
> it out"?  Keeping in mind that:

The right approach in my opinion is to start submitting patches to fs/fat
to add exfat support.  But more importantly it is to first coordinate
with other stakeholder most importantly the fs/fat/ maintainer and the
dosfstools maintainers as our local experts for fat-like file systems
instead of shooting from the hip.

> I think at that point, we can safely say that if it mounts a vfat filesystem,
> it's because the person building the kernel has gone out of their way to
> request that it do so.

Especially with boot time automounting it could happen.  Never mind that
we do not add duplicate file system drivers (or drivers in general) to
the kernel.

> Now, if what you want is "Please make it so the fat16/fat32 code is in separate
> files that aren't built unless requested", that's in fact doable and a
> reasonable request, and one that both doesn't conflict with anything other
> directions we might want to go, and also prepares the code for more easy
> separation if it's decided we really do want an exfat-only module.

No.  Assuming we even want the current codebase (which only you and
Greg seem to think so), that fat16/32 code simply has to go.

> And by the way, it seems like other filesystems rely on "others" to help out.
> Let's look at the non-merge commit for fs/ext4. And these are actual patches,
> not just reviewer comments....
> 
> (For those who don't feel like scrolling - 77.6% of the non-merge ext4 commits
> are from a total of 463 somebodies other than Ted Ts'o)
> 
> Even some guy named Christoph Hellwig gave Ted Ts'o a bunch of help.

That isn't the point.  Everyone who submitted a file system had a clear
plan where they wanted to go.  You just took someone elses out of tree
code, apparently didn't even try to understand it and are not able to
come up with what your plan for it is.  And even after repeated
questions on what that plan is duck the question and instead attack the
people asking for it.
Christoph Hellwig Sept. 2, 2019, 7:38 a.m. UTC | #13
On Sun, Sep 01, 2019 at 07:13:54PM -0400, Valdis Klētnieks wrote:
> Any recommendations on how to approach that?   Clone the current fs/fat code
> and develop on top of that, or create a branch of it and on occasion do the
> merging needed to track further fs/fat development?
> 
> Mostly asking for workflow suggestions - what's known to work well for this
> sort of situation, where we know we won't be merging until we have several
> thousand lines of new code?  And any "don't do <this> or you'll regret it
> later" advice is also appreciated. :)

Just try to hack it in in your local tree and see if it works at all.
Normally you should have a feeling of where this is heading at this
point and start iterating.  One it looks somewhat presentable send it
out to the list and ask for comments.
Greg Kroah-Hartman Sept. 2, 2019, 3:25 p.m. UTC | #14
On Mon, Sep 02, 2019 at 12:35:25AM -0700, Christoph Hellwig wrote:
> On Sat, Aug 31, 2019 at 06:25:21AM -0400, Valdis Klētnieks wrote:
> > On Fri, 30 Aug 2019 23:46:16 -0700, Christoph Hellwig said:
> > 
> > > Since when did Linux kernel submissions become "show me a better patch"
> > > to reject something obviously bad?
> > 
> > Well, do you even have a *suggestion* for a better idea?  Other than "just rip
> > it out"?  Keeping in mind that:
> 
> The right approach in my opinion is to start submitting patches to fs/fat
> to add exfat support.  But more importantly it is to first coordinate
> with other stakeholder most importantly the fs/fat/ maintainer and the
> dosfstools maintainers as our local experts for fat-like file systems
> instead of shooting from the hip.

I dug up my old discussion with the current vfat maintainer and he said
something to the affect of, "leave the existing code alone, make a new
filesystem, I don't want anything to do with exfat".

And I don't blame them, vfat is fine as-is and stable and shouldn't be
touched for new things.

We can keep non-vfat filesystems from being mounted with the exfat
codebase, and make things simpler for everyone involved.

> > Now, if what you want is "Please make it so the fat16/fat32 code is in separate
> > files that aren't built unless requested", that's in fact doable and a
> > reasonable request, and one that both doesn't conflict with anything other
> > directions we might want to go, and also prepares the code for more easy
> > separation if it's decided we really do want an exfat-only module.
> 
> No.  Assuming we even want the current codebase (which only you and
> Greg seem to think so), that fat16/32 code simply has to go.

I don't think it should stay in there, let's drop it from the exfat
code.

As for the other issues discussed here in this thread:
  - yes, putting a filesystem in staging is extra work overall, but for
    projects that want to do that extra work, wonderful, do it here in a
    common place for everyone to work on together.
  - working on in a common place is what we need for exfat right now,
    as there are 40+ different github forks and no one knows which one
    is "correct" or most up to date.  We needed to decide on "one" and
    here it is, the in-tree one.
  - for vfs developers who don't want to even look at the crud in
    staging (remember, it's TAINT_CRAP if you load code from here),
    don't.  Just keep on your own normal development cycles and if you
    break any staging code, it's fine, I will fix it up no complaints at
    all.
  - staging code is for crappy code to get fixed up.  If it isn't
    constantly updated, it will be dropped.  Yes, there is code in there
    that probably should be dropped now as I haven't done a sweep in a
    few years, suggestions always welcome.  There is also code that
    needs to be moved out with just a bit more work needed (greybus,
    comedi, speakup, etc.)  Some of that is underway right now.

thanks,

greg k-h
Valdis Klētnieks Sept. 2, 2019, 7 p.m. UTC | #15
On Mon, 02 Sep 2019 17:25:24 +0200, Greg Kroah-Hartman said:

> I dug up my old discussion with the current vfat maintainer and he said
> something to the affect of, "leave the existing code alone, make a new
> filesystem, I don't want anything to do with exfat".
>
> And I don't blame them, vfat is fine as-is and stable and shouldn't be
> touched for new things.
>
> We can keep non-vfat filesystems from being mounted with the exfat
> codebase, and make things simpler for everyone involved.

Ogawa:

Is this still your position, that you want exfat to be a separate module?
Greg Kroah-Hartman Sept. 2, 2019, 7:06 p.m. UTC | #16
On Mon, Sep 02, 2019 at 03:00:17PM -0400, Valdis Klētnieks wrote:
> On Mon, 02 Sep 2019 17:25:24 +0200, Greg Kroah-Hartman said:
> 
> > I dug up my old discussion with the current vfat maintainer and he said
> > something to the affect of, "leave the existing code alone, make a new
> > filesystem, I don't want anything to do with exfat".
> >
> > And I don't blame them, vfat is fine as-is and stable and shouldn't be
> > touched for new things.
> >
> > We can keep non-vfat filesystems from being mounted with the exfat
> > codebase, and make things simpler for everyone involved.
> 
> Ogawa:
> 
> Is this still your position, that you want exfat to be a separate module?

Personally I agree that this should be separate at least for quite some
time to shake things out at the very least.  But I'll defer to Ogawa if
he thinks things should be merged.

thanks,

greg k-h
OGAWA Hirofumi Sept. 8, 2019, 10:50 a.m. UTC | #17
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Mon, Sep 02, 2019 at 03:00:17PM -0400, Valdis Klētnieks wrote:
>> On Mon, 02 Sep 2019 17:25:24 +0200, Greg Kroah-Hartman said:
>> 
>> > I dug up my old discussion with the current vfat maintainer and he said
>> > something to the affect of, "leave the existing code alone, make a new
>> > filesystem, I don't want anything to do with exfat".
>> >
>> > And I don't blame them, vfat is fine as-is and stable and shouldn't be
>> > touched for new things.
>> >
>> > We can keep non-vfat filesystems from being mounted with the exfat
>> > codebase, and make things simpler for everyone involved.
>> 
>> Ogawa:
>> 
>> Is this still your position, that you want exfat to be a separate module?
>
> Personally I agree that this should be separate at least for quite some
> time to shake things out at the very least.  But I'll defer to Ogawa if
> he thinks things should be merged.

I'm not reading whole of this thread, so I can be pointless though. I
can't recall the discussion of exfat with you. My history about exfat
is,

   write read-only exfat from on-disk data -> MS published patent to
   their site or such -> stopped about exfat -> recently looks like MS
   changed mind

Well, if you are going to developing actively, IMO it would be better to
drop historically bad decisions in fat driver (some stuff would be hard
to fix without user visible changes), and re-think from basic
implementation design.

And I can't recall the detail of exfat format though, IIRC, the common
code is not so big, but some stuff can be shared with fat (timestamp
stuff, fatent stuff, IIRC). So IMO it is better to be different driver
basically, however on other hand, it is better to share the code for
same on-disk format if possible.

Anyway, I don't have strong opinion about it.

Thanks.

Patch
diff mbox series

diff --git a/drivers/staging/exfat/Kconfig b/drivers/staging/exfat/Kconfig
index 78b32aa2ca19..1df177b1dc72 100644
--- a/drivers/staging/exfat/Kconfig
+++ b/drivers/staging/exfat/Kconfig
@@ -4,6 +4,14 @@  config EXFAT_FS
 	help
 	  This adds support for the exFAT file system.
 
+config EXFAT_DONT_MOUNT_VFAT
+	bool "Prohibit mounting of fat/vfat filesysems by exFAT"
+	default y
+	help
+	  By default, the exFAT driver will only mount exFAT filesystems, and refuse
+	  to mount fat/vfat filesystems.  Set this to 'n' to allow the exFAT driver
+	  to mount these filesystems.
+
 config EXFAT_DISCARD
 	bool "enable discard support"
 	depends on EXFAT_FS
diff --git a/drivers/staging/exfat/exfat_super.c b/drivers/staging/exfat/exfat_super.c
index 5b5c2ca8c9aa..7fdb5b8bc928 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -486,10 +486,16 @@  static int ffsMountVol(struct super_block *sb)
 			break;
 
 	if (i < 53) {
+#ifdef CONFIG_EXFAT_DONT_MOUNT_VFAT
+		ret = -EINVAL;
+		printk(KERN_INFO "EXFAT: Attempted to mount VFAT filesystem\n");
+		goto out;
+#else
 		if (GET16(p_pbr->bpb + 11)) /* num_fat_sectors */
 			ret = fat16_mount(sb, p_pbr);
 		else
 			ret = fat32_mount(sb, p_pbr);
+#endif
 	} else {
 		ret = exfat_mount(sb, p_pbr);
 	}