Shuning Zhang
2019-Sep-11 09:58 UTC
[Ocfs2-devel] [PATCH] ocfs2: protect extent tree in the ocfs2_prepare_inode_for_write
When the extent tree is modified, it shuold be protected by
inode cluster lock and ip_alloc_sem.
The extent tree is accessed and modified in the ocfs2_prepare_inode_for_write,
but isn't protected by ip_alloc_sem.
The following is a case. The function ocfs2_fiemap is accessing
the extent tree, which is modified at the same time.
[47145.974472] kernel BUG at fs/ocfs2/extent_map.c:475!
[47145.974480] invalid opcode: 0000 [#1] SMP
[47145.974489] Modules linked in: tun ocfs2 ocfs2_nodemanager configfs
ocfs2_stackglue xen_netback xen_blkback xen_gntalloc xen_gntdev xen_evtchn
xenfs xen_privcmd vfat fat bnx2fc fcoe libfcoe libfc scsi_transport_fc sunrpc
bridge 8021q mrp garp stp llc ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad
ib_core ib_addr dm_round_robin dm_multipath sg pcspkr raid1 shpchp
ipmi_devintf ipmi_msghandler ext4 jbd2 mbcache2 sd_mod nvme nvme_core bnxt_en
xhci_pci xhci_hcd crc32c_intel be2iscsi bnx2i cnic uio cxgb4i cxgb4 cxgb3i
libcxgbi ipv6 cxgb3 mdio qla4xxx wmi dm_mirror dm_region_hash dm_log dm_mod
iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi iscsi_ibft
iscsi_boot_sysfs
[47145.974636] CPU: 16 PID: 14047 Comm: o2info Not tainted
4.1.12-124.23.1.el6uek.x86_64 #2
[47145.974646] Hardware name: Oracle Corporation ORACLE SERVER X7-2L/ASM, MB
MECH, X7-2L, BIOS 42040600 10/19/2018
[47145.974658] task: ffff88019487e200 ti: ffff88003daa4000 task.ti:
ffff88003daa4000
[47145.974667] RIP: e030:[<ffffffffa05e4840>] [<ffffffffa05e4840>]
ocfs2_get_clusters_nocache.isra.11+0x390/0x550 [ocfs2]
[47145.974708] RSP: e02b:ffff88003daa7d88 EFLAGS: 00010287
[47145.974713] RAX: 00000000000000de RBX: ffff8801d1104030 RCX:
ffff8801d1104e10
[47145.974719] RDX: 00000000000000de RSI: 000000000009ec40 RDI:
ffff8801d1104e24
[47145.974725] RBP: ffff88003daa7df8 R08: ffff88003daa7e38 R09:
0000000000000000
[47145.974732] R10: 000000000009ec3f R11: 0000000000000246 R12:
000000000009ec3f
[47145.974739] R13: ffff88004c419000 R14: 0000000000000002 R15:
ffff88003daa7e28
[47145.974754] FS: 00007fdbccc92720(0000) GS:ffff880358800000(0000)
knlGS:ffff880358800000
[47145.974764] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[47145.974772] CR2: 00007fd5dfcd8350 CR3: 0000000208677000 CR4:
0000000000042660
[47145.974785] Stack:
[47145.974790] ffff88003daa7df8 00002000a05e249b ffff8801d1104000
ffff88003daa7e2c
[47145.974802] ffff88003daa7e38 ffff88000cc484c0 ffff880145f5b478
0000000000000000
[47145.974811] 0000000000002000 ffff88000cc484c0 ffff88003daa7ea0
0000000000000000
[47145.974820] Call Trace:
[47145.974837] [<ffffffffa05e53e3>] ocfs2_fiemap+0x1e3/0x430 [ocfs2]
[47145.974848] [<ffffffff816f644f>] ? xen_hypervisor_callback+0x7f/0x120
[47145.974855] [<ffffffff816f6448>] ? xen_hypervisor_callback+0x78/0x120
[47145.974861] [<ffffffff816f64a3>] ? xen_hypervisor_callback+0xd3/0x120
[47145.974872] [<ffffffff81220905>] do_vfs_ioctl+0x155/0x510
[47145.974878] [<ffffffff81220d41>] SyS_ioctl+0x81/0xa0
[47145.974885] [<ffffffff816f1b9f>] ? system_call_after_swapgs+0xe9/0x190
[47145.974891] [<ffffffff816f1b98>] ? system_call_after_swapgs+0xe2/0x190
[47145.974899] [<ffffffff816f1b91>] ? system_call_after_swapgs+0xdb/0x190
[47145.974905] [<ffffffff816f1c5e>] system_call_fastpath+0x18/0xd8
[47145.974910] Code: 18 48 c7 c6 60 7f 65 a0 31 c0 bb e2 ff ff ff 48 8b 4a 40
48 8b 7a 28 48 c7 c2 78 2d 66 a0 e8 38 4f 05 00 e9 28 fe ff ff 0f 1f 00
<0f>
0b 66 0f 1f 44 00 00 bb 86 ff ff ff e9 13 fe ff ff 66 0f 1f
[47145.975000] RIP [<ffffffffa05e4840>]
ocfs2_get_clusters_nocache.isra.11+0x390/0x550 [ocfs2]
[47145.975018] RSP <ffff88003daa7d88>
[47145.989999] ---[ end trace c8aa0c8180e869dc ]---
[47146.087579] Kernel panic - not syncing: Fatal exception
[47146.087691] Kernel Offset: disabled
Signed-off-by: Shuning Zhang <sunny.s.zhang at oracle.com>
---
fs/ocfs2/file.c | 79 +++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 54 insertions(+), 25 deletions(-)
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 4435df3..85cddd2 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2093,29 +2093,19 @@ static int ocfs2_is_io_unaligned(struct inode *inode,
size_t count, loff_t pos)
}
static int ocfs2_prepare_inode_for_refcount(struct inode *inode,
+ struct buffer_head *di_bh,
struct file *file,
- loff_t pos, size_t count,
- int *meta_level)
+ loff_t pos, size_t count)
{
int ret;
- struct buffer_head *di_bh = NULL;
u32 cpos = pos >> OCFS2_SB(inode->i_sb)->s_clustersize_bits;
u32 clusters ocfs2_clusters_for_bytes(inode->i_sb, pos + count) - cpos;
- ret = ocfs2_inode_lock(inode, &di_bh, 1);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- *meta_level = 1;
-
ret = ocfs2_refcount_cow(inode, di_bh, cpos, clusters, UINT_MAX);
if (ret)
mlog_errno(ret);
-out:
- brelse(di_bh);
+
return ret;
}
@@ -2123,6 +2113,7 @@ static int ocfs2_prepare_inode_for_write(struct file
*file,
loff_t pos, size_t count, int wait)
{
int ret = 0, meta_level = 0, overwrite_io = 0;
+ int write_sem = 0;
struct dentry *dentry = file->f_path.dentry;
struct inode *inode = d_inode(dentry);
struct buffer_head *di_bh = NULL;
@@ -2145,25 +2136,30 @@ static int ocfs2_prepare_inode_for_write(struct file
*file,
goto out;
}
+ if (wait)
+ down_read(&OCFS2_I(inode)->ip_alloc_sem);
+ else {
+ ret = down_read_trylock(&OCFS2_I(inode)->ip_alloc_sem);
+ if (!ret) {
+ ret = -EAGAIN;
+ goto out_unlock;
+ }
+ }
+
/*
* Check if IO will overwrite allocated blocks in case
* IOCB_NOWAIT flag is set.
*/
if (!wait && !overwrite_io) {
overwrite_io = 1;
- if (!down_read_trylock(&OCFS2_I(inode)->ip_alloc_sem)) {
- ret = -EAGAIN;
- goto out_unlock;
- }
ret = ocfs2_overwrite_io(inode, di_bh, pos, count);
brelse(di_bh);
di_bh = NULL;
- up_read(&OCFS2_I(inode)->ip_alloc_sem);
if (ret < 0) {
if (ret != -EAGAIN)
mlog_errno(ret);
- goto out_unlock;
+ goto out_up_sem;
}
}
@@ -2178,6 +2174,7 @@ static int ocfs2_prepare_inode_for_write(struct file
*file,
* set inode->i_size at the end of a write. */
if (should_remove_suid(dentry)) {
if (meta_level == 0) {
+ up_read(&OCFS2_I(inode)->ip_alloc_sem);
ocfs2_inode_unlock(inode, meta_level);
meta_level = 1;
continue;
@@ -2186,7 +2183,7 @@ static int ocfs2_prepare_inode_for_write(struct file
*file,
ret = ocfs2_write_remove_suid(inode);
if (ret < 0) {
mlog_errno(ret);
- goto out_unlock;
+ goto out_up_sem;
}
}
@@ -2194,24 +2191,56 @@ static int ocfs2_prepare_inode_for_write(struct file
*file,
ret = ocfs2_check_range_for_refcount(inode, pos, count);
if (ret == 1) {
+ up_read(&OCFS2_I(inode)->ip_alloc_sem);
ocfs2_inode_unlock(inode, meta_level);
- meta_level = -1;
+ brelse(di_bh);
+ di_bh = NULL;
+
+ if (wait)
+ ret = ocfs2_inode_lock(inode, &di_bh, 1);
+ else
+ ret = ocfs2_try_inode_lock(inode, &di_bh, 1);
+ if (ret) {
+ if (ret != -EAGAIN)
+ mlog_errno(ret);
+ goto out;
+ }
+
+ meta_level = 1;
+
+ if (wait)
+ down_write(&OCFS2_I(inode)->ip_alloc_sem);
+ else {
+ ret = down_write_trylock(&OCFS2_I(inode)->ip_alloc_sem);
+ if (!ret) {
+ ret = -EAGAIN;
+ goto out_unlock;
+ }
+ }
+
+ write_sem = 1;
ret = ocfs2_prepare_inode_for_refcount(inode,
+ di_bh,
file,
pos,
- count,
- &meta_level);
+ count);
}
if (ret < 0) {
- mlog_errno(ret);
- goto out_unlock;
+ if (ret != -EAGAIN)
+ mlog_errno(ret);
+ goto out_up_sem;
}
break;
}
+out_up_sem:
+ if (write_sem)
+ up_write(&OCFS2_I(inode)->ip_alloc_sem);
+ else
+ up_read(&OCFS2_I(inode)->ip_alloc_sem);
out_unlock:
trace_ocfs2_prepare_inode_for_write(OCFS2_I(inode)->ip_blkno,
pos, count, wait);
--
2.7.4
Junxiao Bi
2019-Sep-11 22:30 UTC
[Ocfs2-devel] [PATCH] ocfs2: protect extent tree in the ocfs2_prepare_inode_for_write
Hi Shuning, See comments inlined. On 9/11/19 2:58 AM, Shuning Zhang wrote:> When the extent tree is modified, it shuold be protected by > inode cluster lock and ip_alloc_sem. > > The extent tree is accessed and modified in the ocfs2_prepare_inode_for_write, > but isn't protected by ip_alloc_sem. > > The following is a case. The function ocfs2_fiemap is accessing > the extent tree, which is modified at the same time. > > [47145.974472] kernel BUG at fs/ocfs2/extent_map.c:475! > [47145.974480] invalid opcode: 0000 [#1] SMP > [47145.974489] Modules linked in: tun ocfs2 ocfs2_nodemanager configfs > ocfs2_stackglue xen_netback xen_blkback xen_gntalloc xen_gntdev xen_evtchn > xenfs xen_privcmd vfat fat bnx2fc fcoe libfcoe libfc scsi_transport_fc sunrpc > bridge 8021q mrp garp stp llc ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad > ib_core ib_addr dm_round_robin dm_multipath sg pcspkr raid1 shpchp > ipmi_devintf ipmi_msghandler ext4 jbd2 mbcache2 sd_mod nvme nvme_core bnxt_en > xhci_pci xhci_hcd crc32c_intel be2iscsi bnx2i cnic uio cxgb4i cxgb4 cxgb3i > libcxgbi ipv6 cxgb3 mdio qla4xxx wmi dm_mirror dm_region_hash dm_log dm_mod > iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi iscsi_ibft > iscsi_boot_sysfs > [47145.974636] CPU: 16 PID: 14047 Comm: o2info Not tainted > 4.1.12-124.23.1.el6uek.x86_64 #2 > [47145.974646] Hardware name: Oracle Corporation ORACLE SERVER X7-2L/ASM, MB > MECH, X7-2L, BIOS 42040600 10/19/2018 > [47145.974658] task: ffff88019487e200 ti: ffff88003daa4000 task.ti: > ffff88003daa4000 > [47145.974667] RIP: e030:[<ffffffffa05e4840>] [<ffffffffa05e4840>] > ocfs2_get_clusters_nocache.isra.11+0x390/0x550 [ocfs2] > [47145.974708] RSP: e02b:ffff88003daa7d88 EFLAGS: 00010287 > [47145.974713] RAX: 00000000000000de RBX: ffff8801d1104030 RCX: > ffff8801d1104e10 > [47145.974719] RDX: 00000000000000de RSI: 000000000009ec40 RDI: > ffff8801d1104e24 > [47145.974725] RBP: ffff88003daa7df8 R08: ffff88003daa7e38 R09: > 0000000000000000 > [47145.974732] R10: 000000000009ec3f R11: 0000000000000246 R12: > 000000000009ec3f > [47145.974739] R13: ffff88004c419000 R14: 0000000000000002 R15: > ffff88003daa7e28 > [47145.974754] FS: 00007fdbccc92720(0000) GS:ffff880358800000(0000) > knlGS:ffff880358800000 > [47145.974764] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033 > [47145.974772] CR2: 00007fd5dfcd8350 CR3: 0000000208677000 CR4: > 0000000000042660 > [47145.974785] Stack: > [47145.974790] ffff88003daa7df8 00002000a05e249b ffff8801d1104000 > ffff88003daa7e2c > [47145.974802] ffff88003daa7e38 ffff88000cc484c0 ffff880145f5b478 > 0000000000000000 > [47145.974811] 0000000000002000 ffff88000cc484c0 ffff88003daa7ea0 > 0000000000000000 > [47145.974820] Call Trace: > [47145.974837] [<ffffffffa05e53e3>] ocfs2_fiemap+0x1e3/0x430 [ocfs2] > [47145.974848] [<ffffffff816f644f>] ? xen_hypervisor_callback+0x7f/0x120 > [47145.974855] [<ffffffff816f6448>] ? xen_hypervisor_callback+0x78/0x120 > [47145.974861] [<ffffffff816f64a3>] ? xen_hypervisor_callback+0xd3/0x120 > [47145.974872] [<ffffffff81220905>] do_vfs_ioctl+0x155/0x510 > [47145.974878] [<ffffffff81220d41>] SyS_ioctl+0x81/0xa0 > [47145.974885] [<ffffffff816f1b9f>] ? system_call_after_swapgs+0xe9/0x190 > [47145.974891] [<ffffffff816f1b98>] ? system_call_after_swapgs+0xe2/0x190 > [47145.974899] [<ffffffff816f1b91>] ? system_call_after_swapgs+0xdb/0x190 > [47145.974905] [<ffffffff816f1c5e>] system_call_fastpath+0x18/0xd8 > [47145.974910] Code: 18 48 c7 c6 60 7f 65 a0 31 c0 bb e2 ff ff ff 48 8b 4a 40 > 48 8b 7a 28 48 c7 c2 78 2d 66 a0 e8 38 4f 05 00 e9 28 fe ff ff 0f 1f 00 <0f> > 0b 66 0f 1f 44 00 00 bb 86 ff ff ff e9 13 fe ff ff 66 0f 1f > [47145.975000] RIP [<ffffffffa05e4840>] > ocfs2_get_clusters_nocache.isra.11+0x390/0x550 [ocfs2] > [47145.975018] RSP <ffff88003daa7d88> > [47145.989999] ---[ end trace c8aa0c8180e869dc ]--- > [47146.087579] Kernel panic - not syncing: Fatal exception > [47146.087691] Kernel Offset: disabled > > Signed-off-by: Shuning Zhang <sunny.s.zhang at oracle.com> > --- > fs/ocfs2/file.c | 79 +++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 54 insertions(+), 25 deletions(-) > > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index 4435df3..85cddd2 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -2093,29 +2093,19 @@ static int ocfs2_is_io_unaligned(struct inode *inode, size_t count, loff_t pos) > } > > static int ocfs2_prepare_inode_for_refcount(struct inode *inode, > + struct buffer_head *di_bh, > struct file *file, > - loff_t pos, size_t count, > - int *meta_level) > + loff_t pos, size_t count) > { > int ret; > - struct buffer_head *di_bh = NULL; > u32 cpos = pos >> OCFS2_SB(inode->i_sb)->s_clustersize_bits; > u32 clusters > ocfs2_clusters_for_bytes(inode->i_sb, pos + count) - cpos; > > - ret = ocfs2_inode_lock(inode, &di_bh, 1); > - if (ret) { > - mlog_errno(ret); > - goto out; > - } > - > - *meta_level = 1; > - > ret = ocfs2_refcount_cow(inode, di_bh, cpos, clusters, UINT_MAX); > if (ret) > mlog_errno(ret); > -out: > - brelse(di_bh); > + > return ret; > }Since you moved out cluster from ocfs2_prepare_inode_for_refcount() , only ocfs2_refcount_cow left, can we go even further to remove it?> > @@ -2123,6 +2113,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file, > loff_t pos, size_t count, int wait) > { > int ret = 0, meta_level = 0, overwrite_io = 0; > + int write_sem = 0; > struct dentry *dentry = file->f_path.dentry; > struct inode *inode = d_inode(dentry); > struct buffer_head *di_bh = NULL; > @@ -2145,25 +2136,30 @@ static int ocfs2_prepare_inode_for_write(struct file *file, > goto out; > } > > + if (wait) > + down_read(&OCFS2_I(inode)->ip_alloc_sem); > + else { > + ret = down_read_trylock(&OCFS2_I(inode)->ip_alloc_sem); > + if (!ret) { > + ret = -EAGAIN; > + goto out_unlock; > + } > + }can we do it like the following? if(wait) { ??? lock(inode_lock); ??? lock(ip_alloc_sem); } else { ??? try_lock(); }> + > /* > * Check if IO will overwrite allocated blocks in case > * IOCB_NOWAIT flag is set. > */ > if (!wait && !overwrite_io) { > overwrite_io = 1; > - if (!down_read_trylock(&OCFS2_I(inode)->ip_alloc_sem)) { > - ret = -EAGAIN; > - goto out_unlock; > - } > > ret = ocfs2_overwrite_io(inode, di_bh, pos, count); > brelse(di_bh); > di_bh = NULL; > - up_read(&OCFS2_I(inode)->ip_alloc_sem); > if (ret < 0) { > if (ret != -EAGAIN) > mlog_errno(ret); > - goto out_unlock; > + goto out_up_sem; > } > } > > @@ -2178,6 +2174,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file, > * set inode->i_size at the end of a write. */ > if (should_remove_suid(dentry)) { > if (meta_level == 0) { > + up_read(&OCFS2_I(inode)->ip_alloc_sem); > ocfs2_inode_unlock(inode, meta_level); > meta_level = 1; > continue; > @@ -2186,7 +2183,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file, > ret = ocfs2_write_remove_suid(inode); > if (ret < 0) { > mlog_errno(ret); > - goto out_unlock; > + goto out_up_sem; > } > } > > @@ -2194,24 +2191,56 @@ static int ocfs2_prepare_inode_for_write(struct file *file, > > ret = ocfs2_check_range_for_refcount(inode, pos, count); > if (ret == 1) { > + up_read(&OCFS2_I(inode)->ip_alloc_sem); > ocfs2_inode_unlock(inode, meta_level); > - meta_level = -1; > + brelse(di_bh); > + di_bh = NULL; > + > + if (wait) > + ret = ocfs2_inode_lock(inode, &di_bh, 1); > + else > + ret = ocfs2_try_inode_lock(inode, &di_bh, 1);You seemed miss call brelse(di_bh)? That will cause buffer_head leak. ocfs2_assign_bh() get it.> + if (ret) { > + if (ret != -EAGAIN) > + mlog_errno(ret); > + goto out; > + } > + > + meta_level = 1; > + > + if (wait) > + down_write(&OCFS2_I(inode)->ip_alloc_sem); > + else { > + ret = down_write_trylock(&OCFS2_I(inode)->ip_alloc_sem); > + if (!ret) { > + ret = -EAGAIN; > + goto out_unlock; > + } > + }The same style issue like above. Maybe we can abstract it in a function?> + > + write_sem = 1; > > ret = ocfs2_prepare_inode_for_refcount(inode, > + di_bh, > file, > pos, > - count, > - &meta_level); > + count); > } > > if (ret < 0) { > - mlog_errno(ret); > - goto out_unlock; > + if (ret != -EAGAIN) > + mlog_errno(ret); > + goto out_up_sem; > } > > break; > } > > +out_up_sem: > + if (write_sem) > + up_write(&OCFS2_I(inode)->ip_alloc_sem); > + else > + up_read(&OCFS2_I(inode)->ip_alloc_sem);Where were inode cluster lock unlocked? Thanks, Junxiao.> out_unlock: > trace_ocfs2_prepare_inode_for_write(OCFS2_I(inode)->ip_blkno, > pos, count, wait);