| Message ID | 20130719121405.GA3856@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Jul 19, 2013 at 08:14:05PM +0800, Zheng Liu wrote: > Hi Dave, > > After applied this patch, the problem has been fixed in my own sand box. > But that would be great if you could give it a try. I want to make sure > that this patch can fix the problem. It looks like there has the same > problem in ext3. So if this patch is fine, I will generate a patch for > ext3 file system. Seems to fix the bug for me, both with your test case and also with trinity runs, so... Tested-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > Thanks, > - Zheng > > From: Zheng Liu <wenqing.lz@taobao.com> > > When we try to open a file with O_TMPFILE flag, we will trigger a bug. > The root cause is that in ext4_orphan_add() we check ->i_nlink == 0 and > this check always fails because we set ->i_nlink = 1 in > inode_init_always(). We can use the following program to trigger it: > > int main(int argc, char *argv[]) > { > int fd; > > fd = open(argv[1], O_TMPFILE, 0666); > if (fd < 0) { > perror("open "); > return -1; > } > close(fd); > return 0; > } > > The oops message looks like this: > > kernel BUG at fs/ext4/namei.c:2572! > invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC > Modules linked in: dlci bridge stp hidp cmtp kernelcapi l2tp_ppp l2tp_netlink l2tp_core sctp libcrc32c rfcomm tun fuse nfnetli > nk can_raw ipt_ULOG can_bcm x25 scsi_transport_iscsi ipx p8023 p8022 appletalk phonet psnap vmw_vsock_vmci_transport af_key vmw_vmci rose vsock atm can netrom ax25 af_rxrpc ir > da pppoe pppox ppp_generic slhc bluetooth nfc rfkill rds caif_socket caif crc_ccitt af_802154 llc2 llc snd_hda_codec_realtek snd_hda_intel snd_hda_codec serio_raw snd_pcm pcsp > kr edac_core snd_page_alloc snd_timer snd soundcore r8169 mii sr_mod cdrom pata_atiixp radeon backlight drm_kms_helper ttm > CPU: 1 PID: 1812571 Comm: trinity-child2 Not tainted 3.11.0-rc1+ #12 > Hardware name: Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H, BIOS F12a 04/23/2010 > task: ffff88007dfe69a0 ti: ffff88010f7b6000 task.ti: ffff88010f7b6000 > RIP: 0010:[<ffffffff8125ce69>] [<ffffffff8125ce69>] ext4_orphan_add+0x299/0x2b0 > RSP: 0018:ffff88010f7b7cf8 EFLAGS: 00010202 > RAX: 0000000000000000 RBX: ffff8800966d3020 RCX: 0000000000000000 > RDX: 0000000000000000 RSI: ffff88007dfe70b8 RDI: 0000000000000001 > RBP: ffff88010f7b7d40 R08: ffff880126a3c4e0 R09: ffff88010f7b7ca0 > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801271fd668 > R13: ffff8800966d2f78 R14: ffff88011d7089f0 R15: ffff88007dfe69a0 > FS: 00007f70441a3740(0000) GS:ffff88012a800000(0000) knlGS:00000000f77c96c0 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000002834000 CR3: 0000000107964000 CR4: 00000000000007e0 > DR0: 0000000000780000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600 > Stack: > 0000000000002000 00000020810b6dde 0000000000000000 ffff88011d46db00 > ffff8800966d3020 ffff88011d7089f0 ffff88009c7f4c10 ffff88010f7b7f2c > ffff88007dfe69a0 ffff88010f7b7da8 ffffffff8125cfac ffff880100000004 > Call Trace: > [<ffffffff8125cfac>] ext4_tmpfile+0x12c/0x180 > [<ffffffff811cba78>] path_openat+0x238/0x700 > [<ffffffff8100afc4>] ? native_sched_clock+0x24/0x80 > [<ffffffff811cc647>] do_filp_open+0x47/0xa0 > [<ffffffff811db73f>] ? __alloc_fd+0xaf/0x200 > [<ffffffff811ba2e4>] do_sys_open+0x124/0x210 > [<ffffffff81010725>] ? syscall_trace_enter+0x25/0x290 > [<ffffffff811ba3ee>] SyS_open+0x1e/0x20 > [<ffffffff816ca8d4>] tracesys+0xdd/0xe2 > [<ffffffff81001001>] ? start_thread_common.constprop.6+0x1/0xa0 > Code: 04 00 00 00 89 04 24 31 c0 e8 c4 77 04 00 e9 43 fe ff ff 66 25 00 d0 66 3d 00 80 0f 84 0e fe ff ff 83 7b 48 00 0f 84 04 fe ff ff <0f> 0b 49 8b 8c 24 50 07 00 00 e9 88 fe ff ff 0f 1f 84 00 00 00 > > Here we couldn't call clear_nlink() directly because in d_tmpfile() we > will call inode_dec_link_count() to decrease ->i_nlink. So this commit > tries to call d_tmpfile() before ext4_orphan_add() to fix this problem. > > Reported-by: Dave Jones <davej@redhat.com> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > Cc: "Theodore Ts'o" <tytso@mit.edu> > Cc: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/ext4/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 234b834..35f55a0 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2316,11 +2316,11 @@ retry: > inode->i_op = &ext4_file_inode_operations; > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > + d_tmpfile(dentry, inode); > err = ext4_orphan_add(handle, inode); > if (err) > goto err_drop_inode; > mark_inode_dirty(inode); > - d_tmpfile(dentry, inode); > unlock_new_inode(inode); > } > if (handle) > -- > 1.7.9.7 > > -- > 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 -- 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 Fri, Jul 19, 2013 at 12:31:06PM -0700, Darrick J. Wong wrote: > On Fri, Jul 19, 2013 at 08:14:05PM +0800, Zheng Liu wrote: > > Hi Dave, > > > > After applied this patch, the problem has been fixed in my own sand box. > > But that would be great if you could give it a try. I want to make sure > > that this patch can fix the problem. It looks like there has the same > > problem in ext3. So if this patch is fine, I will generate a patch for > > ext3 file system. > > Seems to fix the bug for me, both with your test case and also with trinity > runs, so... Yeah, seems to be working here too. > Tested-by: Darrick J. Wong <darrick.wong@oracle.com> > Tested-by: Darrick J. Wong <darrick.wong@oracle.com> likewise, Tested-by: Dave Jones <davej@redhat.com> Dave -- 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 Fri, Jul 19, 2013 at 08:14:05PM +0800, Zheng Liu wrote: > Hi Dave, > > After applied this patch, the problem has been fixed in my own sand box. > But that would be great if you could give it a try. I want to make sure > that this patch can fix the problem. It looks like there has the same > problem in ext3. So if this patch is fine, I will generate a patch for > ext3 file system. Nice catch, and you have my ACK. Which tree do you prefer that to go through? vfs and ext4 are the obvious candidates... -- 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, On Sat, Jul 20, 2013 at 12:36:13AM +0100, Al Viro wrote: > On Fri, Jul 19, 2013 at 08:14:05PM +0800, Zheng Liu wrote: > > Hi Dave, > > > > After applied this patch, the problem has been fixed in my own sand box. > > But that would be great if you could give it a try. I want to make sure > > that this patch can fix the problem. It looks like there has the same > > problem in ext3. So if this patch is fine, I will generate a patch for > > ext3 file system. > > Nice catch, and you have my ACK. Which tree do you prefer that to go > through? vfs and ext4 are the obvious candidates... Now ext4 tree has not yet rebased against 3.11-rcX. So it seems that vfs tree is better if we want to fix this bug in mainline kernel as soon as possible. This patch is against mainline kernel. I think it should be applied into vfs tree cleanly. Please let me know if I need to rebase it against vfs tree. BTW, the patch for ext3 file system will be sent out soon. Please pick it up. Thanks, - Zheng -- 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 Sun, Jul 21, 2013 at 12:37:37AM +0800, Zheng Liu wrote: > > Now ext4 tree has not yet rebased against 3.11-rcX. So it seems that > vfs tree is better if we want to fix this bug in mainline kernel as soon > as possible. I was going to rebase the ext4 tree to Linus's current tree just to get this patch in. I think it would be good to get this fix before -rc2, so either you or I should get this to Linus ASAP. So Al, if you're going to send vfs patches to Linus this weekend (say, to fix the equivalent bug for ext3), go for it. Otherwise, let me know and I'll send Linus a one-commit pull request just to get this fix in. Cheers, - 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 Zheng, I updated my kernel code to this commit, built kernel and loaded this kernel. I checked "uname -r", it is 3.11.0-rc1+ But when I run your test code, I always got "O_TMPFILE" undeclared. Could you please help, what could be wrong? (I included fcntl.h) Thank you! On Fri, Jul 19, 2013 at 12:14 PM, Zheng Liu <gnehzuil.liu@gmail.com> wrote: > Hi Dave, > > After applied this patch, the problem has been fixed in my own sand box. > But that would be great if you could give it a try. I want to make sure > that this patch can fix the problem. It looks like there has the same > problem in ext3. So if this patch is fine, I will generate a patch for > ext3 file system. > > Thanks, > - Zheng > > From: Zheng Liu <wenqing.lz@taobao.com> > > When we try to open a file with O_TMPFILE flag, we will trigger a bug. > The root cause is that in ext4_orphan_add() we check ->i_nlink == 0 and > this check always fails because we set ->i_nlink = 1 in > inode_init_always(). We can use the following program to trigger it: > > int main(int argc, char *argv[]) > { > int fd; > > fd = open(argv[1], O_TMPFILE, 0666); > if (fd < 0) { > perror("open "); > return -1; > } > close(fd); > return 0; > } > > The oops message looks like this: > > kernel BUG at fs/ext4/namei.c:2572! > invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC > Modules linked in: dlci bridge stp hidp cmtp kernelcapi l2tp_ppp l2tp_netlink l2tp_core sctp libcrc32c rfcomm tun fuse nfnetli > nk can_raw ipt_ULOG can_bcm x25 scsi_transport_iscsi ipx p8023 p8022 appletalk phonet psnap vmw_vsock_vmci_transport af_key vmw_vmci rose vsock atm can netrom ax25 af_rxrpc ir > da pppoe pppox ppp_generic slhc bluetooth nfc rfkill rds caif_socket caif crc_ccitt af_802154 llc2 llc snd_hda_codec_realtek snd_hda_intel snd_hda_codec serio_raw snd_pcm pcsp > kr edac_core snd_page_alloc snd_timer snd soundcore r8169 mii sr_mod cdrom pata_atiixp radeon backlight drm_kms_helper ttm > CPU: 1 PID: 1812571 Comm: trinity-child2 Not tainted 3.11.0-rc1+ #12 > Hardware name: Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H, BIOS F12a 04/23/2010 > task: ffff88007dfe69a0 ti: ffff88010f7b6000 task.ti: ffff88010f7b6000 > RIP: 0010:[<ffffffff8125ce69>] [<ffffffff8125ce69>] ext4_orphan_add+0x299/0x2b0 > RSP: 0018:ffff88010f7b7cf8 EFLAGS: 00010202 > RAX: 0000000000000000 RBX: ffff8800966d3020 RCX: 0000000000000000 > RDX: 0000000000000000 RSI: ffff88007dfe70b8 RDI: 0000000000000001 > RBP: ffff88010f7b7d40 R08: ffff880126a3c4e0 R09: ffff88010f7b7ca0 > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801271fd668 > R13: ffff8800966d2f78 R14: ffff88011d7089f0 R15: ffff88007dfe69a0 > FS: 00007f70441a3740(0000) GS:ffff88012a800000(0000) knlGS:00000000f77c96c0 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000002834000 CR3: 0000000107964000 CR4: 00000000000007e0 > DR0: 0000000000780000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600 > Stack: > 0000000000002000 00000020810b6dde 0000000000000000 ffff88011d46db00 > ffff8800966d3020 ffff88011d7089f0 ffff88009c7f4c10 ffff88010f7b7f2c > ffff88007dfe69a0 ffff88010f7b7da8 ffffffff8125cfac ffff880100000004 > Call Trace: > [<ffffffff8125cfac>] ext4_tmpfile+0x12c/0x180 > [<ffffffff811cba78>] path_openat+0x238/0x700 > [<ffffffff8100afc4>] ? native_sched_clock+0x24/0x80 > [<ffffffff811cc647>] do_filp_open+0x47/0xa0 > [<ffffffff811db73f>] ? __alloc_fd+0xaf/0x200 > [<ffffffff811ba2e4>] do_sys_open+0x124/0x210 > [<ffffffff81010725>] ? syscall_trace_enter+0x25/0x290 > [<ffffffff811ba3ee>] SyS_open+0x1e/0x20 > [<ffffffff816ca8d4>] tracesys+0xdd/0xe2 > [<ffffffff81001001>] ? start_thread_common.constprop.6+0x1/0xa0 > Code: 04 00 00 00 89 04 24 31 c0 e8 c4 77 04 00 e9 43 fe ff ff 66 25 00 d0 66 3d 00 80 0f 84 0e fe ff ff 83 7b 48 00 0f 84 04 fe ff ff <0f> 0b 49 8b 8c 24 50 07 00 00 e9 88 fe ff ff 0f 1f 84 00 00 00 > > Here we couldn't call clear_nlink() directly because in d_tmpfile() we > will call inode_dec_link_count() to decrease ->i_nlink. So this commit > tries to call d_tmpfile() before ext4_orphan_add() to fix this problem. > > Reported-by: Dave Jones <davej@redhat.com> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > Cc: "Theodore Ts'o" <tytso@mit.edu> > Cc: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/ext4/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 234b834..35f55a0 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2316,11 +2316,11 @@ retry: > inode->i_op = &ext4_file_inode_operations; > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > + d_tmpfile(dentry, inode); > err = ext4_orphan_add(handle, inode); > if (err) > goto err_drop_inode; > mark_inode_dirty(inode); > - d_tmpfile(dentry, inode); > unlock_new_inode(inode); > } > if (handle) > -- > 1.7.9.7 > > -- > 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 -- 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, Jul 23, 2013 at 07:51:41AM +0000, jon ernst wrote: > Hi Zheng, > > I updated my kernel code to this commit, built kernel and loaded this kernel. > I checked "uname -r", it is 3.11.0-rc1+ But when I run your test code, > I always got "O_TMPFILE" undeclared. > Could you please help, what could be wrong? (I included fcntl.h) Ah, actually I used the following program to hit the bug. Just for your information. Please let me know if you have any question. Regards, - Zheng #include <stdio.h> #include <stdlib.h> #include <string.h> #include <unistd.h> #include <sys/types.h> #include <fcntl.h> #define __O_TMPFILE 020000000 #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) int main(int argc, char *argv[]) { int fd; fd = open(argv[1], O_RDWR|O_TMPFILE); if (fd < 0) { perror("open "); return -1; } close(fd); return 0; } -- 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
> Ah, actually I used the following program to hit the bug. Just for your > information. Please let me know if you have any question. > > Regards, > - Zheng > > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > > #include <unistd.h> > #include <sys/types.h> > #include <fcntl.h> > > #define __O_TMPFILE 020000000 > #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) > > int main(int argc, char *argv[]) > { > int fd; > > fd = open(argv[1], O_RDWR|O_TMPFILE); > if (fd < 0) { > perror("open "); > return -1; > } > close(fd); > return 0; > } Thank you Zheng! Pardon if this is a dumb question: why do we need to manually define "O_TMPFILE"? Not like "O_APPEND" etc. ? I saw it has been defined in header file. (fcntl.h) Did I miss anything? Thanks! -- 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, Jul 23, 2013 at 01:39:39PM +0000, jon ernst wrote: > > Ah, actually I used the following program to hit the bug. Just for your > > information. Please let me know if you have any question. > > > > Regards, > > - Zheng > > > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > > > #include <unistd.h> > > #include <sys/types.h> > > #include <fcntl.h> > > > > #define __O_TMPFILE 020000000 > > #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) > > > > int main(int argc, char *argv[]) > > { > > int fd; > > > > fd = open(argv[1], O_RDWR|O_TMPFILE); > > if (fd < 0) { > > perror("open "); > > return -1; > > } > > close(fd); > > return 0; > > } > > Thank you Zheng! Pardon if this is a dumb question: why do we need to > manually define "O_TMPFILE"? Not like "O_APPEND" etc. ? My purpose is to trigger this bug and fix it. So I manually define this flag for my convenience. > I saw it has > been defined in header file. (fcntl.h) Did I miss anything? I guess that you might 'include <fcntl.h>' header file, right? But the O_TMPFILE is defined in $LINUX/include/uapi/asm-generic/fcntl.h. So maybe compiler couldn't find this header file. Regards, - Zheng -- 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
> My purpose is to trigger this bug and fix it. So I manually define this > flag for my convenience. > >> I saw it has >> been defined in header file. (fcntl.h) Did I miss anything? > > I guess that you might 'include <fcntl.h>' header file, right? But the > O_TMPFILE is defined in $LINUX/include/uapi/asm-generic/fcntl.h. So > maybe compiler couldn't find this header file. > > Regards, > - Zheng -- Got it. Thank you. BTW, the weird thing is I didn't see any discussion on ext4 mailing list before Al Viro committed this change. The first time I heard about this is someone reporting a bug about it. Document about this FLAG might be necessary. Also, about this code: /* a horrid kludge trying to make sure that this will fail on old kernels */ #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) why not doing something like this: #ifndef O_TMPFILE /* a horrid kludge trying to make sure that this will fail on old kernels */ #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) #endif Thanks, Jon -- 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
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 234b834..35f55a0 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2316,11 +2316,11 @@ retry: inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations; ext4_set_aops(inode); + d_tmpfile(dentry, inode); err = ext4_orphan_add(handle, inode); if (err) goto err_drop_inode; mark_inode_dirty(inode); - d_tmpfile(dentry, inode); unlock_new_inode(inode); } if (handle)