2019-08-30 16:44:17

by Valdis Klētnieks

[permalink] [raw]
Subject: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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 <[email protected]>
Signed-off-by: Valdis Kletnieks <[email protected]>

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);
}


2019-08-30 16:46:19

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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?

2019-08-31 00:49:48

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

On Fri, 30 Aug 2019 09:45:03 -0700, Christoph Hellwig said:
> 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

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.

> 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. 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....

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.


Attachments:
(No filename) (849.00 B)

2019-08-31 06:48:48

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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.

2019-08-31 10:26:48

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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. "Just rip it out" prevents following some directions
that people have voiced interest in. Now, if there was consensus that we
wanted a separate module that only did exfat, you'd be correct.

And just for the record, I've been around since the 2.5.47 or so kernel, and
I've seen *plenty* of times when somebody has submitted a better alternative
patch.

> 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.

Well.. let's see... It won't build unless you select CONFIG_STAGING. *And* you
select CONFIG_EXFAT_FS. *And* it wont mount anything other than exfat
unless you change CONFIG_EXFAT_DONT_MOUNT_VFAT

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.

Either that, or we have a major bug in kbuild. In general, kbuild doesn't build
things "for no reason".

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.

> > > 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?

So far, the only comment I've rejected is one "just rip it out" that conflicts with
directions that others have indicated we may want to pursue.

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.

