| Message ID | 20161004090041.GE17515@quack2.suse.cz |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > Never seen this but I suspect it is a fallout from Al's directory locking > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > directory entries in file->private_data (and generally modifies the > structure stored there) but after Al's changes we don't have exclusive > access to struct file if I'm right so if two processes end up calling > getdents() for the same 'struct file' we are doomed. I haven't seen it either, and I've been doing a lot of testing on the ext4 test branch. So I'm guessing Tony has the only reliable repro for the problem at the moment. That being said, it shouldn't be that hard to create a test case for this and add it to xfstests. I'm pretty sure Jan is right about this, though, but it would be great to a get a quick confirmation from Tony if at all possible. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Jan Kara <jack@suse.cz> [161004 02:01]: > Hi! > > On Mon 03-10-16 16:30:55, Tony Lindgren wrote: > > I'm seeing a repeatable oops with Linux next while running > > update-initramfs, see below. I tried reverting commit 59aa5a3aeead > > ("fscrypto: make filename crypto functions return 0 on success") > > as that's the only commit changing ext4_htree_store_dirent, but > > that did not help. > > > > Anybody else seeing something like this? > > Never seen this but I suspect it is a fallout from Al's directory locking > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > directory entries in file->private_data (and generally modifies the > structure stored there) but after Al's changes we don't have exclusive > access to struct file if I'm right so if two processes end up calling > getdents() for the same 'struct file' we are doomed. OK.. > That being said two getdents() calls for a single fd looks like a stupid > things to do but I don't see anything that would prevent this. Does the > attached patch fix the issue for you? Looks like with your patch I saw the oops few times during kernel boot already, see below. And the errors seem more random now.. I've verified I can run mkinitramfs just fine with my curent for-next that's based on 4.8.0-rc6, so this does not seem to be any memory or MMC related issue. Trying to bisect this would be a pain as the error does not happen every time.. Regards, Tony 8< ------------------- Unable to handle kernel NULL pointer dereference at virtual address 00000004 pgd = c22a46c0 [00000004] *pgd=00000000 Internal error: Oops: 205 [#1] SMP ARM Modules linked in: usbkbd usb_f_rndis usb_f_ecm usb_f_acm u_ether usb_f_mass_storage usb] CPU: 1 PID: 364 Comm: startpar Not tainted 4.8.0-rc8-next-20161003+ #1005 Hardware name: Generic OMAP5 (Flattened Device Tree) task: c2090ec0 task.stack: c25b2000 PC is at rb_insert_color+0x1c/0x1b8 LR is at ext4_htree_store_dirent+0xe0/0x104 pc : [<c0594b10>] lr : [<c042ade0>] psr: 600f0013 sp : c25b3db0 ip : c2297bd8 fp : c0a17618 r10: 00000000 r9 : c25b3e00 r8 : c2297bd0 r7 : c2298f80 r6 : 79de0293 r5 : 998623c8 r4 : 00000000 r3 : 00000000 r2 : c2297c08 r1 : c2298f80 r0 : c2297bd8 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 30c5387d Table: 822a46c0 DAC: 55555555 Process startpar (pid: 364, stack limit = 0xc25b2218) Stack: (0xc25b3db0 to 0xc25b4000) 3da0: 00000000 998623c8 79de0293 c042ade0 3dc0: e92460b4 ed080200 ed084790 c25b3e58 e92460bc e9246ff8 00000000 c043ee9c 3de0: c25b3e00 ed080200 e9246000 00001000 000000b4 c028a6e4 00000009 ee48a400 3e00: e92460bc 0000000d c1025cb4 c2298f80 ed0848c0 ed084790 00000000 ee48a400 3e20: 00000000 c25b3e58 c2257800 c043fc14 00000000 00000000 00000006 024080c0 3e40: c2298fa0 00000000 600f0013 c100253c c10936b4 c02871ac 998623c8 79de0293 3e60: 00000004 c22570b8 c2298f80 c0383f40 030d8fb9 69db434f c2298f80 c20aa000 3e80: 7f5c2000 c2298f80 ed0848c0 c25b3f68 ed084790 ed084830 ee48a400 ed084790 3ea0: c2257800 c042aa04 00000000 00000000 600f0013 c10930b0 00000001 c028c230 3ec0: 00000001 00000000 00000000 00000000 00000000 00000000 00000800 600f0013 3ee0: 00000000 00000002 00000000 ed084830 00000000 00000001 c03b4e3c ed084830 3f00: ee48a400 00000000 bef92f0c c08d4530 00000001 ee48a400 00000000 00000000 3f20: c25b3f68 ed084830 ee48a400 00000000 bef92f0c c03b4e70 ee48a400 bef90800 3f40: 7f59d01d 7f5ba6f0 00008000 00000004 000000d9 ee48a400 ee48a400 00000000 3f60: bef92f0c c03b54a0 c03b5124 00000000 00000000 00000000 7f5ba6f0 00000000 3f80: 00008000 00000000 00000020 00000020 7f5ba6d0 7f5ba6f0 000000d9 c02080c4 3fa0: c25b2000 c0207f40 00000020 7f5ba6d0 00000004 7f5ba6f0 00008000 00000000 3fc0: 00000020 7f5ba6d0 7f5ba6f0 000000d9 b6f15850 00000000 00000000 bef92f0c 3fe0: 000000d9 bef9080c b6e57fa3 b6e006f6 200f0030 00000004 00000000 00150072 [<c0594b10>] (rb_insert_color) from [<c042ade0>] (ext4_htree_store_dirent+0xe0/0x104) [<c042ade0>] (ext4_htree_store_dirent) from [<c043ee9c>] (htree_dirblock_to_tree+0xb0/0x) [<c043ee9c>] (htree_dirblock_to_tree) from [<c043fc14>] (ext4_htree_fill_tree+0x7c/0x2a0) [<c043fc14>] (ext4_htree_fill_tree) from [<c042aa04>] (ext4_readdir+0x62c/0x910) [<c042aa04>] (ext4_readdir) from [<c03b4e70>] (iterate_dir+0xcc/0x194) [<c03b4e70>] (iterate_dir) from [<c03b54a0>] (SyS_getdents64+0x7c/0x10c) [<c03b54a0>] (SyS_getdents64) from [<c0207f40>] (ret_fast_syscall+0x0/0x1c) Code: e5923000 e3130001 1a000062 e92d4070 (e593c004) ---[ end trace 8970a95866a25898 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Theodore Ts'o <tytso@mit.edu> [161004 07:04]: > On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > > Never seen this but I suspect it is a fallout from Al's directory locking > > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > > directory entries in file->private_data (and generally modifies the > > structure stored there) but after Al's changes we don't have exclusive > > access to struct file if I'm right so if two processes end up calling > > getdents() for the same 'struct file' we are doomed. > > I haven't seen it either, and I've been doing a lot of testing on the > ext4 test branch. So I'm guessing Tony has the only reliable repro > for the problem at the moment. That being said, it shouldn't be that > hard to create a test case for this and add it to xfstests. > > I'm pretty sure Jan is right about this, though, but it would be great > to a get a quick confirmation from Tony if at all possible. Looks like Jan's patch somehow makes things worse for me. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > Hi! > > On Mon 03-10-16 16:30:55, Tony Lindgren wrote: > > I'm seeing a repeatable oops with Linux next while running > > update-initramfs, see below. I tried reverting commit 59aa5a3aeead > > ("fscrypto: make filename crypto functions return 0 on success") > > as that's the only commit changing ext4_htree_store_dirent, but > > that did not help. > > > > Anybody else seeing something like this? > > Never seen this but I suspect it is a fallout from Al's directory locking > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > directory entries in file->private_data (and generally modifies the > structure stored there) but after Al's changes we don't have exclusive > access to struct file if I'm right so if two processes end up calling > getdents() for the same 'struct file' we are doomed. RTFS. We sure as hell *do* have exclusive access to struct file. See /* POSIX.1-2008/SUSv4 Section XSI 2.9.7 */ if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) f->f_mode |= FMODE_ATOMIC_POS; in do_dentry_open() as well as if (file && (file->f_mode & FMODE_ATOMIC_POS)) { if (file_count(file) > 1) { v |= FDPUT_POS_UNLOCK; mutex_lock(&file->f_pos_lock); } } in __fdget_pos() and f = fdget_pos(fd); if (!f.file) return -EBADF; in getdents(2). -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 04, 2016 at 10:02:31AM -0400, Theodore Ts'o wrote: > On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > > Never seen this but I suspect it is a fallout from Al's directory locking > > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > > directory entries in file->private_data (and generally modifies the > > structure stored there) but after Al's changes we don't have exclusive > > access to struct file if I'm right so if two processes end up calling > > getdents() for the same 'struct file' we are doomed. > > I haven't seen it either, and I've been doing a lot of testing on the > ext4 test branch. So I'm guessing Tony has the only reliable repro > for the problem at the moment. That being said, it shouldn't be that > hard to create a test case for this and add it to xfstests. > > I'm pretty sure Jan is right about this, though, but it would be great > to a get a quick confirmation from Tony if at all possible. Jan is wrong - we do have per-struct-file serialization for getdents() et.al. It might be a race between getdents() on *different* struct file for the same directory, but ->private_data is not a problem. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 04, 2016 at 03:59:15PM +0100, Al Viro wrote: > Jan is wrong - we do have per-struct-file serialization for getdents() > et.al. It might be a race between getdents() on *different* struct > file for the same directory, but ->private_data is not a problem. So the rb_insert_color() OOPS seems to indicate that the rbtree has gotten corrupted, and it hangs off of private_data. And I've just checked the ext4.git tree and I can't see any changes in the dev branch (which I was just about to push to Linus) that would impact the readdir path for non-encrypted directories. So if it's a race on ->private_data I'm not sure what else might be going wrong. Hmm... Tony, what version was the last good version where it wouldn't reproduce for you? v4.7? Does it reproduce for you on v4.8? Also, would you mind seeing if you can reproduce it on the dev branch of the ext4 git tree? git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev Thanks, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, * Al Viro <viro@ZenIV.linux.org.uk> [161004 08:00]: > On Tue, Oct 04, 2016 at 10:02:31AM -0400, Theodore Ts'o wrote: > > On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > > > Never seen this but I suspect it is a fallout from Al's directory locking > > > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > > > directory entries in file->private_data (and generally modifies the > > > structure stored there) but after Al's changes we don't have exclusive > > > access to struct file if I'm right so if two processes end up calling > > > getdents() for the same 'struct file' we are doomed. > > > > I haven't seen it either, and I've been doing a lot of testing on the > > ext4 test branch. So I'm guessing Tony has the only reliable repro > > for the problem at the moment. That being said, it shouldn't be that > > hard to create a test case for this and add it to xfstests. > > > > I'm pretty sure Jan is right about this, though, but it would be great > > to a get a quick confirmation from Tony if at all possible. > > Jan is wrong - we do have per-struct-file serialization for getdents() > et.al. It might be a race between getdents() on *different* struct > file for the same directory, but ->private_data is not a problem. OK found the guilty person after git bisect and that's me. Git bisect points to commit d776fc86b82f ("wlcore: sdio: Populate config firmware data"), so adding Kalle to Cc. Looks like update-initramfs does rmmod of wlcore_sdio and that triggers some issue with the wlcore driver or with SDIO/MMC. Or maybe it's a memory corruption issue. I don't know yet exactly what's going on here yet but I plan to find out after some lunch. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Theodore Ts'o <tytso@mit.edu> [161004 12:08]: > On Tue, Oct 04, 2016 at 03:59:15PM +0100, Al Viro wrote: > > Jan is wrong - we do have per-struct-file serialization for getdents() > > et.al. It might be a race between getdents() on *different* struct > > file for the same directory, but ->private_data is not a problem. > > So the rb_insert_color() OOPS seems to indicate that the rbtree has > gotten corrupted, and it hangs off of private_data. Yes so it seems.. > And I've just checked the ext4.git tree and I can't see any changes in > the dev branch (which I was just about to push to Linus) that would > impact the readdir path for non-encrypted directories. So if it's a > race on ->private_data I'm not sure what else might be going wrong. > > Hmm... Tony, what version was the last good version where it wouldn't > reproduce for you? v4.7? Does it reproduce for you on v4.8? Well I just found it's caused by my commit d776fc86b82f ("wlcore: sdio: Populate config firmware data") and has nothing to do with ext4, see the mail I just posted. > Also, would you mind seeing if you can reproduce it on the dev > branch of the ext4 git tree? > > git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev Seems like there's nothing wrong with ext4, this is probably some memory corruption issue. Thanks everybody for help and sorry for the false ext4 alert. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 04-10-16 15:56:39, Al Viro wrote: > On Tue, Oct 04, 2016 at 11:00:41AM +0200, Jan Kara wrote: > > Hi! > > > > On Mon 03-10-16 16:30:55, Tony Lindgren wrote: > > > I'm seeing a repeatable oops with Linux next while running > > > update-initramfs, see below. I tried reverting commit 59aa5a3aeead > > > ("fscrypto: make filename crypto functions return 0 on success") > > > as that's the only commit changing ext4_htree_store_dirent, but > > > that did not help. > > > > > > Anybody else seeing something like this? > > > > Never seen this but I suspect it is a fallout from Al's directory locking > > changes. In particular ext4_htree_fill_tree() builds rb-tree of found > > directory entries in file->private_data (and generally modifies the > > structure stored there) but after Al's changes we don't have exclusive > > access to struct file if I'm right so if two processes end up calling > > getdents() for the same 'struct file' we are doomed. > > RTFS. We sure as hell *do* have exclusive access to struct file. See > /* POSIX.1-2008/SUSv4 Section XSI 2.9.7 */ > if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) > f->f_mode |= FMODE_ATOMIC_POS; > in do_dentry_open() as well as > if (file && (file->f_mode & FMODE_ATOMIC_POS)) { > if (file_count(file) > 1) { > v |= FDPUT_POS_UNLOCK; > mutex_lock(&file->f_pos_lock); > } > } > in __fdget_pos() and > f = fdget_pos(fd); > if (!f.file) > return -EBADF; > in getdents(2). Yeah, sorry. I've misunderstood how the FMODE_ATOMIC_POS thing works... Honza
From d13477d654e60ec4434c266d11828347a135ca32 Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@suse.cz> Date: Tue, 4 Oct 2016 10:56:00 +0200 Subject: [PATCH] ext4: Use exclusive locking for ext4_readdir() We use file->private_data to track information for readdir calls. As such we cannot allow two readdir calls to happen at the same time for one struct file as they end up corrupting the information. For now just revert back to the old behavior where readdirs are synchronized. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index 67415e0e6af0..5ee86243620b 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -651,7 +651,7 @@ int ext4_check_all_de(struct inode *dir, struct buffer_head *bh, void *buf, const struct file_operations ext4_dir_operations = { .llseek = ext4_dir_llseek, .read = generic_read_dir, - .iterate_shared = ext4_readdir, + .iterate = ext4_readdir, .unlocked_ioctl = ext4_ioctl, #ifdef CONFIG_COMPAT .compat_ioctl = ext4_compat_ioctl, -- 2.6.6