| Message ID | 1635497520-8168-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Fix the free logic of state in xfs_attr_node_hasname | expand |
[adding the resident xattrs expert] On Fri, Oct 29, 2021 at 04:52:00PM +0800, Yang Xu wrote: > When testing xfstests xfs/126 on lastest upstream kernel, it will hang on some machine. > Adding a getxattr operation after xattr corrupted, I can reproduce it 100%. > > The deadlock as below: > [983.923403] task:setfattr state:D stack: 0 pid:17639 ppid: 14687 flags:0x00000080 > [ 983.923405] Call Trace: > [ 983.923410] __schedule+0x2c4/0x700 > [ 983.923412] schedule+0x37/0xa0 > [ 983.923414] schedule_timeout+0x274/0x300 > [ 983.923416] __down+0x9b/0xf0 > [ 983.923451] ? xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] > [ 983.923453] down+0x3b/0x50 > [ 983.923471] xfs_buf_lock+0x33/0xf0 [xfs] > [ 983.923490] xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] > [ 983.923508] xfs_buf_get_map+0x4c/0x320 [xfs] > [ 983.923525] xfs_buf_read_map+0x53/0x310 [xfs] > [ 983.923541] ? xfs_da_read_buf+0xcf/0x120 [xfs] > [ 983.923560] xfs_trans_read_buf_map+0x1cf/0x360 [xfs] > [ 983.923575] ? xfs_da_read_buf+0xcf/0x120 [xfs] > [ 983.923590] xfs_da_read_buf+0xcf/0x120 [xfs] > [ 983.923606] xfs_da3_node_read+0x1f/0x40 [xfs] > [ 983.923621] xfs_da3_node_lookup_int+0x69/0x4a0 [xfs] > [ 983.923624] ? kmem_cache_alloc+0x12e/0x270 > [ 983.923637] xfs_attr_node_hasname+0x6e/0xa0 [xfs] > [ 983.923651] xfs_has_attr+0x6e/0xd0 [xfs] > [ 983.923664] xfs_attr_set+0x273/0x320 [xfs] > [ 983.923683] xfs_xattr_set+0x87/0xd0 [xfs] > [ 983.923686] __vfs_removexattr+0x4d/0x60 > [ 983.923688] __vfs_removexattr_locked+0xac/0x130 > [ 983.923689] vfs_removexattr+0x4e/0xf0 > [ 983.923690] removexattr+0x4d/0x80 > [ 983.923693] ? __check_object_size+0xa8/0x16b > [ 983.923695] ? strncpy_from_user+0x47/0x1a0 > [ 983.923696] ? getname_flags+0x6a/0x1e0 > [ 983.923697] ? _cond_resched+0x15/0x30 > [ 983.923699] ? __sb_start_write+0x1e/0x70 > [ 983.923700] ? mnt_want_write+0x28/0x50 > [ 983.923701] path_removexattr+0x9b/0xb0 > [ 983.923702] __x64_sys_removexattr+0x17/0x20 > [ 983.923704] do_syscall_64+0x5b/0x1a0 > [ 983.923705] entry_SYSCALL_64_after_hwframe+0x65/0xca > [ 983.923707] RIP: 0033:0x7f080f10ee1b > > When getxattr calls xfs_attr_node_get, xfs_da3_node_lookup_int fails in > xfs_attr_node_hasname because we have use blocktrash to random it in xfs/126. So it > free stat and xfs_attr_node_get doesn't do xfs_buf_trans release job. > > Then subsequent removexattr will hang because of it. > > This bug was introduced by kernel commit 07120f1abdff ("xfs: Add xfs_has_attr and subroutines"). > It adds xfs_attr_node_hasname helper and said caller will be responsible for freeing the state > in this case. But xfs_attr_node_hasname will free stat itself instead of caller if > xfs_da3_node_lookup_int fails. > > Fix this bug by moving the step of free state into caller. > > Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines") > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> Ah, I knew this function was gross. Before, we would set *statep to NULL upon entry to the function, and we would pass the newly allocated da state back out if !error. However, the caller has no idea if the return value came from error or retval, other than (I guess) the comment implies (or had better imply) that ENOATTR/EEXIST only come from retval. Now you're changing it to always pass state out via **statep even if the da3 lookup returns error and we want to pass that out. But then xfs_attr_node_addname_find_attr does this: retval = xfs_attr_node_hasname(args, &dac->da_state); if (retval != -ENOATTR && retval != -EEXIST) return error; without ever clearing dac->da_state. Won't that leak the da state? Granted, I wonder if the xfs_attr_node_hasname call in xfs_attr_node_removename_setup will also leak the state if the return value is ENOATTR? If you ask me the whole ENOATTR/EEXIST thing still needs to be replaced with an enum xfs_attr_lookup_result passed out separately so that we don't have to think about which magic errno values are not really errors. --D > --- > fs/xfs/libxfs/xfs_attr.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index fbc9d816882c..6ad50a76fd8d 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1077,21 +1077,18 @@ xfs_attr_node_hasname( > > state = xfs_da_state_alloc(args); > if (statep != NULL) > - *statep = NULL; > + *statep = state; > > /* > * Search to see if name exists, and get back a pointer to it. > */ > error = xfs_da3_node_lookup_int(state, &retval); > - if (error) { > - xfs_da_state_free(state); > - return error; > - } > + if (error) > + retval = error; > > - if (statep != NULL) > - *statep = state; > - else > + if (!statep) > xfs_da_state_free(state); > + > return retval; > } > > -- > 2.23.0 >
On 2021/10/30 2:50, Darrick J. Wong wrote: > [adding the resident xattrs expert] > > On Fri, Oct 29, 2021 at 04:52:00PM +0800, Yang Xu wrote: >> When testing xfstests xfs/126 on lastest upstream kernel, it will hang on some machine. >> Adding a getxattr operation after xattr corrupted, I can reproduce it 100%. >> >> The deadlock as below: >> [983.923403] task:setfattr state:D stack: 0 pid:17639 ppid: 14687 flags:0x00000080 >> [ 983.923405] Call Trace: >> [ 983.923410] __schedule+0x2c4/0x700 >> [ 983.923412] schedule+0x37/0xa0 >> [ 983.923414] schedule_timeout+0x274/0x300 >> [ 983.923416] __down+0x9b/0xf0 >> [ 983.923451] ? xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] >> [ 983.923453] down+0x3b/0x50 >> [ 983.923471] xfs_buf_lock+0x33/0xf0 [xfs] >> [ 983.923490] xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] >> [ 983.923508] xfs_buf_get_map+0x4c/0x320 [xfs] >> [ 983.923525] xfs_buf_read_map+0x53/0x310 [xfs] >> [ 983.923541] ? xfs_da_read_buf+0xcf/0x120 [xfs] >> [ 983.923560] xfs_trans_read_buf_map+0x1cf/0x360 [xfs] >> [ 983.923575] ? xfs_da_read_buf+0xcf/0x120 [xfs] >> [ 983.923590] xfs_da_read_buf+0xcf/0x120 [xfs] >> [ 983.923606] xfs_da3_node_read+0x1f/0x40 [xfs] >> [ 983.923621] xfs_da3_node_lookup_int+0x69/0x4a0 [xfs] >> [ 983.923624] ? kmem_cache_alloc+0x12e/0x270 >> [ 983.923637] xfs_attr_node_hasname+0x6e/0xa0 [xfs] >> [ 983.923651] xfs_has_attr+0x6e/0xd0 [xfs] >> [ 983.923664] xfs_attr_set+0x273/0x320 [xfs] >> [ 983.923683] xfs_xattr_set+0x87/0xd0 [xfs] >> [ 983.923686] __vfs_removexattr+0x4d/0x60 >> [ 983.923688] __vfs_removexattr_locked+0xac/0x130 >> [ 983.923689] vfs_removexattr+0x4e/0xf0 >> [ 983.923690] removexattr+0x4d/0x80 >> [ 983.923693] ? __check_object_size+0xa8/0x16b >> [ 983.923695] ? strncpy_from_user+0x47/0x1a0 >> [ 983.923696] ? getname_flags+0x6a/0x1e0 >> [ 983.923697] ? _cond_resched+0x15/0x30 >> [ 983.923699] ? __sb_start_write+0x1e/0x70 >> [ 983.923700] ? mnt_want_write+0x28/0x50 >> [ 983.923701] path_removexattr+0x9b/0xb0 >> [ 983.923702] __x64_sys_removexattr+0x17/0x20 >> [ 983.923704] do_syscall_64+0x5b/0x1a0 >> [ 983.923705] entry_SYSCALL_64_after_hwframe+0x65/0xca >> [ 983.923707] RIP: 0033:0x7f080f10ee1b >> >> When getxattr calls xfs_attr_node_get, xfs_da3_node_lookup_int fails in >> xfs_attr_node_hasname because we have use blocktrash to random it in xfs/126. So it >> free stat and xfs_attr_node_get doesn't do xfs_buf_trans release job. >> >> Then subsequent removexattr will hang because of it. >> >> This bug was introduced by kernel commit 07120f1abdff ("xfs: Add xfs_has_attr and subroutines"). >> It adds xfs_attr_node_hasname helper and said caller will be responsible for freeing the state >> in this case. But xfs_attr_node_hasname will free stat itself instead of caller if >> xfs_da3_node_lookup_int fails. >> >> Fix this bug by moving the step of free state into caller. >> >> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines") >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com> > > Ah, I knew this function was gross. Before, we would set *statep to > NULL upon entry to the function, and we would pass the newly allocated > da state back out if !error. However, the caller has no idea if the > return value came from error or retval, other than (I guess) the comment > implies (or had better imply) that ENOATTR/EEXIST only come from retval. > > Now you're changing it to always pass state out via **statep even if the > da3 lookup returns error and we want to pass that out. But then > xfs_attr_node_addname_find_attr does this: > > retval = xfs_attr_node_hasname(args,&dac->da_state); > if (retval != -ENOATTR&& retval != -EEXIST) > return error; > > without ever clearing dac->da_state. Won't that leak the da state? Yes. We should clear dac->da_state here by using goto error instead of return error. > > Granted, I wonder if the xfs_attr_node_hasname call in > xfs_attr_node_removename_setup will also leak the state if the return > value is ENOATTR? Yes, it will also leak the state if error is not EEXIST. Will fix them in v2. > > If you ask me the whole ENOATTR/EEXIST thing still needs to be replaced > with an enum xfs_attr_lookup_result passed out separately so that we > don't have to think about which magic errno values are not really > errors. Agree. Best Regards Yang Xu > > --D > >> --- >> fs/xfs/libxfs/xfs_attr.c | 13 +++++-------- >> 1 file changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index fbc9d816882c..6ad50a76fd8d 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1077,21 +1077,18 @@ xfs_attr_node_hasname( >> >> state = xfs_da_state_alloc(args); >> if (statep != NULL) >> - *statep = NULL; >> + *statep = state; >> >> /* >> * Search to see if name exists, and get back a pointer to it. >> */ >> error = xfs_da3_node_lookup_int(state,&retval); >> - if (error) { >> - xfs_da_state_free(state); >> - return error; >> - } >> + if (error) >> + retval = error; >> >> - if (statep != NULL) >> - *statep = state; >> - else >> + if (!statep) >> xfs_da_state_free(state); >> + >> return retval; >> } >> >> -- >> 2.23.0 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index fbc9d816882c..6ad50a76fd8d 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1077,21 +1077,18 @@ xfs_attr_node_hasname( state = xfs_da_state_alloc(args); if (statep != NULL) - *statep = NULL; + *statep = state; /* * Search to see if name exists, and get back a pointer to it. */ error = xfs_da3_node_lookup_int(state, &retval); - if (error) { - xfs_da_state_free(state); - return error; - } + if (error) + retval = error; - if (statep != NULL) - *statep = state; - else + if (!statep) xfs_da_state_free(state); + return retval; }
When testing xfstests xfs/126 on lastest upstream kernel, it will hang on some machine. Adding a getxattr operation after xattr corrupted, I can reproduce it 100%. The deadlock as below: [983.923403] task:setfattr state:D stack: 0 pid:17639 ppid: 14687 flags:0x00000080 [ 983.923405] Call Trace: [ 983.923410] __schedule+0x2c4/0x700 [ 983.923412] schedule+0x37/0xa0 [ 983.923414] schedule_timeout+0x274/0x300 [ 983.923416] __down+0x9b/0xf0 [ 983.923451] ? xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] [ 983.923453] down+0x3b/0x50 [ 983.923471] xfs_buf_lock+0x33/0xf0 [xfs] [ 983.923490] xfs_buf_find.isra.29+0x3c8/0x5f0 [xfs] [ 983.923508] xfs_buf_get_map+0x4c/0x320 [xfs] [ 983.923525] xfs_buf_read_map+0x53/0x310 [xfs] [ 983.923541] ? xfs_da_read_buf+0xcf/0x120 [xfs] [ 983.923560] xfs_trans_read_buf_map+0x1cf/0x360 [xfs] [ 983.923575] ? xfs_da_read_buf+0xcf/0x120 [xfs] [ 983.923590] xfs_da_read_buf+0xcf/0x120 [xfs] [ 983.923606] xfs_da3_node_read+0x1f/0x40 [xfs] [ 983.923621] xfs_da3_node_lookup_int+0x69/0x4a0 [xfs] [ 983.923624] ? kmem_cache_alloc+0x12e/0x270 [ 983.923637] xfs_attr_node_hasname+0x6e/0xa0 [xfs] [ 983.923651] xfs_has_attr+0x6e/0xd0 [xfs] [ 983.923664] xfs_attr_set+0x273/0x320 [xfs] [ 983.923683] xfs_xattr_set+0x87/0xd0 [xfs] [ 983.923686] __vfs_removexattr+0x4d/0x60 [ 983.923688] __vfs_removexattr_locked+0xac/0x130 [ 983.923689] vfs_removexattr+0x4e/0xf0 [ 983.923690] removexattr+0x4d/0x80 [ 983.923693] ? __check_object_size+0xa8/0x16b [ 983.923695] ? strncpy_from_user+0x47/0x1a0 [ 983.923696] ? getname_flags+0x6a/0x1e0 [ 983.923697] ? _cond_resched+0x15/0x30 [ 983.923699] ? __sb_start_write+0x1e/0x70 [ 983.923700] ? mnt_want_write+0x28/0x50 [ 983.923701] path_removexattr+0x9b/0xb0 [ 983.923702] __x64_sys_removexattr+0x17/0x20 [ 983.923704] do_syscall_64+0x5b/0x1a0 [ 983.923705] entry_SYSCALL_64_after_hwframe+0x65/0xca [ 983.923707] RIP: 0033:0x7f080f10ee1b When getxattr calls xfs_attr_node_get, xfs_da3_node_lookup_int fails in xfs_attr_node_hasname because we have use blocktrash to random it in xfs/126. So it free stat and xfs_attr_node_get doesn't do xfs_buf_trans release job. Then subsequent removexattr will hang because of it. This bug was introduced by kernel commit 07120f1abdff ("xfs: Add xfs_has_attr and subroutines"). It adds xfs_attr_node_hasname helper and said caller will be responsible for freeing the state in this case. But xfs_attr_node_hasname will free stat itself instead of caller if xfs_da3_node_lookup_int fails. Fix this bug by moving the step of free state into caller. Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines") Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- fs/xfs/libxfs/xfs_attr.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)