RE: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
> From: David Sterba > Sent: Friday, August 14, 2020 4:41 PM > On Fri, Aug 14, 2020 at 12:29:01PM +, Konstantin Komarov wrote: > > This patch adds NTFS Read-Write driver to fs/ntfs3. > > > > Having decades of expertise in commercial file systems development and huge > > test coverage, we at Paragon Software GmbH want to make our contribution to > > the Open Source Community by providing implementation of NTFS Read-Write > > driver for the Linux Kernel. > > > > This is fully functional NTFS Read-Write driver. Current version works with > > NTFS(including v3.1) normal/compressed/sparse files and supports journal > > replaying. > > > > We plan to support this version after the codebase once merged, and add new > > features and fix bugs. For example, full journaling support over JBD will be > > added in later updates. > > > > The patch is too big to handle it within an e-mail body, so it is available > > to download > > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch > > > > Signed-off-by: Konstantin Komarov > > In case somebody wants to compile it, this fixup is needed to let 'make > fs/ntfs3/' actually work, besides enabling it in the config. > > diff --git a/fs/Makefile b/fs/Makefile > index 1c7b0e3f6daa..b0b4ad8affa0 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/ > obj-$(CONFIG_CIFS) += cifs/ > obj-$(CONFIG_HPFS_FS)+= hpfs/ > obj-$(CONFIG_NTFS_FS)+= ntfs/ > +obj-$(CONFIG_NTFS3_FS) += ntfs3/ > obj-$(CONFIG_UFS_FS) += ufs/ > obj-$(CONFIG_EFS_FS) += efs/ > obj-$(CONFIG_JFFS2_FS) += jffs2/ > diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile > index 4d4fe198b8b8..d99dd1af43aa 100644 > --- a/fs/ntfs3/Makefile > +++ b/fs/ntfs3/Makefile > @@ -5,7 +5,7 @@ > > obj-$(CONFIG_NTFS3_FS) += ntfs3.o > > -ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ > +ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ > index.o attrlist.o record.o attrib.o run.o xattr.o\ > upcase.o super.o file.o dir.o namei.o lznt.o\ > fslog.o > --- Hi David, thanks! Indeed these fixups are needed to the patch (lost them during final polishing of the code before submitting). Will be fixed in v2.
RE: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
From: David Sterba Sent: Friday, August 14, 2020 4:41 PM > In case somebody wants to compile it, this fixup is needed to let 'make > fs/ntfs3/' actually work, besides enabling it in the config. > > diff --git a/fs/Makefile b/fs/Makefile > index 1c7b0e3f6daa..b0b4ad8affa0 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/ > obj-$(CONFIG_CIFS) += cifs/ > obj-$(CONFIG_HPFS_FS)+= hpfs/ > obj-$(CONFIG_NTFS_FS)+= ntfs/ > +obj-$(CONFIG_NTFS3_FS) += ntfs3/ > obj-$(CONFIG_UFS_FS) += ufs/ > obj-$(CONFIG_EFS_FS) += efs/ > obj-$(CONFIG_JFFS2_FS) += jffs2/ > diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile > index 4d4fe198b8b8..d99dd1af43aa 100644 > --- a/fs/ntfs3/Makefile > +++ b/fs/ntfs3/Makefile > @@ -5,7 +5,7 @@ > > obj-$(CONFIG_NTFS3_FS) += ntfs3.o > > -ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ > +ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ > index.o attrlist.o record.o attrib.o run.o xattr.o\ > upcase.o super.o file.o dir.o namei.o lznt.o\ > fslog.o Thanks! Indeed these fixups are needed to the patch (lost them during final polishing of the code before submitting). Will be fixed in v2.
RE: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
From: David Sterba Sent: Saturday, August 15, 2020 10:07 PM > > 1. check existing support in kernel > > There is fs/ntfs with read-only support from Tuxera, last change in the > git tree is 3 years ago. The driver lacks read-write support so there's > not 100% functionality duplication but there's still driver for the same > filesystem. > > I don't know what's the feature parity, how much the in-kernel driver is > used (or what are business relations of Tuxera and Paragon), compared to > the FUSE-based driver, but ideally there's just one NTFS driver in linux > kernel. > > The question I'd ask: > > - what are users of current fs/ntfs driver going to lose, if the driver > is going to be completely replaced by your driver > > If the answer is 'nothing' then, the straightfowrad plan is to just > replace it. Otherwise, we'll have to figure that out. Hi! Regarding the comparison with fs/ntfs driver - we of course did the analysis. There are two main points which make the difference: full read-write support (including compressed/sparse files) and journal replaying. The one thing which is missing in fs/ntfs3 in the inital patch is the appropriate processing of hybernated volumes (mounting them read-only to avoid corruptions). This, however, is considered as trivial change and will be added either in v2, or in v3. In general, we want to have the community's feedback on the topic, and if it's more suitable for the Linux Kernel not to have two implementations in Kernel, then the 'fs/ntfs' may be replaced. > > 2. split the patch > > One patch of 27k lines of code is way too much to anybody to look at. The patch will be splitted in v2 file-wise. Wasn't clear initially which way will be more convenient to review. > 3. determine support status > > You state intentions to work on the driver and there's a new entry in > MAINTAINERS file with 'Supported', so that's a good sign that it's not > just one-time dump of code. Fixing bugs or adding functionality is > expected. > > 4. development git tree > > Once the filesystem is merged, you'd be getting mails from people > running eg. static checkers, build tests, sending cleanups or other > tree-wide changes. The entry in MAINTAINER file does not point to any > git tree, so that's not clear to me what's the expected way to get the > fixes to Linus' tree or where are people supposed to look for 'is this > fixed already'. The external git repo for the code is currently being prepared, so it's the matter of time to have it. Will add the appropriate links to the MAINTERS once done.
RE: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
From: Aurélien Aptel Sent: Friday, August 14, 2020 6:30 PM > I've tried this using libntfs-3g mkfs.ntfs > > # mkfs.ntfs /dev/vb1 > # mount -t ntfs3 /dev/vb1 /mnt > > This already triggered UBSAN: > Then I've tried to copy /etc into it: > ... > # cp -rp /etc /mnt > > But this triggered a NULL ptr deref: > > BUG: kernel NULL pointer dereference, address: 0028 Thanks! This will be fixed in v2. To give some context: we use our mkfs utility in tests, coupled with a Windows-native one as a reference. P.S. Already have extended this approach with the current mkfs.ntfs utility as well.
RE: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
From: Aurélien Aptel Sent: Friday, August 14, 2020 5:09 PM > > Hi Konstantin, > > That's cool :) As Nikolay said it needs a little change to the makefiles > to even build. > > Are you also going to publish your own mkfs.ntfs3 tool? I dont think the > existing one would support 64k clusters. Hi Aurélien. Thanks for your feedback. We plan to publish our mkfs.ntfs utility as the open source as well (and possibly fschk.ntfs - after mkfs). > > I would recommend to run checkpatch (I see already 87 warnings... some > of it is noise): > > $ ./scripts/checkpatch.pl > > And sparse (I dont see much): > > $ touch fs/ntfs3/*.[ch] && make C=1 > > You need a recent build of sparse to do that last one. You can pass your > own sparse bin (make CHECK=~/prog/sparse/sparse C=1) > > This will be a good first step. The sparse utility is running against the code, as well as checkpatch.pl. Sprase output is clean now. Checkpatch's somehow important warnings will be fixed in v2 (except maybe typedefs). > > Have you tried to run the xfstests suite against it? > xfstests are being one of our standard test suites among others. Currently we have the 'generic/339' and 'generic/013' test cases failing, working on it now. Other tests either pass or being skipped (due to missing features e.g. reflink). > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
On Friday 14 August 2020 12:29:01 Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) normal/compressed/sparse files and supports journal > replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. > > The patch is too big to handle it within an e-mail body, so it is available > to download > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch > > Signed-off-by: Konstantin Komarov Hello Konstantin! I agree with David that patch is big and could be split into smaller patches per file, like exfat driver was reviewed. When you send a new version of ntfs driver, please add me to CC and I will try to find some time to do code review. Thanks!
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
On Sat, Aug 15, 2020 at 09:06:42PM +0200, David Sterba wrote: > There's maybe more I missed, but hopefully HTH. One thing you missed is adding support to fstests git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git If it passes that torture test, I think we can have confidence that this is a really good implementation of a filesystem.
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
On Fri, Aug 14, 2020 at 12:29:01PM +, Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) normal/compressed/sparse files and supports journal > replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. > > The patch is too big to handle it within an e-mail body, so it is available > to download > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch The way you submit the driver does not meet significant number of requirements documented in https://www.kernel.org/doc/html/latest/process/submitting-patches.html . so this may lead to ignoring the patch as this puts the burden on the kernel community to make the merge somehow happen. I don't see kernel involvement from Paragon, so let me build our half of the bridge. As I reckon, there is interest to have NTFS implementation that's better than the existing FUSE based support by NTFS-3G (namely performance), so let me propose something that might lead to merging the patch eventually. 1. check existing support in kernel There is fs/ntfs with read-only support from Tuxera, last change in the git tree is 3 years ago. The driver lacks read-write support so there's not 100% functionality duplication but there's still driver for the same filesystem. I don't know what's the feature parity, how much the in-kernel driver is used (or what are business relations of Tuxera and Paragon), compared to the FUSE-based driver, but ideally there's just one NTFS driver in linux kernel. The question I'd ask: - what are users of current fs/ntfs driver going to lose, if the driver is going to be completely replaced by your driver If the answer is 'nothing' then, the straightfowrad plan is to just replace it. Otherwise, we'll have to figure that out. 2. split the patch One patch of 27k lines of code is way too much to anybody to look at. As an example, what worked for the recent EXFAT support, send each new file as a patch. This will pass the mailinglist size filters and keep the patches logically separated, so eg. there are smaller patches implementing interaction with VFS (on the inode or directory level) and leaving out the other internal stuff of the filesystem. I'm counting about 20 files, that's acceptable. The last patch, or maybe a separate patch, adds the actual build and config-time support. The situation is a bit more complicated as there's an existing driver and I don't see a clear way how to do the transition from 2 implementations (one intermediate) to just one in the final state. We have that already with the old VFAT and new EXFAT drivers, and it's solved on the module level. Just one can be loaded (AFAIK). The same could be done here but it puts some burden on users to know what driver to load to get the expected set of features. 3. determine support status You state intentions to work on the driver and there's a new entry in MAINTAINERS file with 'Supported', so that's a good sign that it's not just one-time dump of code. Fixing bugs or adding functionality is expected. 4. development git tree Once the filesystem is merged, you'd be getting mails from people running eg. static checkers, build tests, sending cleanups or other tree-wide changes. The entry in MAINTAINER file does not point to any git tree, so that's not clear to me what's the expected way to get the fixes to Linus' tree or where are people supposed to look for 'is this fixed already'. There's maybe more I missed, but hopefully HTH. d.
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
I've tried this using libntfs-3g mkfs.ntfs
# mkfs.ntfs /dev/vb1
# mount -t ntfs3 /dev/vb1 /mnt
This already triggered UBSAN:
UBSAN: object-size-mismatch in fs/ntfs3/super.c:834:16
load of address 6ae096b5 with insufficient space
for an object of type 'const char'
CPU: 3 PID: 1248 Comm: mount Not tainted 5.8.0+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
Call Trace:
dump_stack+0x78/0xa0
ubsan_epilogue+0x5/0x40
ubsan_type_mismatch_common.cold+0xc8/0xcd
__ubsan_handle_type_mismatch_v1+0x32/0x37
ntfs_fill_super+0x9f/0x13e0
? vsnprintf+0x1ef/0x4f0
mount_bdev+0x193/0x1c0
Which points to:
sb->s_magic = *(unsigned long *)s_magic; /* TODO */
Maybe store ('n'<<32)|('t'<<24)|('f'<<16)|('s'<<8) ?
Seems harmless.
* * *
Then I've tried to copy /etc into it:
# cp -rp /etc /mnt
But this triggered a NULL ptr deref:
BUG: kernel NULL pointer dereference, address: 0028
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
PGD 0 P4D 0
Oops: [#1] SMP NOPTI
CPU: 0 PID: 1255 Comm: cp Not tainted 5.8.0+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
RIP: 0010:ntfs_insert_security+0x187/0x4a0
Code: 00 48 83 c4 18 85 c0 0f 85 54 01 00 00 48 8b 44 24 50 49 8d b5 d8 01 00
00 8b 54 24 60 83 c3 14 48 89 74 24 30 48 85 c0 74 3a <39> 58 28 0f 84 7e 01 00
00 49 89 e8 48 8d 4c 24 50 4c 89 f2 4c 89
RSP: 0018:ac73403dfc58 EFLAGS: 00010246
RAX: RBX: 0064 RCX: 0010
RDX: 00b0 RSI: RDI:
RBP: 94154ed5fe00 R08: R09: 0001
R10: 9415781a6180 R11: 0003 R12: 94155c989800
R13: 94151e8d2a38 R14: 9415781a6170 R15: 9415781173f0
FS: 7fd19b86e580() GS:94157dc0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 0028 CR3: 1ac2a000 CR4: 00350ef0
Call Trace:
? mark_held_locks+0x49/0x70
? lockdep_hardirqs_on_prepare+0xf7/0x190
? ktime_get_coarse_real_ts64+0x9e/0xd0
? trace_hardirqs_on+0x1c/0xe0
ntfs_create_inode+0x2db/0x11c0
ntfs_mkdir+0x58/0x90
vfs_mkdir+0x109/0x1f0
do_mkdirat+0x81/0x120
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fd19ad54dd7
Code: 00 b8 ff ff ff ff c3 0f 1f 40 00 48 8b 05 b9 70 2c 00 64 c7 00 5f 00 00
00 b8 ff ff ff ff c3 0f 1f 40 00 b8 53 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01
c3 48 8b 0d 91 70 2c 00 f7 d8 64 89 01 48
RSP: 002b:7ffec3c41588 EFLAGS: 0206 ORIG_RAX: 0053
RAX: ffda RBX: 0001 RCX: 7fd19ad54dd7
RDX: 000c0001 RSI: 01c0 RDI: 55cad585fcf0
RBP: 7ffec3c41990 R08: 7ffec3c41b50 R09: 7fd19ada55c0
R10: 01ef R11: 0206 R12: 7ffec3c41b50
R13: R14: R15: 7ffec3c437be
(gdb) list *(ntfs_insert_security+0x187)
0x814e5097 is in ntfs_insert_security (fs/ntfs3/fsntfs.c:1811).
1806
1807if (!e)
1808goto insert_security;
1809
1810next_security:
1811if (le32_to_cpu(e->sec_hdr.size) != new_sec_size)
1812goto skip_read_sds;
1813
1814err = ntfs_read_run_nb(sbi, >file.run,
le64_to_cpu(e->sec_hdr.off),
1815 d_security, new_sec_size, NULL);
(gdb) disas /s ntfs_insert_security
1811if (le32_to_cpu(e->sec_hdr.size) != new_sec_size)
0x814e5097 <+391>: cmp%ebx,0x28(%rax) <=
0x814e509a <+394>: je 0x814e521e
(gdb) p/x (int)&((NTFS_DE_SDH*)0)->sec_hdr.size
$4 = 0x28
So I think 'e' is NULL. Not sure how it can happen.
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
Hi Konstantin, That's cool :) As Nikolay said it needs a little change to the makefiles to even build. Are you also going to publish your own mkfs.ntfs3 tool? I dont think the existing one would support 64k clusters. I would recommend to run checkpatch (I see already 87 warnings... some of it is noise): $ ./scripts/checkpatch.pl And sparse (I dont see much): $ touch fs/ntfs3/*.[ch] && make C=1 You need a recent build of sparse to do that last one. You can pass your own sparse bin (make CHECK=~/prog/sparse/sparse C=1) This will be a good first step. Have you tried to run the xfstests suite against it? Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
On Fri, Aug 14, 2020 at 12:29:01PM +, Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) normal/compressed/sparse files and supports journal > replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. > > The patch is too big to handle it within an e-mail body, so it is available > to download > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch > > Signed-off-by: Konstantin Komarov In case somebody wants to compile it, this fixup is needed to let 'make fs/ntfs3/' actually work, besides enabling it in the config. diff --git a/fs/Makefile b/fs/Makefile index 1c7b0e3f6daa..b0b4ad8affa0 100644 --- a/fs/Makefile +++ b/fs/Makefile @@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/ obj-$(CONFIG_CIFS) += cifs/ obj-$(CONFIG_HPFS_FS) += hpfs/ obj-$(CONFIG_NTFS_FS) += ntfs/ +obj-$(CONFIG_NTFS3_FS) += ntfs3/ obj-$(CONFIG_UFS_FS) += ufs/ obj-$(CONFIG_EFS_FS) += efs/ obj-$(CONFIG_JFFS2_FS) += jffs2/ diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile index 4d4fe198b8b8..d99dd1af43aa 100644 --- a/fs/ntfs3/Makefile +++ b/fs/ntfs3/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_NTFS3_FS) += ntfs3.o -ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ +ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \ index.o attrlist.o record.o attrib.o run.o xattr.o\ upcase.o super.o file.o dir.o namei.o lznt.o\ fslog.o ---
Re: [PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
On 14.08.20 г. 15:29 ч., Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) normal/compressed/sparse files and supports journal > replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. > > The patch is too big to handle it within an e-mail body, so it is available > to download > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch > > Signed-off-by: Konstantin Komarov > So how exactly do you expect someone to review this monstrosity ?
[PATCH] fs: NTFS read-write driver GPL implementation by Paragon Software.
This patch adds NTFS Read-Write driver to fs/ntfs3. Having decades of expertise in commercial file systems development and huge test coverage, we at Paragon Software GmbH want to make our contribution to the Open Source Community by providing implementation of NTFS Read-Write driver for the Linux Kernel. This is fully functional NTFS Read-Write driver. Current version works with NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying. We plan to support this version after the codebase once merged, and add new features and fix bugs. For example, full journaling support over JBD will be added in later updates. The patch is too big to handle it within an e-mail body, so it is available to download on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch Signed-off-by: Konstantin Komarov