[/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
718 Author: Theodore Ts'o <[email protected]>
260 Author: Jan Kara <[email protected]>
151 Author: Aneesh Kumar K.V <[email protected]>
120 Author: Eric Sandeen <[email protected]>
119 Author: Lukas Czerner <[email protected]>
99 Author: Dmitry Monakhov <[email protected]>
84 Author: Al Viro <[email protected]>
67 Author: Eric Biggers <[email protected]>
63 Author: Tao Ma <[email protected]>
61 Author: Yongqiang Yang <[email protected]>
49 Author: Zheng Liu <[email protected]>
47 Author: Christoph Hellwig <[email protected]>
42 Author: Darrick J. Wong <[email protected]>
40 Author: Tahsin Erdogan <[email protected]>
36 Author: Mingming Cao <[email protected]>
32 Author: Eric Whitney <[email protected]>
28 Author: Christoph Hellwig <[email protected]>
27 Author: Curt Wohlgemuth <[email protected]>
24 Author: Darrick J. Wong <[email protected]>
22 Author: Akira Fujita <[email protected]>
20 Author: Ross Zwisler <[email protected]>
18 Author: Eryu Guan <[email protected]>
16 Author: Vasily Averin <[email protected]>
15 Author: Akinobu Mita <[email protected]>
15 Author: Allison Henderson <[email protected]>
15 Author: Robin Dong <[email protected]>
14 Author: Dan Carpenter <[email protected]>
13 Author: Michael Halcrow <[email protected]>
13 Author: Miklos Szeredi <[email protected]>
13 Author: Wang Shilong <[email protected]>
12 Author: Daeho Jeong <[email protected]>
12 Author: Jan Kara <[email protected]>
12 Author: Joe Perches <[email protected]>
12 Author: Tejun Heo <[email protected]>
11 Author: Jiaying Zhang <[email protected]>
11 Author: Namjae Jeon <[email protected]>
11 Author: Shen Feng <[email protected]>
10 Author: David Howells <[email protected]>
10 Author: Guo Chao <[email protected]>
10 Author: Matthew Wilcox <[email protected]>
9 Author: Alexey Dobriyan <[email protected]>
9 Author: Andreas Gruenbacher <[email protected]>
9 Author: Fabian Frederick <[email protected]>
9 Author: Linus Torvalds <[email protected]>
8 Author: Amir Goldstein <[email protected]>
8 Author: Amir Goldstein <[email protected]>
8 Author: Andrew Morton <[email protected]>
8 Author: Artem Bityutskiy <[email protected]>
8 Author: Dan Williams <[email protected]>
7 Author: Aditya Kali <[email protected]>
7 Author: Al Viro <[email protected]>
7 Author: Alex Tomas <[email protected]>
7 Author: Arnd Bergmann <[email protected]>
7 Author: Colin Ian King <[email protected]>
7 Author: Dave Jiang <[email protected]>
7 Author: Hugh Dickins <[email protected]>
7 Author: Kazuya Mio <[email protected]>
7 Author: Toshiyuki Okajima <[email protected]>
7 Author: Zheng Liu <[email protected]>
7 Author: zhangyi (F) <[email protected]>
6 Author: Anatol Pomozov <[email protected]>
6 Author: Andreas Dilger <[email protected]>
6 Author: Christoph Lameter <[email protected]>
6 Author: Dan Carpenter <[email protected]>
6 Author: Duane Griffin <[email protected]>
6 Author: Eric W. Biederman <[email protected]>
6 Author: Josef Bacik <[email protected]>
6 Author: Kirill A. Shutemov <[email protected]>
6 Author: Konstantin Khlebnikov <[email protected]>
6 Author: Li Zefan <[email protected]>
6 Author: Mingming <[email protected]>
6 Author: Tyson Nottingham <[email protected]>
5 Author: Andrew Morton <[email protected]>
5 Author: Avantika Mathur <[email protected]>
5 Author: Chandan Rajendra <[email protected]>
5 Author: Frank Mayhar <[email protected]>
5 Author: Frederic Bohe <[email protected]>
5 Author: Jose R. Santos <[email protected]>
5 Author: Kees Cook <[email protected]>
5 Author: Michal Hocko <[email protected]>
5 Author: Mike Christie <[email protected]>
5 Author: Omar Sandoval <[email protected]>
5 Author: Peter Zijlstra <[email protected]>
5 Author: Vegard Nossum <[email protected]>
5 Author: Wu Fengguang <[email protected]>
5 Author: yangerkun <[email protected]>
4 Author: Amit Arora <[email protected]>
4 Author: Carlos Maiolino <[email protected]>
4 Author: Chengguang Xu <[email protected]>
4 Author: Coly Li <[email protected]>
4 Author: David Gstir <[email protected]>
4 Author: Djalal Harouni <[email protected]>
4 Author: Hidehiro Kawai <[email protected]>
4 Author: Jeff Layton <[email protected]>
4 Author: Josef Bacik <[email protected]>
4 Author: Li Xi <[email protected]>
4 Author: Manish Katiyar <[email protected]>
4 Author: Miao Xie <[email protected]>
4 Author: Miklos Szeredi <[email protected]>
4 Author: Roel Kluin <[email protected]>
4 Author: Toshi Kani <[email protected]>
4 Author: Uwe Kleine-K?nig <[email protected]>
4 Author: Valerie Clement <[email protected]>
4 Author: Xiaoguang Wang <[email protected]>
3 Author: Andi Kleen <[email protected]>
3 Author: Azat Khuzhin <[email protected]>
3 Author: Ben Hutchings <[email protected]>
3 Author: Chandan Rajendra <[email protected]>
3 Author: Dave Chinner <[email protected]>
3 Author: Eric Gouriou <[email protected]>
3 Author: Eric Sandeen <[email protected]>
3 Author: Gabriel Krisman Bertazi <[email protected]>
3 Author: Gabriel Krisman Bertazi <[email protected]>
3 Author: H Hartley Sweeten <[email protected]>
3 Author: Jaegeuk Kim <[email protected]>
3 Author: Jeff Moyer <[email protected]>
3 Author: Jing Zhang <[email protected]>
3 Author: Julia Lawall <[email protected]>
3 Author: Mathieu Malaterre <[email protected]>
3 Author: Maurizio Lombardi <[email protected]>
3 Author: Nick Piggin <[email protected]>
3 Author: Nick Piggin <[email protected]>
3 Author: Nicolai Stange <[email protected]>
3 Author: Nikolay Borisov <[email protected]>
3 Author: Pekka Enberg <[email protected]>
3 Author: Peng Tao <[email protected]>
3 Author: Randy Dunlap <[email protected]>
3 Author: Rasmus Villemoes <[email protected]>
3 Author: Riccardo Schirone <[email protected]>
3 Author: Robin Dong <[email protected]>
3 Author: Thiemo Nagel <[email protected]>
3 Author: Wei Yongjun <[email protected]>
3 Author: jon ernst <[email protected]>
2 Author: Adam Buchbinder <[email protected]>
2 Author: Alexander Beregalov <[email protected]>
2 Author: Alexandre Ratchov <[email protected]>
2 Author: Andi Kleen <[email protected]>
2 Author: Andi Kleen <[email protected]>
2 Author: Andreas Dilger <[email protected]>
2 Author: Andreas Dilger <[email protected]>
2 Author: Andreas Dilger <[email protected]>
2 Author: Andreas Gruenbacher <[email protected]>
2 Author: Andrey Sidorov <[email protected]>
2 Author: Andr? Goddard Rosa <[email protected]>
2 Author: Ashish Sangwan <[email protected]>
2 Author: Ashish Sangwan <[email protected]>
2 Author: Badari Pulavarty <[email protected]>
2 Author: Bernd Schubert <[email protected]>
2 Author: Boaz Harrosh <[email protected]>
2 Author: Chao Yu <[email protected]>
2 Author: Coly Li <[email protected]>
2 Author: Dan Ehrenberg <[email protected]>
2 Author: Daniel Mack <[email protected]>
2 Author: Dave Hansen <[email protected]>
2 Author: Dave Kleikamp <[email protected]>
2 Author: Deepa Dinamani <[email protected]>
2 Author: Dennis Zhou <[email protected]>
2 Author: Dmitry Monakhov <[email protected]>
2 Author: Eric Paris <[email protected]>
2 Author: Eryu Guan <[email protected]>
2 Author: Fengguang Wu <[email protected]>
2 Author: Goldwyn Rodrigues <[email protected]>
2 Author: Herbert Xu <[email protected]>
2 Author: Hisashi Hifumi <[email protected]>
2 Author: Ingo Molnar <[email protected]>
2 Author: Jakub Wilk <[email protected]>
2 Author: Jan Blunck <[email protected]>
2 Author: Jan Blunck <[email protected]>
2 Author: Jens Axboe <[email protected]>
2 Author: Jens Axboe <[email protected]>
2 Author: Jens Axboe <[email protected]>
2 Author: Jens Axboe <[email protected]>
2 Author: Jie Liu <[email protected]>
2 Author: Kalpak Shah <[email protected]>
2 Author: Kent Overstreet <[email protected]>
2 Author: Khazhismel Kumykov <[email protected]>
2 Author: Kirill Tkhai <[email protected]>
2 Author: Laurent Navet <[email protected]>
2 Author: Marcin Slusarz <[email protected]>
2 Author: Markus Elfring <[email protected]>
2 Author: Mel Gorman <[email protected]>
2 Author: Mel Gorman <[email protected]>
2 Author: Ming Lei <[email protected]>
2 Author: Namhyung Kim <[email protected]>
2 Author: Nikanth Karthikesan <[email protected]>
2 Author: Nikitas Angelinas <[email protected]>
2 Author: Pan Bian <[email protected]>
2 Author: Paul Bolle <[email protected]>
2 Author: Richard Weinberger <[email protected]>
2 Author: Roel Kluin <[email protected]>
2 Author: Roman Pen <[email protected]>
2 Author: Salman Qazi <[email protected]>
2 Author: Sergey Senozhatsky <[email protected]>
2 Author: Shaohua Li <[email protected]>
2 Author: Solofo Ramangalahy <[email protected]>
2 Author: Souptick Joarder <[email protected]>
2 Author: Thadeu Lima de Souza Cascardo <[email protected]>
2 Author: Tobias Klauser <[email protected]>
2 Author: Vasily Averin <[email protected]>
2 Author: Vivek Haldar <[email protected]>
2 Author: Wang Sheng-Hui <[email protected]>
2 Author: Wei Yongjun <[email protected]>
2 Author: Yu Jian <[email protected]>
2 Author: Zhang Zhen <[email protected]>
1 Author: Aaro Koskinen <[email protected]>
1 Author: Adam Borowski <[email protected]>
1 Author: Adrian Bunk <[email protected]>
1 Author: Adrian Bunk <[email protected]>
1 Author: Aihua Zhang <[email protected]>
1 Author: Akria Fujita <[email protected]>
1 Author: Alan Cox <[email protected]>
1 Author: Ales Novak <[email protected]>
1 Author: Alessio Igor Bogani <[email protected]>
1 Author: Alexander V. Lukyanov <[email protected]>
1 Author: Alexey Khoroshilov <[email protected]>
1 Author: Amerigo Wang <[email protected]>
1 Author: Amir G <[email protected]>
1 Author: Anand Gadiyar <[email protected]>
1 Author: Andi Shyti <[email protected]>
1 Author: Andrea Arcangeli <[email protected]>
1 Author: Andreas Dilger <[email protected]>
1 Author: Andreas Schlick <[email protected]>
1 Author: Andrew Perepechko <[email protected]>
1 Author: Andrey Savochkin <[email protected]>
1 Author: Andrey Tsyvarev <[email protected]>
1 Author: Andries E. Brouwer <[email protected]>
1 Author: Andy Leiserson <[email protected]>
1 Author: Andy Lutomirski <[email protected]>
1 Author: Andy Lutomirski <[email protected]>
1 Author: Andy Shevchenko <[email protected]>
1 Author: Aneesh Kumar K.V <[email protected]>
1 Author: Anton Protopopov <[email protected]>
1 Author: Arjan van de Ven <[email protected]>
1 Author: Artem Blagodarenko <[email protected]>
1 Author: Barret Rhoden <[email protected]>
1 Author: Bartlomiej Zolnierkiewicz <[email protected]>
1 Author: Benoit Boissinot <[email protected]>
1 Author: Bobi Jam <[email protected]>
1 Author: Borislav Petkov <[email protected]>
1 Author: BoxiLiu <[email protected]>
1 Author: Brian Foster <[email protected]>
1 Author: Bryan Donlan <[email protected]>
1 Author: Chanho Park <[email protected]>
1 Author: Chen Gang <[email protected]>
1 Author: Chen Gang <[email protected]>
1 Author: Christian Borntraeger <[email protected]>
1 Author: Christoph Lameter <[email protected]>
1 Author: Christoph Lameter <[email protected]>
1 Author: Chuck Ebbert <[email protected]>
1 Author: Cody P Schafer <[email protected]>
1 Author: Coly Li <[email protected]>
1 Author: Cong Ding <[email protected]>
1 Author: Cyrill Gorcunov <[email protected]>
1 Author: Damien Guibouret <[email protected]>
1 Author: Dan Magenheimer <[email protected]>
1 Author: Dave Jones <[email protected]>
1 Author: David Moore <[email protected]>
1 Author: David Turner <[email protected]>
1 Author: David Windsor <[email protected]>
1 Author: Davide Italiano <[email protected]>
1 Author: Debabrata Banerjee <[email protected]>
1 Author: Denis V. Lunev <[email protected]>
1 Author: Dennis Zhou (Facebook) <[email protected]>
1 Author: Dmitri Monakho <[email protected]>
1 Author: Dmitriy Monakhov <[email protected]>
1 Author: Dmitriy Monakhov <[email protected]>
1 Author: Dmitry Mishin <[email protected]>
1 Author: Dr. Tilmann Bubeck <[email protected]>
1 Author: Eldad Zack <[email protected]>
1 Author: Emese Revfy <[email protected]>
1 Author: Emoly Liu <[email protected]>
1 Author: Eric Dumazet <[email protected]>
1 Author: Eric Engestrom <[email protected]>
1 Author: Eric Sesterhenn <[email protected]>
1 Author: Ernesto A. Fern?ndez <[email protected]>
1 Author: Eugene Shatokhin <[email protected]>
1 Author: Fabrice Jouhaud <[email protected]>
1 Author: Fan Yong <[email protected]>
1 Author: Feng Tang <[email protected]>
1 Author: Fengguang Wu <[email protected]>
1 Author: Forrest Liu <[email protected]>
1 Author: From: Thiemo Nagel <[email protected]>
1 Author: Geert Uytterhoeven <[email protected]>
1 Author: Geliang Tang <[email protected]>
1 Author: Giedrius Rekasius <[email protected]>
1 Author: Gioh Kim <[email protected]>
1 Author: Girish Shilamkar <[email protected]>
1 Author: Goffredo Baroncelli <[email protected]>
1 Author: Greg Harm <[email protected]>
1 Author: Greg Kroah-Hartman <[email protected]>
1 Author: Gustavo A. R. Silva <[email protected]>
1 Author: Haibo Liu <[email protected]>
1 Author: HaiboLiu <[email protected]>
1 Author: Haogang Chen <[email protected]>
1 Author: Harshad Shirwadkar <[email protected]>
1 Author: Harvey Harrison <[email protected]>
1 Author: Heiko Carstens <[email protected]>
1 Author: Herton Ronaldo Krzesinski <[email protected]>
1 Author: Hiroshi Shimamoto <[email protected]>
1 Author: Huaitong Han <[email protected]>
1 Author: Huang Weiyi <[email protected]>
1 Author: Hugh Dickins <[email protected]>
1 Author: Ingo Molnar <[email protected]>
1 Author: Insu Yun <[email protected]>
1 Author: J. Bruce Fields <[email protected]>
1 Author: Jan Mrazek <[email protected]>
1 Author: Jason A. Donenfeld <[email protected]>
1 Author: Jason Yan <[email protected]>
1 Author: Jean Delvare <[email protected]>
1 Author: Jean Delvare <[email protected]>
1 Author: Jean Noel Cordenner <[email protected]>
1 Author: Jeremy Cline <[email protected]>
1 Author: Jerry Lee <[email protected]>
1 Author: Jesper Dangaard Brouer <[email protected]>
1 Author: Jesper Juhl <[email protected]>
1 Author: Jiri Slaby <[email protected]>
1 Author: Jiufei Xue <[email protected]>
1 Author: Johann Lombardi <[email protected]>
1 Author: Johann Lombardi <[email protected]>
1 Author: Johann Lombardi <[email protected]>
1 Author: Johannes Weiner <[email protected]>
1 Author: John Stultz <[email protected]>
1 Author: Jon Derrick <[email protected]>
1 Author: Jon Ernst <[email protected]>
1 Author: Josef "Jeff" Sipek <[email protected]>
1 Author: Josef 'Jeff' Sipek <[email protected]>
1 Author: Josef Bacik <[email protected]>
1 Author: Julia Lawall <[email protected]>
1 Author: Jun Piao <[email protected]>
1 Author: Junho Ryu <[email protected]>
1 Author: Junichi Uekawa <[email protected]>
1 Author: Justin P. Mattock <[email protected]>
1 Author: Kaho Ng <[email protected]>
1 Author: Kalpak Shah <[email protected]>
1 Author: Kent Overstreet <[email protected]>
1 Author: Kent Overstreet <[email protected]>
1 Author: Kimberly Brown <[email protected]>
1 Author: Kirill Korotaev <[email protected]>
1 Author: Konstantin Khlebnikov <[email protected]>
1 Author: Lachlan McIlroy <[email protected]>
1 Author: Laurent Vivier <[email protected]>
1 Author: Leonard Michlmayr <[email protected]>
1 Author: Li Dongyang <[email protected]>
1 Author: Linus Torvalds <[email protected]>
1 Author: Liu Song <[email protected]>
1 Author: Liu Xiang <[email protected]>
1 Author: Lucas De Marchi <[email protected]>
1 Author: Luis R. Rodriguez <[email protected]>
1 Author: Maarten ter Huurne <[email protected]>
1 Author: Maciej Żenczykowski <[email protected]>
1 Author: Maninder Singh <[email protected]>
1 Author: Mariusz Kozlowski <[email protected]>
1 Author: Markus Rechberger <[email protected]>
1 Author: Markus Trippelsdorf <[email protected]>
1 Author: Martin K. Petersen <[email protected]>
1 Author: Masahiro Yamada <[email protected]>
1 Author: Mathias Krause <[email protected]>
1 Author: Mathieu Desnoyers <[email protected]>
1 Author: Matt LaPlante <[email protected]>
1 Author: Matthew Garrett <[email protected]>
1 Author: Maxim Patlasov <[email protected]>
1 Author: Maxim Patlasov <[email protected]>
1 Author: Miao Xie <[email protected]>
1 Author: Michael Callahan <[email protected]>
1 Author: Michael Tokarev <[email protected]>
1 Author: Michal Hocko <[email protected]>
1 Author: Miguel Ojeda <[email protected]>
1 Author: Mike Snitzer <[email protected]>
1 Author: Mikulas Patocka <[email protected]>
1 Author: Mimi Zohar <[email protected]>
1 Author: Ming Lei <[email protected]>
1 Author: Namjae Jeon <[email protected]>
1 Author: Nicolas Kaiser <[email protected]>
1 Author: Nikolay Borisov <[email protected]>
1 Author: Niu Yawei <[email protected]>
1 Author: Pankaj Gupta <[email protected]>
1 Author: Patrick J. LoPresti <[email protected]>
1 Author: Patrick Palka <[email protected]>
1 Author: Paul Gortmaker <[email protected]>
1 Author: Paul Mackerras <[email protected]>
1 Author: Paul Mundt <[email protected]>
1 Author: Paul Taysom <[email protected]>
1 Author: Peter Huewe <[email protected]>
1 Author: Peter Zijlstra <[email protected]>
1 Author: Petros Koutoupis <[email protected]>
1 Author: Philippe De Muyter <[email protected]>
1 Author: Piotr Sarna <[email protected]>
1 Author: Pranay Kr. Srivastava <[email protected]>
1 Author: Rakesh Pandit <[email protected]>
1 Author: Randy Dodgen <[email protected]>
1 Author: Randy Dunlap <[email protected]>
1 Author: Richard Kennedy <[email protected]>
1 Author: Robert P. J. Day <[email protected]>
1 Author: Ross Zwisler <[email protected]>
1 Author: Rusty Russell <[email protected]>
1 Author: Sachin Kamat <[email protected]>
1 Author: Sahitya Tummala <[email protected]>
1 Author: Santosh Nayak <[email protected]>
1 Author: Satyam Sharma <[email protected]>
1 Author: Sean Fu <[email protected]>
1 Author: Serge E. Hallyn <[email protected]>
1 Author: Sergey Karamov <[email protected]>
1 Author: Seth Forshee <[email protected]>
1 Author: Seunghun Lee <[email protected]>
1 Author: Sheng Yong <[email protected]>
1 Author: Shi Siyuan <[email protected]>
1 Author: Simon Ruderich <[email protected]>
1 Author: [email protected] <>
1 Author: Sriram Rajagopalan <[email protected]>
1 Author: Stephen Hemminger <[email protected]>
1 Author: Stephen Hemminger <[email protected]>
1 Author: Steven Liu <[email protected]>
1 Author: Steven Whitehouse <[email protected]>
1 Author: Stoyan Gaydarov <[email protected]>
1 Author: Suparna Bhattacharya <[email protected]>
1 Author: Surbhi Palande <[email protected]>
1 Author: T Makphaibulchoke <[email protected]>
1 Author: Takashi Sato <[email protected]>
1 Author: Takashi Sato <[email protected]>
1 Author: Tao Ma <[email protected]>
1 Author: Theodore Tso <[email protected]>
1 Author: Thomas Gleixner <[email protected]>
1 Author: Tiger Yang <[email protected]>
1 Author: Tim Schmielau <[email protected]>
1 Author: Utako Kusaka <[email protected]>
1 Author: Vahram Martirosyan <[email protected]>
1 Author: Valerie Aurora <[email protected]>
1 Author: Venkatesh Pallipadi <[email protected]>
1 Author: Vignesh Babu <[email protected]>
1 Author: Vincent Minet <[email protected]>
1 Author: Viresh Kumar <[email protected]>
1 Author: Vladimir Davydov <[email protected]>
1 Author: Wang Sheng-Hui <[email protected]>
1 Author: Wang Shilong <[email protected]>
1 Author: Wang Shilong <[email protected]>
1 Author: Wei Yuan <[email protected]>
1 Author: Xi Wang <[email protected]>
1 Author: Xiaoguang Wang <[email protected]>
1 Author: Xu Cang <[email protected]>
1 Author: Yan, Zheng <[email protected]>
1 Author: Yang Ruirui <[email protected]>
1 Author: Yaowei Bai <[email protected]>
1 Author: Yasunori Goto <[email protected]>
1 Author: Younger Liu <[email protected]>
1 Author: ZhangXiaoxu <[email protected]>
1 Author: Zhao Hongjiang <[email protected]>
1 Author: Zhi Yong Wu <[email protected]>
1 Author: Zhouyi Zhou <[email protected]>
1 Author: boxi liu <[email protected]>
1 Author: gmail <[email protected]>
1 Author: harshads <[email protected]>
1 Author: [email protected] (Jiaying Zhang) <>
1 Author: jon ernst <[email protected]>
1 Author: liang xie <[email protected]>
1 Author: piaojun <[email protected]>
1 Author: ruippan (潘睿) <[email protected]>
1 Author: vignesh babu <[email protected]>
1 Author: vikram.jadhav07 <[email protected]>
1 Author: wangguang <[email protected]>
1 Author: yalin wang <[email protected]>
1 Author: zhangjs <[email protected]>
1 Author: zhenwei.pi <[email protected]>
1 Author: zhong jiang <[email protected]>
1 Author: zilong.liu <[email protected]>



Attachments:
(No filename) (849.00 B)

2019-08-31 14:31:10

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

On Sat, Aug 31, 2019 at 1:26 PM Valdis Klētnieks
<[email protected]> 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.


--
With Best Regards,
Andy Shevchenko

2019-08-31 14:52:24

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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...


Attachments:
(No filename) (849.00 B)

2019-09-01 01:08:37

by Dave Chinner

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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.
--
Dave Chinner
[email protected]

2019-09-01 01:42:27

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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

2019-09-01 03:08:21

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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...

2019-09-01 03:27:59

by Gao Xiang

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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/[email protected]/
[2] https://lore.kernel.org/linux-fsdevel/20190831064853.GA162401@architecture4/

Thanks,
Gao Xiang

2019-09-01 03:39:31

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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 ;)



Attachments:
(No filename) (849.00 B)

2019-09-01 22:52:18

by Dave Chinner

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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.
--
Dave Chinner
[email protected]

2019-09-01 23:15:49

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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. :)


Attachments:
(No filename) (849.00 B)

2019-09-02 08:17:45

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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.

2019-09-02 08:33:10

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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.

2019-09-02 15:35:51

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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

2019-09-02 19:02:49

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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?


Attachments:
(No filename) (849.00 B)

2019-09-02 19:07:28

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

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

2019-09-08 13:01:01

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat

Greg Kroah-Hartman <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>