* [PATCH] xfs: validate quota log items during log recovery
@ 2009-03-03 17:54 Christoph Hellwig
2009-03-16 7:54 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2009-03-03 17:54 UTC (permalink / raw)
To: xfs
Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
I can only explain by a log item beeing too smal to actually fit the
xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
graceful checks for NULL or too small quota items to the log recovery
code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_log_recover.c
===================================================================
--- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
+++ xfs/fs/xfs/xfs_log_recover.c 2009-03-02 04:16:29.649444226 +0100
@@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
error = 0;
if (buf_f->blf_flags &
(XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
+ if (item->ri_buf[i].i_addr == NULL ||
+ item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
+ cmn_err(CE_ALERT,
+ "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
+ item->ri_buf[i].i_len);
+ goto next;
+ }
error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
item->ri_buf[i].i_addr,
-1, 0, XFS_QMOPT_DOWARN,
"dquot_buf_recover");
+ if (error)
+ goto next;
}
- if (!error)
- memcpy(xfs_buf_offset(bp,
- (uint)bit << XFS_BLI_SHIFT), /* dest */
- item->ri_buf[i].i_addr, /* source */
- nbits<<XFS_BLI_SHIFT); /* length */
+
+ memcpy(xfs_buf_offset(bp,
+ (uint)bit << XFS_BLI_SHIFT), /* dest */
+ item->ri_buf[i].i_addr, /* source */
+ nbits<<XFS_BLI_SHIFT); /* length */
+ next:
i++;
bit += nbits;
}
@@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
return (0);
recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
- ASSERT(recddq);
+
+ if (item->ri_buf[1].i_addr == NULL ||
+ item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
+ cmn_err(CE_ALERT,
+ "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
+ item->ri_buf[1].i_len);
+ return XFS_ERROR(EIO);
+ }
+
/*
* This type of quotas was turned off, so ignore this record.
*/
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-03-03 17:54 [PATCH] xfs: validate quota log items during log recovery Christoph Hellwig
@ 2009-03-16 7:54 ` Christoph Hellwig
2009-03-29 7:42 ` Christoph Hellwig
2009-04-04 11:59 ` Arkadiusz Miskiewicz
2009-05-26 16:07 ` Eric Sandeen
2 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2009-03-16 7:54 UTC (permalink / raw)
To: xfs
ping?
On Tue, Mar 03, 2009 at 12:54:27PM -0500, Christoph Hellwig wrote:
> Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> I can only explain by a log item beeing too smal to actually fit the
> xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> graceful checks for NULL or too small quota items to the log recovery
> code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
> +++ xfs/fs/xfs/xfs_log_recover.c 2009-03-02 04:16:29.649444226 +0100
> @@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
> error = 0;
> if (buf_f->blf_flags &
> (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> + if (item->ri_buf[i].i_addr == NULL ||
> + item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> + item->ri_buf[i].i_len);
> + goto next;
> + }
> error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> item->ri_buf[i].i_addr,
> -1, 0, XFS_QMOPT_DOWARN,
> "dquot_buf_recover");
> + if (error)
> + goto next;
> }
> - if (!error)
> - memcpy(xfs_buf_offset(bp,
> - (uint)bit << XFS_BLI_SHIFT), /* dest */
> - item->ri_buf[i].i_addr, /* source */
> - nbits<<XFS_BLI_SHIFT); /* length */
> +
> + memcpy(xfs_buf_offset(bp,
> + (uint)bit << XFS_BLI_SHIFT), /* dest */
> + item->ri_buf[i].i_addr, /* source */
> + nbits<<XFS_BLI_SHIFT); /* length */
> + next:
> i++;
> bit += nbits;
> }
> @@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
> return (0);
>
> recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> - ASSERT(recddq);
> +
> + if (item->ri_buf[1].i_addr == NULL ||
> + item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
> + item->ri_buf[1].i_len);
> + return XFS_ERROR(EIO);
> + }
> +
> /*
> * This type of quotas was turned off, so ignore this record.
> */
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-03-16 7:54 ` Christoph Hellwig
@ 2009-03-29 7:42 ` Christoph Hellwig
2009-05-26 9:11 ` Christoph Hellwig
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2009-03-29 7:42 UTC (permalink / raw)
To: xfs
ping^2?
On Mon, Mar 16, 2009 at 03:54:07AM -0400, Christoph Hellwig wrote:
> ping?
>
> On Tue, Mar 03, 2009 at 12:54:27PM -0500, Christoph Hellwig wrote:
> > Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> > I can only explain by a log item beeing too smal to actually fit the
> > xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> > graceful checks for NULL or too small quota items to the log recovery
> > code.
> >
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Index: xfs/fs/xfs/xfs_log_recover.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
> > +++ xfs/fs/xfs/xfs_log_recover.c 2009-03-02 04:16:29.649444226 +0100
> > @@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
> > error = 0;
> > if (buf_f->blf_flags &
> > (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> > + if (item->ri_buf[i].i_addr == NULL ||
> > + item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> > + cmn_err(CE_ALERT,
> > + "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> > + item->ri_buf[i].i_len);
> > + goto next;
> > + }
> > error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> > item->ri_buf[i].i_addr,
> > -1, 0, XFS_QMOPT_DOWARN,
> > "dquot_buf_recover");
> > + if (error)
> > + goto next;
> > }
> > - if (!error)
> > - memcpy(xfs_buf_offset(bp,
> > - (uint)bit << XFS_BLI_SHIFT), /* dest */
> > - item->ri_buf[i].i_addr, /* source */
> > - nbits<<XFS_BLI_SHIFT); /* length */
> > +
> > + memcpy(xfs_buf_offset(bp,
> > + (uint)bit << XFS_BLI_SHIFT), /* dest */
> > + item->ri_buf[i].i_addr, /* source */
> > + nbits<<XFS_BLI_SHIFT); /* length */
> > + next:
> > i++;
> > bit += nbits;
> > }
> > @@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
> > return (0);
> >
> > recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> > - ASSERT(recddq);
> > +
> > + if (item->ri_buf[1].i_addr == NULL ||
> > + item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> > + cmn_err(CE_ALERT,
> > + "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
> > + item->ri_buf[1].i_len);
> > + return XFS_ERROR(EIO);
> > + }
> > +
> > /*
> > * This type of quotas was turned off, so ignore this record.
> > */
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> ---end quoted text---
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-03-03 17:54 [PATCH] xfs: validate quota log items during log recovery Christoph Hellwig
2009-03-16 7:54 ` Christoph Hellwig
@ 2009-04-04 11:59 ` Arkadiusz Miskiewicz
2009-05-26 16:07 ` Eric Sandeen
2 siblings, 0 replies; 8+ messages in thread
From: Arkadiusz Miskiewicz @ 2009-04-04 11:59 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tuesday 03 of March 2009, Christoph Hellwig wrote:
> Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> I can only explain by a log item beeing too smal to actually fit the
> xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> graceful checks for NULL or too small quota items to the log recovery
> code.
Unfortunately this validation doesn't cover my case. I still got oops even
with the patch applied 1). I also tried xfs debug enabled kernel 2)
1) oops with "[PATCH] xfs: validate quota log items during log recovery"
[ 37.379658] Filesystem "dm-0": Disabling barriers, trial barrier write failed
[ 37.445959] XFS mounting filesystem dm-0
[ 39.478398] Starting XFS recovery on filesystem: dm-0 (logdev: internal)
[ 42.833274] BUG: unable to handle kernel paging request at fffffffffffffc00
[ 42.835651] IP: [<ffffffffa0101e5a>] xfs_qm_dqcheck+0x9ca/0x2270 [xfs]
[ 42.835651] PGD 549067 PUD 54b067 PMD 0
[ 42.876885] Oops: 0002 [#1] SMP
[ 42.876885] last sysfs file: /sys/devices/virtual/block/md3/dev
[ 42.906879] CPU 2
[ 42.906879] Modules linked in: ext3 jbd mbcache raid456 async_xor async_memcpy async_tx xor raid1 dm_mod e1000 e1000e ipmi_devintf ipmi_si ipmi_msghandler 8021q garp stp xfs
scsi_wait_scan sd_mod crc_t10dif mptsas mptscsih mptbase scsi_transport_sas scsi_mod raid10 md_mod
[ 42.976880] Pid: 1718, comm: mount Not tainted 2.6.28.9-2 #1
[ 42.976880] RIP: 0010:[<ffffffffa0101e5a>] [<ffffffffa0101e5a>] xfs_qm_dqcheck+0x9ca/0x2270 [xfs]
[ 42.976880] RSP: 0018:ffff88015b53fa58 EFLAGS: 00010256
[ 42.976880] RAX: 0000000000000000 RBX: ffff88015bcaee40 RCX: 0000000000000000
[ 42.976880] RDX: fffffffffffffc00 RSI: 0000000000000000 RDI: fffffffffffffc00
[ 42.976880] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffffffffffc00
[ 42.976880] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88015bcaebc0
[ 42.976880] R13: ffff88015bcaeb40 R14: 0000000000001000 R15: ffff88015e55d000
[ 42.976880] FS: 00007f3de87f27d0(0000) GS:ffff88015fa4c180(0000) knlGS:0000000000000000
[ 43.176886] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 43.176886] CR2: fffffffffffffc00 CR3: 000000015b501000 CR4: 00000000000006e0
[ 43.176886] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 43.176886] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 43.176886] Process mount (pid: 1718, threadinfo ffff88015b53e000, task ffff88015ccd9470)
[ 43.276880] Stack:
[ 43.276880] ffff88015bcaeb40 000000005d34a0c8 ffff88015b53fb98 ffffc200115e839c
[ 43.313546] ffffc200115e83a8 0000000000000003 ffffc200115e9e00 ffffffffa010351c
[ 43.313546] 0000000000039c4a 000000015b5ab000 ffff88015b5ab000 ffff88015b53fb88
[ 43.340212] Call Trace:
[ 43.340212] [<ffffffffa010351c>] xfs_qm_dqcheck+0x208c/0x2270 [xfs]
[ 43.340212] [<ffffffffa0103ed9>] xlog_get_bp+0x1e9/0x1750 [xfs]
[ 43.340212] [<ffffffffa01256d0>] xfs_dir_file_operations+0x2290/0xe906 [xfs]
[ 43.340212] [<ffffffffa01044e7>] xlog_get_bp+0x7f7/0x1750 [xfs]
[ 43.340212] [<ffffffffa010454a>] xlog_get_bp+0x85a/0x1750 [xfs]
[ 43.340212] [<ffffffffa01058ea>] xlog_recover+0x7a/0x90 [xfs]
[ 43.340212] [<ffffffffa00ff706>] xfs_log_mount+0xa6/0xd30 [xfs]
[ 43.340212] [<ffffffffa010816b>] xfs_mountfs+0x33b/0x680 [xfs]
[ 43.340212] [<ffffffffa00ebb00>] xfs_filestream_lookup_ag+0x60/0x4f0 [xfs]
[ 43.340212] [<ffffffffa0114e3b>] kmem_zalloc+0x2b/0x40 [xfs]
[ 43.340212] [<ffffffffa01091af>] xfs_mru_cache_create+0x12f/0x160 [xfs]
[ 43.340212] [<ffffffffa01216c2>] xfs_blkdev_get+0x492/0x660 [xfs]
[ 43.340212] [<ffffffff802d7c54>] get_sb_bdev+0x174/0x1a0
[ 43.340212] [<ffffffffa0121460>] xfs_blkdev_get+0x230/0x660 [xfs]
[ 43.340212] [<ffffffff802a6854>] kstrdup+0x54/0x70
[ 43.340212] [<ffffffff802d75e6>] vfs_kern_mount+0x86/0x250
[ 43.340212] [<ffffffff802d7813>] do_kern_mount+0x53/0x120
[ 43.340212] [<ffffffff802f14bd>] do_mount+0x2ed/0xa50
[ 43.340212] [<ffffffff802f1d19>] sys_mount+0xf9/0x110
[ 43.340212] [<ffffffff802031bb>] system_call_fastpath+0x16/0x1b
[ 43.340212] Code: 49 8b 54 24 08 8b 42 18 85 c0 75 65 49 8b 45 28 48 8b 58 08 8b 6b 18 85 ed 0f 84 92 00 00 00 48 63 43 14 48 8b 53 20 48 c1 e0 04 <4c> 89 3c 02 48 63 43 14 48 8b 53 20
48 c1 e0 04 44 89 74 02 08
[ 43.340212] RIP [<ffffffffa0101e5a>] xfs_qm_dqcheck+0x9ca/0x2270 [xfs]
[ 43.340212] RSP <ffff88015b53fa58>
[ 43.340212] CR2: fffffffffffffc00
[ 43.340212] ---[ end trace 3fb966a92b4fc211 ]---
[ 60.803310] 0000:05:00.0: eth0: changing MTU from 1500 to 9000
2) ops without "[PATCH] xfs: validate quota log items during log recovery" but with xfs debug enabled kernel
[ 37.768605] Filesystem "dm-0": Disabling barriers, trial barrier write failed
[ 37.831256] XFS mounting filesystem dm-0
[ 39.575446] Starting XFS recovery on filesystem: dm-0 (logdev: internal)
[ 42.482802] Assertion failed: item->ri_total > item->ri_cnt, file: fs/xfs/xfs_log_recover.c, line: 1452
[ 42.482841] ------------[ cut here ]------------
[ 42.486059] kernel BUG at fs/xfs/support/debug.c:81!
[ 42.486059] invalid opcode: 0000 [#1] SMP
[ 42.486059] last sysfs file: /sys/devices/virtual/block/md3/dev
[ 42.486059] CPU 3
[ 42.486059] Modules linked in: ext3 jbd mbcache raid456 async_xor async_memcpy async_tx xor raid1 dm_mod e1000 e1000e ipmi_devintf ipmi_si ipmi_msghandler 8021q garp stp xfs
scsi_wait_scan sd_mod crc_t10dif mptsas mptscsih mptbase scsi_transport_sas scsi_mod raid10 md_mod
[ 42.486059] Pid: 1718, comm: mount Not tainted 2.6.28.7-1 #1
[ 42.486059] RIP: 0010:[<ffffffffa013ddca>] [<ffffffffa013ddca>] assfail+0x1a/0x20 [xfs]
[ 42.486059] RSP: 0018:ffff88015bc7ba48 EFLAGS: 00010292
[ 42.486059] RAX: 000000000000006e RBX: ffff88015bc723c0 RCX: 0000000000000004
[ 42.486059] RDX: 0000000000000d0d RSI: 0000000000000046 RDI: ffffffff8084d310
[ 42.486059] RBP: ffffc200115e63a8 R08: 0000000000000000 R09: 0000000000000006
[ 42.486059] R10: ffff88015bc7b7e8 R11: 000000005bc7b8e8 R12: ffff88015bc72180
[ 42.486059] R13: ffff88015bc722c0 R14: 0000000000001000 R15: ffff88015e0fb000
[ 42.486059] FS: 00007ff1ac5557d0(0000) GS:ffff88015fa4c500(0000) knlGS:0000000000000000
[ 42.486059] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 42.486059] CR2: 00000000006e3000 CR3: 000000015bc6d000 CR4: 00000000000006e0
[ 42.486059] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 42.486059] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 42.486059] Process mount (pid: 1718, threadinfo ffff88015bc7a000, task ffff88015d0ba210)
[ 42.486059] Stack:
[ 42.486059] ffffc200115e63a8 ffffffffa0119883 ffff88015bc72180 ffffc200115e639c
[ 42.486059] ffffc200115e63a8 ffffc200115e7e00 0000000000000003 ffff88015bc7bb78
[ 42.486059] ffff88015b4b4000 ffffffffa011b47e ffff88015d455680 00000001a0132b9d
[ 42.486059] Call Trace:
[ 42.486059] [<ffffffffa0119883>] ? xlog_recover_add_to_trans+0xa3/0x1f0 [xfs]
[ 42.486059] [<ffffffffa011b47e>] ? xlog_recover_process_data+0x23e/0x290 [xfs]
[ 42.486059] [<ffffffffa011ba39>] ? xlog_do_recovery_pass+0x389/0x810 [xfs]
[ 42.486059] [<ffffffff802308f0>] ? default_wake_function+0x0/0x10
[ 42.486059] [<ffffffffa014afa0>] ? xfs_dir_file_operations+0xbd20/0x24571 [xfs]
[ 42.486059] [<ffffffffa011bf16>] ? xlog_do_log_recovery+0x56/0x100 [xfs]
[ 42.486059] [<ffffffffa011bfe0>] ? xlog_do_recover+0x20/0x250 [xfs]
[ 42.486059] [<ffffffffa011d6ca>] ? xlog_recover+0x7a/0x90 [xfs]
[ 42.486059] [<ffffffffa01174ca>] ? xfs_log_mount+0xaa/0x1b0 [xfs]
[ 42.486059] [<ffffffffa012085b>] ? xfs_mountfs+0x32b/0x6b0 [xfs]
[ 42.486059] [<ffffffffa00fcf70>] ? xfs_fstrm_free_func+0x0/0xc0 [xfs]
[ 42.486059] [<ffffffffa012fd7b>] ? kmem_zalloc+0x2b/0x40 [xfs]
[ 42.486059] [<ffffffffa01211ef>] ? xfs_mru_cache_create+0x12f/0x160 [xfs]
[ 42.486059] [<ffffffffa013d472>] ? xfs_fs_fill_super+0x262/0x430 [xfs]
[ 42.486059] [<ffffffff802d7e64>] ? get_sb_bdev+0x174/0x1a0
[ 42.486059] [<ffffffffa013d210>] ? xfs_fs_fill_super+0x0/0x430 [xfs]
[ 42.486059] [<ffffffff802a6964>] ? kstrdup+0x54/0x70
[ 42.486059] [<ffffffff802d77f6>] ? vfs_kern_mount+0x86/0x250
[ 42.486059] [<ffffffff802d7a23>] ? do_kern_mount+0x53/0x120
[ 42.486059] [<ffffffff802f150d>] ? do_mount+0x2ed/0xa50
[ 42.486059] [<ffffffff802f1d69>] ? sys_mount+0xf9/0x110
[ 42.486059] [<ffffffff802031bb>] ? system_call_fastpath+0x16/0x1b
[ 42.486059] Code: 08 01 00 00 00 e8 77 df 26 e0 48 83 c4 18 c3 66 90 89 d1 48 83 ec 08 48 89 f2 31 c0 48 89 fe 48 c7 c7 e0 d2 14 a0 e8 57 4b 3f e0 <0f> 0b eb fe 66 90 41 55 41 54 49 89 f4 55
89 fd 48 c7 c7 80 19
[ 42.486059] RIP [<ffffffffa013ddca>] assfail+0x1a/0x20 [xfs]
[ 42.486059] RSP <ffff88015bc7ba48>
[ 43.497724] ---[ end trace 07b3fe479be2dbfb ]---
[ 59.200404] 0000:05:00.0: eth0: changing MTU from 1500 to 9000
--
Arkadiusz Miśkiewicz PLD/Linux Team
arekm / maven.pl http://ftp.pld-linux.org/
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-03-29 7:42 ` Christoph Hellwig
@ 2009-05-26 9:11 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2009-05-26 9:11 UTC (permalink / raw)
To: xfs
ping^3?
On Sun, Mar 29, 2009 at 03:42:46AM -0400, Christoph Hellwig wrote:
> ping^2?
>
> On Mon, Mar 16, 2009 at 03:54:07AM -0400, Christoph Hellwig wrote:
> > ping?
> >
> > On Tue, Mar 03, 2009 at 12:54:27PM -0500, Christoph Hellwig wrote:
> > > Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> > > I can only explain by a log item beeing too smal to actually fit the
> > > xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> > > graceful checks for NULL or too small quota items to the log recovery
> > > code.
> > >
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > >
> > > Index: xfs/fs/xfs/xfs_log_recover.c
> > > ===================================================================
> > > --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
> > > +++ xfs/fs/xfs/xfs_log_recover.c 2009-03-02 04:16:29.649444226 +0100
> > > @@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
> > > error = 0;
> > > if (buf_f->blf_flags &
> > > (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> > > + if (item->ri_buf[i].i_addr == NULL ||
> > > + item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> > > + cmn_err(CE_ALERT,
> > > + "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> > > + item->ri_buf[i].i_len);
> > > + goto next;
> > > + }
> > > error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> > > item->ri_buf[i].i_addr,
> > > -1, 0, XFS_QMOPT_DOWARN,
> > > "dquot_buf_recover");
> > > + if (error)
> > > + goto next;
> > > }
> > > - if (!error)
> > > - memcpy(xfs_buf_offset(bp,
> > > - (uint)bit << XFS_BLI_SHIFT), /* dest */
> > > - item->ri_buf[i].i_addr, /* source */
> > > - nbits<<XFS_BLI_SHIFT); /* length */
> > > +
> > > + memcpy(xfs_buf_offset(bp,
> > > + (uint)bit << XFS_BLI_SHIFT), /* dest */
> > > + item->ri_buf[i].i_addr, /* source */
> > > + nbits<<XFS_BLI_SHIFT); /* length */
> > > + next:
> > > i++;
> > > bit += nbits;
> > > }
> > > @@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
> > > return (0);
> > >
> > > recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> > > - ASSERT(recddq);
> > > +
> > > + if (item->ri_buf[1].i_addr == NULL ||
> > > + item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> > > + cmn_err(CE_ALERT,
> > > + "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
> > > + item->ri_buf[1].i_len);
> > > + return XFS_ERROR(EIO);
> > > + }
> > > +
> > > /*
> > > * This type of quotas was turned off, so ignore this record.
> > > */
> > >
> > > _______________________________________________
> > > xfs mailing list
> > > xfs@oss.sgi.com
> > > http://oss.sgi.com/mailman/listinfo/xfs
> > ---end quoted text---
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> ---end quoted text---
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-03-03 17:54 [PATCH] xfs: validate quota log items during log recovery Christoph Hellwig
2009-03-16 7:54 ` Christoph Hellwig
2009-04-04 11:59 ` Arkadiusz Miskiewicz
@ 2009-05-26 16:07 ` Eric Sandeen
2009-05-27 9:17 ` Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2009-05-26 16:07 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Christoph Hellwig wrote:
> Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> I can only explain by a log item beeing too smal to actually fit the
^^being too small^^
> xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> graceful checks for NULL or too small quota items to the log recovery
> code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
> +++ xfs/fs/xfs/xfs_log_recover.c 2009-03-02 04:16:29.649444226 +0100
> @@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
> error = 0;
> if (buf_f->blf_flags &
> (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> + if (item->ri_buf[i].i_addr == NULL ||
> + item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> + item->ri_buf[i].i_len);
Shouldn't this differentiate between i_addr == NULL and i_len too small,
though? While we're at it anyway...
Maybe:
+ "XFS: dquot null addr (%p) or len too small (%d) in %s."
+ item->ri_buf[i].i_addr, item->ri_buf[i].i_len, __func__);
? (not hardcoding function name may be good too)
> + goto next;
> + }
> error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> item->ri_buf[i].i_addr,
> -1, 0, XFS_QMOPT_DOWARN,
> "dquot_buf_recover");
> + if (error)
> + goto next;
I guess we can't do much else, but what happens in the end, when we skip
a buffer...
> }
> - if (!error)
> - memcpy(xfs_buf_offset(bp,
> - (uint)bit << XFS_BLI_SHIFT), /* dest */
> - item->ri_buf[i].i_addr, /* source */
> - nbits<<XFS_BLI_SHIFT); /* length */
> +
> + memcpy(xfs_buf_offset(bp,
> + (uint)bit << XFS_BLI_SHIFT), /* dest */
> + item->ri_buf[i].i_addr, /* source */
> + nbits<<XFS_BLI_SHIFT); /* length */
> + next:
> i++;
> bit += nbits;
> }
> @@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
> return (0);
>
> recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> - ASSERT(recddq);
> +
> + if (item->ri_buf[1].i_addr == NULL ||
> + item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
> + item->ri_buf[1].i_len);
Same deal here, should you differentiate on the error & use __func__ ?
-Eric
> + return XFS_ERROR(EIO);
> + }
> +
> /*
> * This type of quotas was turned off, so ignore this record.
> */
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-05-26 16:07 ` Eric Sandeen
@ 2009-05-27 9:17 ` Christoph Hellwig
2009-05-27 19:06 ` Eric Sandeen
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2009-05-27 9:17 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Christoph Hellwig, xfs
On Tue, May 26, 2009 at 11:07:56AM -0500, Eric Sandeen wrote:
> Christoph Hellwig wrote:
> > Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> > I can only explain by a log item beeing too smal to actually fit the
> ^^being too small^^
Thanks, corrected.
> > + if (item->ri_buf[i].i_addr == NULL ||
> > + item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> > + cmn_err(CE_ALERT,
> > + "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> > + item->ri_buf[i].i_len);
>
> Shouldn't this differentiate between i_addr == NULL and i_len too small,
> though? While we're at it anyway...
>
> Maybe:
I've split both into individual checks and used __func__ to print
the function instead of hardconding it.
> > error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> > item->ri_buf[i].i_addr,
> > -1, 0, XFS_QMOPT_DOWARN,
> > "dquot_buf_recover");
> > + if (error)
> > + goto next;
>
> I guess we can't do much else, but what happens in the end, when we skip
> a buffer...
Yeah, same action a a xfs_qm_dqcheck failure. Error handling here
probably wants to be revisited, but that should be a separate patch.
Updated patch below:
Subject: xfs: validate quota log items during log recovery
From: Christoph Hellwig <hch@lst.de>
Arkadiusz has seen really strange crashes in xfs_qm_dqcheck that
I can only explain by a log item being too smal to actually fit the
xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
graceful checks for NULL or too small quota items to the log recovery
code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_log_recover.c
===================================================================
--- xfs.orig/fs/xfs/xfs_log_recover.c 2009-05-27 10:40:03.752821404 +0200
+++ xfs/fs/xfs/xfs_log_recover.c 2009-05-27 10:43:23.740939498 +0200
@@ -1975,16 +1975,30 @@ xlog_recover_do_reg_buffer(
error = 0;
if (buf_f->blf_flags &
(XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
+ if (item->ri_buf[i].i_addr == NULL) {
+ cmn_err(CE_ALERT,
+ "XFS: NULL dquot in %s.", __func__);
+ goto next;
+ }
+ if (item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
+ cmn_err(CE_ALERT,
+ "XFS: dquot too small (%d) in %s.",
+ item->ri_buf[i].i_len, __func__);
+ goto next;
+ }
error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
item->ri_buf[i].i_addr,
-1, 0, XFS_QMOPT_DOWARN,
"dquot_buf_recover");
+ if (error)
+ goto next;
}
- if (!error)
- memcpy(xfs_buf_offset(bp,
- (uint)bit << XFS_BLI_SHIFT), /* dest */
- item->ri_buf[i].i_addr, /* source */
- nbits<<XFS_BLI_SHIFT); /* length */
+
+ memcpy(xfs_buf_offset(bp,
+ (uint)bit << XFS_BLI_SHIFT), /* dest */
+ item->ri_buf[i].i_addr, /* source */
+ nbits<<XFS_BLI_SHIFT); /* length */
+ next:
i++;
bit += nbits;
}
@@ -2615,7 +2629,19 @@ xlog_recover_do_dquot_trans(
return (0);
recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
- ASSERT(recddq);
+
+ if (item->ri_buf[1].i_addr == NULL) {
+ cmn_err(CE_ALERT,
+ "XFS: NULL dquot in %s.", __func__);
+ return XFS_ERROR(EIO);
+ }
+ if (item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
+ cmn_err(CE_ALERT,
+ "XFS: dquot too small (%d) in %s.",
+ item->ri_buf[1].i_len, __func__);
+ return XFS_ERROR(EIO);
+ }
+
/*
* This type of quotas was turned off, so ignore this record.
*/
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: validate quota log items during log recovery
2009-05-27 9:17 ` Christoph Hellwig
@ 2009-05-27 19:06 ` Eric Sandeen
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sandeen @ 2009-05-27 19:06 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Christoph Hellwig wrote:
> Updated patch below:
>
>
> Subject: xfs: validate quota log items during log recovery
> From: Christoph Hellwig <hch@lst.de>
>
> Arkadiusz has seen really strange crashes in xfs_qm_dqcheck that
> I can only explain by a log item being too smal to actually fit the
^^small ;)
> xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck. So add
> graceful checks for NULL or too small quota items to the log recovery
> code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
A bit more verbose now isn't it, but oh well :)
Reviewed-by: Eric Sandeen <sandeen@sandeen.net>
> Index: xfs/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-05-27 10:40:03.752821404 +0200
> +++ xfs/fs/xfs/xfs_log_recover.c 2009-05-27 10:43:23.740939498 +0200
> @@ -1975,16 +1975,30 @@ xlog_recover_do_reg_buffer(
> error = 0;
> if (buf_f->blf_flags &
> (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> + if (item->ri_buf[i].i_addr == NULL) {
> + cmn_err(CE_ALERT,
> + "XFS: NULL dquot in %s.", __func__);
> + goto next;
> + }
> + if (item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in %s.",
> + item->ri_buf[i].i_len, __func__);
> + goto next;
> + }
> error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
> item->ri_buf[i].i_addr,
> -1, 0, XFS_QMOPT_DOWARN,
> "dquot_buf_recover");
> + if (error)
> + goto next;
> }
> - if (!error)
> - memcpy(xfs_buf_offset(bp,
> - (uint)bit << XFS_BLI_SHIFT), /* dest */
> - item->ri_buf[i].i_addr, /* source */
> - nbits<<XFS_BLI_SHIFT); /* length */
> +
> + memcpy(xfs_buf_offset(bp,
> + (uint)bit << XFS_BLI_SHIFT), /* dest */
> + item->ri_buf[i].i_addr, /* source */
> + nbits<<XFS_BLI_SHIFT); /* length */
> + next:
> i++;
> bit += nbits;
> }
> @@ -2615,7 +2629,19 @@ xlog_recover_do_dquot_trans(
> return (0);
>
> recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> - ASSERT(recddq);
> +
> + if (item->ri_buf[1].i_addr == NULL) {
> + cmn_err(CE_ALERT,
> + "XFS: NULL dquot in %s.", __func__);
> + return XFS_ERROR(EIO);
> + }
> + if (item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> + cmn_err(CE_ALERT,
> + "XFS: dquot too small (%d) in %s.",
> + item->ri_buf[1].i_len, __func__);
> + return XFS_ERROR(EIO);
> + }
> +
> /*
> * This type of quotas was turned off, so ignore this record.
> */
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-05-27 19:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-03 17:54 [PATCH] xfs: validate quota log items during log recovery Christoph Hellwig
2009-03-16 7:54 ` Christoph Hellwig
2009-03-29 7:42 ` Christoph Hellwig
2009-05-26 9:11 ` Christoph Hellwig
2009-04-04 11:59 ` Arkadiusz Miskiewicz
2009-05-26 16:07 ` Eric Sandeen
2009-05-27 9:17 ` Christoph Hellwig
2009-05-27 19:06 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox