| Message ID | BANLkTin4vEQZ4W5LQT36U3BVxWG4VeDZYw@mail.gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, 16 May 2011, Amir Goldstein wrote: > This fixes a bug in commit 21f97697 (ext4: remove unnecessary [cm]time > update of quota file) > > The above commit was merged for 2.6.39-rc3 and introduced a crash > in xfstest 232 (Run fsstress with quotas enabled and verify accounted > quotas in the end). > > Signed-off-by: Amir Goldstein <amir73il@users.sf.net> > --- > > This fix solves the crash on my system, but the test still fails with the > following output: > > QA output created by 232 > > Testing fsstress > > fsstress -n 2000 -d outdir -p 7 > seed = S > Comparing user usage > 3a4,493 > > #1 -- 2456 0 0 13 0 0 > > #10 -- 0 0 0 1 0 0 > > #10098 -- 556 0 0 1 0 0 > > #1023 -- 20 0 0 1 0 0 > > #10253 -- 0 0 0 1 0 0 > > #1026286 -- 4 0 0 1 0 0 > > #103086187 -- 4 0 0 1 0 0 > > #1036 -- 0 0 0 1 0 0 > > #1052282 -- 4 0 0 1 0 0 > > #1057 -- 260 0 0 1 0 0 > > I checked with kernel 2.6.38 and the test passes. > I also tried reverting the commit, but the test still fails. > > In any case, the fix resolved the following crash: > > [ 1319.112544] EXT4-fs (sda8): mounted filesystem with ordered data > mode. Opts: acl,user_xattr,usrquota,grpquota > [ 1319.270023] EXT4-fs (sda8): re-mounted. Opts: (null) > [ 1319.271464] EXT4-fs (sda8): re-mounted. Opts: (null) > [ 1368.214854] BUG: unable to handle kernel NULL pointer dereference > at 0000000000000018 > [ 1368.219348] IP: [<ffffffff8122e152>] ext4_quota_off+0x42/0xd0 > [ 1368.221628] PGD 0 > [ 1368.222978] Oops: 0000 [#2] SMP > [ 1368.222978] last sysfs file: > /sys/devices/system/cpu/cpu3/cache/index2/shared_cpu_map > [ 1368.222978] CPU 0 > [ 1368.222978] Modules linked in: binfmt_misc parport_pc ppdev > snd_hda_codec_realtek snd_hda_intel snd_hda_codec i915 snd_hwdep > snd_pcm drm_kms_helper drm snd_seq_midi snd_rawmidi e1000e > snd_seq_midi_event i2c_algo_bit snd_seq lp firewire_ohci firewire_core > snd_timer snd_seq_device snd soundcore snd_page_alloc psmouse parport > pata_marvell usbhid hid video intel_agp intel_gtt tpm_tis crc_itu_t > serio_raw tpm tpm_bios > [ 1368.222978] > [ 1368.222978] Pid: 2691, comm: quotaon Tainted: G M D > 2.6.39-rc7 #9 /DQ35JO > [ 1368.222978] RIP: 0010:[<ffffffff8122e152>] [<ffffffff8122e152>] > ext4_quota_off+0x42/0xd0 > [ 1368.222978] RSP: 0018:ffff8800c4bb3e28 EFLAGS: 00010292 > [ 1368.222978] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000018 > [ 1368.222978] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000246 > [ 1368.222978] RBP: ffff8800c4bb3e48 R08: 0000000000000001 R09: 0000000000000000 > [ 1368.222978] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880114576000 > [ 1368.222978] R13: ffff880114576000 R14: 0000000000000001 R15: 0000000000000000 > [ 1368.222978] FS: 00007f5c2bf97720(0000) GS:ffff88012bc00000(0000) > knlGS:0000000000000000 > [ 1368.222978] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > [ 1368.222978] CR2: 0000000000000018 CR3: 00000000c693f000 CR4: 00000000000006f0 > [ 1368.222978] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 1368.222978] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > [ 1368.222978] Process quotaon (pid: 2691, threadinfo > ffff8800c4bb2000, task ffff880116bc5ee0) > [ 1368.222978] Stack: > [ 1368.222978] 0000000000800003 0000000000000001 ffff880114576000 > 00000000ffffffda > [ 1368.222978] ffff8800c4bb3ef8 ffffffff811c9e05 0000000000000000 > 0000000000000000 > [ 1368.222978] ffff8800c4bb3e78 ffff880114576068 ffff880115009800 > ffff880114576068 > [ 1368.222978] Call Trace: > [ 1368.222978] [<ffffffff811c9e05>] do_quotactl+0x4e5/0x560 > [ 1368.222978] [<ffffffff815d376c>] ? down_read+0x4c/0x70 > [ 1368.222978] [<ffffffff811711cf>] ? get_super+0x9f/0xd0 > [ 1368.222978] [<ffffffff81189f78>] ? iput+0x48/0x200 > [ 1368.222978] [<ffffffff811c9f4c>] sys_quotactl+0xcc/0x1a0 > [ 1368.222978] [<ffffffff8116be26>] ? filp_close+0x66/0x90 > [ 1368.222978] [<ffffffff812fd76e>] ? trace_hardirqs_on_thunk+0x3a/0x3f > [ 1368.222978] [<ffffffff815dd2c2>] system_call_fastpath+0x16/0x1b > [ 1368.222978] Code: 89 74 24 18 0f 1f 44 00 00 48 63 c6 49 89 fc 41 > 89 f6 48 8b 9c c7 60 03 00 00 48 8b 87 90 04 00 00 f6 40 73 08 0f 85 > 7e 00 00 00 > [ 1368.222978] 8b 7b 18 be 01 00 00 00 e8 c0 fb ff ff 48 3d 00 f0 ff ff 49 > [ 1368.222978] RIP [<ffffffff8122e152>] ext4_quota_off+0x42/0xd0 > [ 1368.222978] RSP <ffff8800c4bb3e28> > [ 1368.222978] CR2: 0000000000000018 > [ 1368.310246] ---[ end trace 62a147f050ade229 ]--- > > > fs/ext4/super.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index fc827bb..2689351 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block > *sb, int type) > if (test_opt(sb, DELALLOC)) > sync_filesystem(sb); > > + if (!inode) > + goto out; Just out of curiosity, why would the quota inode be NULL ? Thanks! -Lukas > + > /* Update modification times of quota files when userspace can > * start looking at them */ > handle = ext4_journal_start(inode, 1); > -- 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 Mon 16-05-11 12:31:15, Amir Goldstein wrote: > This fixes a bug in commit 21f97697 (ext4: remove unnecessary [cm]time > update of quota file) > > The above commit was merged for 2.6.39-rc3 and introduced a crash > in xfstest 232 (Run fsstress with quotas enabled and verify accounted > quotas in the end). > > Signed-off-by: Amir Goldstein <amir73il@users.sf.net> Ah, I see we worked on the patch together :). Anyway, Ted can pick any of them I guess. Honza > --- > > This fix solves the crash on my system, but the test still fails with the > following output: > > QA output created by 232 > > Testing fsstress > > fsstress -n 2000 -d outdir -p 7 > seed = S > Comparing user usage > 3a4,493 > > #1 -- 2456 0 0 13 0 0 > > #10 -- 0 0 0 1 0 0 > > #10098 -- 556 0 0 1 0 0 > > #1023 -- 20 0 0 1 0 0 > > #10253 -- 0 0 0 1 0 0 > > #1026286 -- 4 0 0 1 0 0 > > #103086187 -- 4 0 0 1 0 0 > > #1036 -- 0 0 0 1 0 0 > > #1052282 -- 4 0 0 1 0 0 > > #1057 -- 260 0 0 1 0 0 > > I checked with kernel 2.6.38 and the test passes. > I also tried reverting the commit, but the test still fails. > > In any case, the fix resolved the following crash: > > [ 1319.112544] EXT4-fs (sda8): mounted filesystem with ordered data > mode. Opts: acl,user_xattr,usrquota,grpquota > [ 1319.270023] EXT4-fs (sda8): re-mounted. Opts: (null) > [ 1319.271464] EXT4-fs (sda8): re-mounted. Opts: (null) > [ 1368.214854] BUG: unable to handle kernel NULL pointer dereference > at 0000000000000018 > [ 1368.219348] IP: [<ffffffff8122e152>] ext4_quota_off+0x42/0xd0 > [ 1368.221628] PGD 0 > [ 1368.222978] Oops: 0000 [#2] SMP > [ 1368.222978] last sysfs file: > /sys/devices/system/cpu/cpu3/cache/index2/shared_cpu_map > [ 1368.222978] CPU 0 > [ 1368.222978] Modules linked in: binfmt_misc parport_pc ppdev > snd_hda_codec_realtek snd_hda_intel snd_hda_codec i915 snd_hwdep > snd_pcm drm_kms_helper drm snd_seq_midi snd_rawmidi e1000e > snd_seq_midi_event i2c_algo_bit snd_seq lp firewire_ohci firewire_core > snd_timer snd_seq_device snd soundcore snd_page_alloc psmouse parport > pata_marvell usbhid hid video intel_agp intel_gtt tpm_tis crc_itu_t > serio_raw tpm tpm_bios > [ 1368.222978] > [ 1368.222978] Pid: 2691, comm: quotaon Tainted: G M D > 2.6.39-rc7 #9 /DQ35JO > [ 1368.222978] RIP: 0010:[<ffffffff8122e152>] [<ffffffff8122e152>] > ext4_quota_off+0x42/0xd0 > [ 1368.222978] RSP: 0018:ffff8800c4bb3e28 EFLAGS: 00010292 > [ 1368.222978] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000018 > [ 1368.222978] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000246 > [ 1368.222978] RBP: ffff8800c4bb3e48 R08: 0000000000000001 R09: 0000000000000000 > [ 1368.222978] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880114576000 > [ 1368.222978] R13: ffff880114576000 R14: 0000000000000001 R15: 0000000000000000 > [ 1368.222978] FS: 00007f5c2bf97720(0000) GS:ffff88012bc00000(0000) > knlGS:0000000000000000 > [ 1368.222978] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > [ 1368.222978] CR2: 0000000000000018 CR3: 00000000c693f000 CR4: 00000000000006f0 > [ 1368.222978] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 1368.222978] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > [ 1368.222978] Process quotaon (pid: 2691, threadinfo > ffff8800c4bb2000, task ffff880116bc5ee0) > [ 1368.222978] Stack: > [ 1368.222978] 0000000000800003 0000000000000001 ffff880114576000 > 00000000ffffffda > [ 1368.222978] ffff8800c4bb3ef8 ffffffff811c9e05 0000000000000000 > 0000000000000000 > [ 1368.222978] ffff8800c4bb3e78 ffff880114576068 ffff880115009800 > ffff880114576068 > [ 1368.222978] Call Trace: > [ 1368.222978] [<ffffffff811c9e05>] do_quotactl+0x4e5/0x560 > [ 1368.222978] [<ffffffff815d376c>] ? down_read+0x4c/0x70 > [ 1368.222978] [<ffffffff811711cf>] ? get_super+0x9f/0xd0 > [ 1368.222978] [<ffffffff81189f78>] ? iput+0x48/0x200 > [ 1368.222978] [<ffffffff811c9f4c>] sys_quotactl+0xcc/0x1a0 > [ 1368.222978] [<ffffffff8116be26>] ? filp_close+0x66/0x90 > [ 1368.222978] [<ffffffff812fd76e>] ? trace_hardirqs_on_thunk+0x3a/0x3f > [ 1368.222978] [<ffffffff815dd2c2>] system_call_fastpath+0x16/0x1b > [ 1368.222978] Code: 89 74 24 18 0f 1f 44 00 00 48 63 c6 49 89 fc 41 > 89 f6 48 8b 9c c7 60 03 00 00 48 8b 87 90 04 00 00 f6 40 73 08 0f 85 > 7e 00 00 00 > [ 1368.222978] 8b 7b 18 be 01 00 00 00 e8 c0 fb ff ff 48 3d 00 f0 ff ff 49 > [ 1368.222978] RIP [<ffffffff8122e152>] ext4_quota_off+0x42/0xd0 > [ 1368.222978] RSP <ffff8800c4bb3e28> > [ 1368.222978] CR2: 0000000000000018 > [ 1368.310246] ---[ end trace 62a147f050ade229 ]--- > > > fs/ext4/super.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index fc827bb..2689351 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block > *sb, int type) > if (test_opt(sb, DELALLOC)) > sync_filesystem(sb); > > + if (!inode) > + goto out; > + > /* Update modification times of quota files when userspace can > * start looking at them */ > handle = ext4_journal_start(inode, 1); > -- > 1.7.4.1
On Mon 16-05-11 11:49:22, Lukas Czerner wrote: > On Mon, 16 May 2011, Amir Goldstein wrote: > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index fc827bb..2689351 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block > > *sb, int type) > > if (test_opt(sb, DELALLOC)) > > sync_filesystem(sb); > > > > + if (!inode) > > + goto out; > > Just out of curiosity, why would the quota inode be NULL ? Because quota is already turned off (we then release all references to quota file). Just what I don't understand is why in Amir's testing quota is not turned on before calling quota off. Because when I run the same test, I don't trigger the issue. Honza
On Mon, 16 May 2011, Jan Kara wrote: > On Mon 16-05-11 11:49:22, Lukas Czerner wrote: > > On Mon, 16 May 2011, Amir Goldstein wrote: > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > index fc827bb..2689351 100644 > > > --- a/fs/ext4/super.c > > > +++ b/fs/ext4/super.c > > > @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block > > > *sb, int type) > > > if (test_opt(sb, DELALLOC)) > > > sync_filesystem(sb); > > > > > > + if (!inode) > > > + goto out; > > > > Just out of curiosity, why would the quota inode be NULL ? > Because quota is already turned off (we then release all references to > quota file). Just what I don't understand is why in Amir's testing quota is > not turned on before calling quota off. Because when I run the same test, I > don't trigger the issue. > > Honza > Exactly, I did not read the quota code very deeply, but it seems to me that when we are turning the quota off, it should be on before. So if it is not, it might be something broken and this is not the solution (or maybe it is and I just do not see why:)). Thanks Honzo! -Lukas -- 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 Mon 16-05-11 12:53:47, Lukas Czerner wrote: > On Mon, 16 May 2011, Jan Kara wrote: > > > On Mon 16-05-11 11:49:22, Lukas Czerner wrote: > > > On Mon, 16 May 2011, Amir Goldstein wrote: > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > > index fc827bb..2689351 100644 > > > > --- a/fs/ext4/super.c > > > > +++ b/fs/ext4/super.c > > > > @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block > > > > *sb, int type) > > > > if (test_opt(sb, DELALLOC)) > > > > sync_filesystem(sb); > > > > > > > > + if (!inode) > > > > + goto out; > > > > > > Just out of curiosity, why would the quota inode be NULL ? > > Because quota is already turned off (we then release all references to > > quota file). Just what I don't understand is why in Amir's testing quota is > > not turned on before calling quota off. Because when I run the same test, I > > don't trigger the issue. > > > > Honza > > > > Exactly, I did not read the quota code very deeply, but it seems to me > that when we are turning the quota off, it should be on before. So if it > is not, it might be something broken and this is not the solution (or > maybe it is and I just do not see why:)). Well, userspace can try to turn quotas off whenever it desires and it should not crash the kernel. The check whether quotas are actually turned on is only in dquot_quota_off() called from ext4_quota_off(). It seems to be some problem in Amir's build of xfstests that they happen to call quotaoff on the fs is without quotas turned on. But anyway it should not crash the kernel... Honza
On Mon, May 16, 2011 at 12:11:54PM +0200, Jan Kara wrote: > Ah, I see we worked on the patch together :). Anyway, Ted can pick any of > them I guess. I actually combined the commit description and used elements from both of them. Jan's short description was a better summary, but I liked Amir description of the commit (note though that if you're going to use abberviated git commit id's, please don't shorten them to 10-12 characters since given the size of the git repository, shorter git commit id's could suffer from hash collision). - Ted ext4: fix oops in ext4_quota_off() From: Amir Goldstein <amir73il@gmail.com> If quota is not enabled when ext4_quota_off() is called, we must not dereference quota file inode since it is NULL. Check properly for this. This fixes a bug in commit 21f976975cbe (ext4: remove unnecessary [cm]time update of quota file), which was merged for 2.6.39-rc3. Reported-by: Amir Goldstein <amir73il@users.sf.net> Signed-off-by: Amir Goldstein <amir73il@users.sf.net> Signed-off-by: Jan Kara <jack@suse.cz> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> - 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
On Mon, May 16, 2011 at 5:04 PM, Ted Ts'o <tytso@mit.edu> wrote: > On Mon, May 16, 2011 at 12:11:54PM +0200, Jan Kara wrote: >> Ah, I see we worked on the patch together :). Anyway, Ted can pick any of >> them I guess. > > I actually combined the commit description and used elements from both > of them. Jan's short description was a better summary, but I liked > Amir description of the commit (note though that if you're going to > use abberviated git commit id's, please don't shorten them to 10-12 > characters since given the size of the git repository, shorter git > commit id's could suffer from hash collision). Note taken. I suppose this is not going into 2.6.39(?), so CC: stable is in order. Thanks, Amir. > > - Ted > > ext4: fix oops in ext4_quota_off() > > From: Amir Goldstein <amir73il@gmail.com> > > If quota is not enabled when ext4_quota_off() is called, we must not > dereference quota file inode since it is NULL. Check properly for > this. > > This fixes a bug in commit 21f976975cbe (ext4: remove unnecessary > [cm]time update of quota file), which was merged for 2.6.39-rc3. > > Reported-by: Amir Goldstein <amir73il@users.sf.net> > Signed-off-by: Amir Goldstein <amir73il@users.sf.net> > Signed-off-by: Jan Kara <jack@suse.cz> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > > - 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
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index fc827bb..2689351 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4681,6 +4681,9 @@ static int ext4_quota_off(struct super_block *sb, int type) if (test_opt(sb, DELALLOC)) sync_filesystem(sb); + if (!inode) + goto out; + /* Update modification times of quota files when userspace can * start looking at them */