| Message ID | 20200515021731.cb5y557wsxf66fo3@debian.debian-2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] fs/btrfs: Fix unlocking in btrfs_ref_tree_mod | expand |
On 15/05/2020 04:17, Bo YU wrote: > It adds spin_lock() in add_block_entry() but out path does not unlock > it. Which call path doesn't unlock it? There is an out_unlock label with a spin_unlock() right above your insert. So either coverity messed something up or the call path that needs the unlock has to jump to out_unlock instead of out.
Hi, On Fri, May 15, 2020 at 5:03 PM Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > > On 15/05/2020 04:17, Bo YU wrote: > > It adds spin_lock() in add_block_entry() but out path does not unlock > > it. > > Which call path doesn't unlock it? There is an out_unlock label with a > spin_unlock() right above your insert. So either coverity messed something > up or the call path that needs the unlock has to jump to out_unlock instead > of out. This is out label without unlocking it. It will be offered spin_lock in add_block_entry() for be. But here I was worried about that unlock it in if() whether it is right or not.
On 15/05/2020 11:24, Bo YU wrote: > Hi, > On Fri, May 15, 2020 at 5:03 PM Johannes Thumshirn > <Johannes.Thumshirn@wdc.com> wrote: >> >> On 15/05/2020 04:17, Bo YU wrote: >>> It adds spin_lock() in add_block_entry() but out path does not unlock >>> it. >> >> Which call path doesn't unlock it? There is an out_unlock label with a >> spin_unlock() right above your insert. So either coverity messed something >> up or the call path that needs the unlock has to jump to out_unlock instead >> of out. > This is out label without unlocking it. It will be offered spin_lock > in add_block_entry() > for be. But here I was worried about that unlock it in if() whether it > is right or not. > No add_block_entry() returns with the ref_verify_lock held on success only: static struct block_entry *add_block_entry(struct btrfs_fs_info *fs_info, u64 bytenr, u64 len, u64 root_objectid) { struct block_entry *be = NULL, *exist; struct root_entry *re = NULL; re = kzalloc(sizeof(struct root_entry), GFP_KERNEL); be = kzalloc(sizeof(struct block_entry), GFP_KERNEL); if (!be || !re) { kfree(re); kfree(be); return ERR_PTR(-ENOMEM); } be->bytenr = bytenr; be->len = len; re->root_objectid = root_objectid; re->num_refs = 0; spin_lock(&fs_info->ref_verify_lock); [...] While the code caller checks for an error: if (action == BTRFS_ADD_DELAYED_EXTENT) { /* * For subvol_create we'll just pass in whatever the parent root * is and the new root objectid, so let's not treat the passed * in root as if it really has a ref for this bytenr. */ be = add_block_entry(fs_info, bytenr, num_bytes, ref_root); if (IS_ERR(be)) { kfree(ref); kfree(ra); ret = PTR_ERR(be); goto out; } So if add_block_entry returns -ENOMEM it didn't take the lock and thus no unlock is needed. Or did I miss something?
On Fri, May 15, 2020 at 5:36 PM Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > > On 15/05/2020 11:24, Bo YU wrote: > > Hi, > > On Fri, May 15, 2020 at 5:03 PM Johannes Thumshirn > > <Johannes.Thumshirn@wdc.com> wrote: > >> > >> On 15/05/2020 04:17, Bo YU wrote: > >>> It adds spin_lock() in add_block_entry() but out path does not unlock > >>> it. > >> > >> Which call path doesn't unlock it? There is an out_unlock label with a > >> spin_unlock() right above your insert. So either coverity messed something > >> up or the call path that needs the unlock has to jump to out_unlock instead > >> of out. > > This is out label without unlocking it. It will be offered spin_lock > > in add_block_entry() > > for be. But here I was worried about that unlock it in if() whether it > > is right or not. > > > > No add_block_entry() returns with the ref_verify_lock held on success only: > static struct block_entry *add_block_entry(struct btrfs_fs_info *fs_info, > u64 bytenr, u64 len, > u64 root_objectid) > { > struct block_entry *be = NULL, *exist; > struct root_entry *re = NULL; > > re = kzalloc(sizeof(struct root_entry), GFP_KERNEL); > be = kzalloc(sizeof(struct block_entry), GFP_KERNEL); > if (!be || !re) { > kfree(re); > kfree(be); > return ERR_PTR(-ENOMEM); > } > be->bytenr = bytenr; > be->len = len; > > re->root_objectid = root_objectid; > re->num_refs = 0; > > spin_lock(&fs_info->ref_verify_lock); > [...] > > > While the code caller checks for an error: > > if (action == BTRFS_ADD_DELAYED_EXTENT) { > /* > * For subvol_create we'll just pass in whatever the parent root > * is and the new root objectid, so let's not treat the passed > * in root as if it really has a ref for this bytenr. > */ > be = add_block_entry(fs_info, bytenr, num_bytes, ref_root); > if (IS_ERR(be)) { > kfree(ref); > kfree(ra); > ret = PTR_ERR(be); > goto out; > } > > So if add_block_entry returns -ENOMEM it didn't take the lock and thus no unlock > is needed. Ok, I got it. Please drop it. Thank you! > > Or did I miss something?
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c index 7887317033c9..8f644511006d 100644 --- a/fs/btrfs/ref-verify.c +++ b/fs/btrfs/ref-verify.c @@ -894,8 +894,10 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info, out_unlock: spin_unlock(&fs_info->ref_verify_lock); out: - if (ret) + if (ret) { + spin_unlock(&fs_info->ref_verify_lock); btrfs_clear_opt(fs_info->mount_opt, REF_VERIFY); + } return ret; }
It adds spin_lock() in add_block_entry() but out path does not unlock it. Detected by CoversityScan, CID# 1463343:(Missing unlock) Fixes: fd708b81d972a (Btrfs: add a extent ref verify tool) Signed-off-by: Bo YU <tsu.yubo@gmail.com> --- fs/btrfs/ref-verify.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.11.0