Patchwork ext4: fix NULL pointer dereference in ext4_quota_off

login
register
mail settings
Submitter Amir Goldstein
Date May 16, 2011, 9:31 a.m.
Message ID <BANLkTin4vEQZ4W5LQT36U3BVxWG4VeDZYw@mail.gmail.com>
Download mbox | patch
Permalink /patch/95701/
State Accepted
Headers show

Comments

Amir Goldstein - May 16, 2011, 9:31 a.m.
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(-)

 	handle = ext4_journal_start(inode, 1);
Lukas Czerner - May 16, 2011, 9:49 a.m.
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
Jan Kara - May 16, 2011, 10:11 a.m.
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
Jan Kara - May 16, 2011, 10:13 a.m.
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
Lukas Czerner - May 16, 2011, 10:53 a.m.
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
Jan Kara - May 16, 2011, 11:03 a.m.
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
Theodore Ts'o - May 16, 2011, 2:04 p.m.
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
Amir Goldstein - May 16, 2011, 2:40 p.m.
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

Patch

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 */