| Message ID | 20241019092357.212439-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: Fix passing 0 to ERR_PTR in btrfs_search_dir_index_item() | expand |
On 19.10.24 11:07, Yue Haibing wrote: > Return NULL instead of passing to ERR_PTR while ret is zero, this fix > smatch warnings: > > fs/btrfs/dir-item.c:353 > btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR' > > Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item") > Signed-off-by: Yue Haibing <yuehaibing@huawei.com> > --- > fs/btrfs/dir-item.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c > index 001c0c2f872c..cdb30ec7366a 100644 > --- a/fs/btrfs/dir-item.c > +++ b/fs/btrfs/dir-item.c > @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, > if (ret > 0) > ret = 0; > > - return ERR_PTR(ret); > + return ret ? ERR_PTR(ret) : NULL; > } > > struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans, The only caller to this is in btrfs_unlink_subvol(), which does the following: di = btrfs_search_dir_index_item(root, path, dir_ino, &fname.disk_name); if (IS_ERR_OR_NULL(di)) { if (!di) ret = -ENOENT; else ret = PTR_ERR(di); btrfs_abort_transaction(trans, ret); goto out; } to do: diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c index d3093eba54a5..e755228d909a 100644 --- a/fs/btrfs/dir-item.c +++ b/fs/btrfs/dir-item.c @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, return di; } /* Adjust return code if the key was not found in the next leaf. */ - if (ret > 0) - ret = 0; - - return ERR_PTR(ret); + return ERR_PTR(-ENOENT); } struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans, diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 35f89d14c110..00602634db3a 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans, */ if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) { di = btrfs_search_dir_index_item(root, path, dir_ino, &fname.disk_name); - if (IS_ERR_OR_NULL(di)) { - if (!di) - ret = -ENOENT; - else - ret = PTR_ERR(di); + if (IS_ERR(di)) { + ret = PTR_ERR(di); btrfs_abort_transaction(trans, ret); goto out; } This is completely untested though and needs to be re-checked if it's even correct.
On 2024/10/21 16:25, Johannes Thumshirn wrote: > On 19.10.24 11:07, Yue Haibing wrote: >> Return NULL instead of passing to ERR_PTR while ret is zero, this fix >> smatch warnings: >> >> fs/btrfs/dir-item.c:353 >> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR' >> >> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item") >> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >> --- >> fs/btrfs/dir-item.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c >> index 001c0c2f872c..cdb30ec7366a 100644 >> --- a/fs/btrfs/dir-item.c >> +++ b/fs/btrfs/dir-item.c >> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, >> if (ret > 0) >> ret = 0; >> >> - return ERR_PTR(ret); >> + return ret ? ERR_PTR(ret) : NULL; >> } >> >> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans, > > The only caller to this is in btrfs_unlink_subvol(), which does the > following: > > > di = btrfs_search_dir_index_item(root, path, dir_ino, > &fname.disk_name); > if (IS_ERR_OR_NULL(di)) { > if (!di) > ret = -ENOENT; > else > ret = PTR_ERR(di); > btrfs_abort_transaction(trans, ret); > goto out; > } > > to do: > > diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c > index d3093eba54a5..e755228d909a 100644 > --- a/fs/btrfs/dir-item.c > +++ b/fs/btrfs/dir-item.c > @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root > *root, struct btrfs_path *path, > return di; > } > /* Adjust return code if the key was not found in the next leaf. */ ret is output variable of btrfs_for_each_slot, that return value can be 0, if a valid slot was found, 1 if there were no more leaves, and < 0 if there was an error. > - if (ret > 0) > - ret = 0; > - > - return ERR_PTR(ret); > + return ERR_PTR(-ENOENT); This overwrite other ret code, which expecting return to upstream caller > } > > struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle > *trans, > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 35f89d14c110..00602634db3a 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct > btrfs_trans_handle *trans, > */ > if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) { > di = btrfs_search_dir_index_item(root, path, dir_ino, > &fname.disk_name); > - if (IS_ERR_OR_NULL(di)) { > - if (!di) > - ret = -ENOENT; > - else > - ret = PTR_ERR(di); > + if (IS_ERR(di)) { > + ret = PTR_ERR(di); > btrfs_abort_transaction(trans, ret); > goto out; > } > This is completely untested though and needs to be re-checked if it's > even correct.
On 22.10.24 05:22, Yue Haibing wrote: > On 2024/10/21 16:25, Johannes Thumshirn wrote: >> On 19.10.24 11:07, Yue Haibing wrote: >>> Return NULL instead of passing to ERR_PTR while ret is zero, this fix >>> smatch warnings: >>> >>> fs/btrfs/dir-item.c:353 >>> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR' >>> >>> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item") >>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >>> --- >>> fs/btrfs/dir-item.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c >>> index 001c0c2f872c..cdb30ec7366a 100644 >>> --- a/fs/btrfs/dir-item.c >>> +++ b/fs/btrfs/dir-item.c >>> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, >>> if (ret > 0) >>> ret = 0; >>> >>> - return ERR_PTR(ret); >>> + return ret ? ERR_PTR(ret) : NULL; >>> } >>> >>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans, >> >> The only caller to this is in btrfs_unlink_subvol(), which does the >> following: >> >> >> di = btrfs_search_dir_index_item(root, path, dir_ino, >> &fname.disk_name); >> if (IS_ERR_OR_NULL(di)) { >> if (!di) >> ret = -ENOENT; >> else >> ret = PTR_ERR(di); >> btrfs_abort_transaction(trans, ret); >> goto out; >> } >> >> to do: >> >> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c >> index d3093eba54a5..e755228d909a 100644 >> --- a/fs/btrfs/dir-item.c >> +++ b/fs/btrfs/dir-item.c >> @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root >> *root, struct btrfs_path *path, >> return di; >> } >> /* Adjust return code if the key was not found in the next leaf. */ > > > ret is output variable of btrfs_for_each_slot, that return value can be 0, if a > valid slot was found, 1 if there were no more leaves, and < 0 if there was an > error. > Yes. >> - if (ret > 0) >> - ret = 0; >> - >> - return ERR_PTR(ret); >> + return ERR_PTR(-ENOENT); > > This overwrite other ret code, which expecting return to upstream caller Right for ret < 0, but for ret >= 0 we set it to 0 and then do return (void*)0 a.k.a. return NULL. > >> } >> >> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle >> *trans, >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 35f89d14c110..00602634db3a 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct >> btrfs_trans_handle *trans, >> */ >> if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) { >> di = btrfs_search_dir_index_item(root, path, dir_ino, >> &fname.disk_name); >> - if (IS_ERR_OR_NULL(di)) { >> - if (!di) >> - ret = -ENOENT; and then set it to ENOENT if it is NULL. So it should be if (ret >= 0) ret = -ENOENT; return ERR_PTR(-ENOENT); and in the caller if (IS_ERR(di)) { ret = PTR_ERR(di); btrfs_abort_transaction(...); break; }
On 2024/10/22 14:37, Johannes Thumshirn wrote: > On 22.10.24 05:22, Yue Haibing wrote: >> On 2024/10/21 16:25, Johannes Thumshirn wrote: >>> On 19.10.24 11:07, Yue Haibing wrote: >>>> Return NULL instead of passing to ERR_PTR while ret is zero, this fix >>>> smatch warnings: >>>> >>>> fs/btrfs/dir-item.c:353 >>>> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR' >>>> >>>> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item") >>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >>>> --- >>>> fs/btrfs/dir-item.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c >>>> index 001c0c2f872c..cdb30ec7366a 100644 >>>> --- a/fs/btrfs/dir-item.c >>>> +++ b/fs/btrfs/dir-item.c >>>> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, >>>> if (ret > 0) >>>> ret = 0; >>>> >>>> - return ERR_PTR(ret); >>>> + return ret ? ERR_PTR(ret) : NULL; >>>> } >>>> >>>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans, >>> >>> The only caller to this is in btrfs_unlink_subvol(), which does the >>> following: >>> >>> >>> di = btrfs_search_dir_index_item(root, path, dir_ino, >>> &fname.disk_name); >>> if (IS_ERR_OR_NULL(di)) { >>> if (!di) >>> ret = -ENOENT; >>> else >>> ret = PTR_ERR(di); >>> btrfs_abort_transaction(trans, ret); >>> goto out; >>> } >>> >>> to do: >>> >>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c >>> index d3093eba54a5..e755228d909a 100644 >>> --- a/fs/btrfs/dir-item.c >>> +++ b/fs/btrfs/dir-item.c >>> @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root >>> *root, struct btrfs_path *path, >>> return di; >>> } >>> /* Adjust return code if the key was not found in the next leaf. */ >> >> >> ret is output variable of btrfs_for_each_slot, that return value can be 0, if a >> valid slot was found, 1 if there were no more leaves, and < 0 if there was an >> error. >> > > Yes. > >>> - if (ret > 0) >>> - ret = 0; >>> - >>> - return ERR_PTR(ret); >>> + return ERR_PTR(-ENOENT); >> >> This overwrite other ret code, which expecting return to upstream caller > > Right for ret < 0, but for ret >= 0 we set it to 0 and then do return > (void*)0 a.k.a. return NULL. > >> >>> } >>> >>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle >>> *trans, >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >>> index 35f89d14c110..00602634db3a 100644 >>> --- a/fs/btrfs/inode.c >>> +++ b/fs/btrfs/inode.c >>> @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct >>> btrfs_trans_handle *trans, >>> */ >>> if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) { >>> di = btrfs_search_dir_index_item(root, path, dir_ino, >>> &fname.disk_name); >>> - if (IS_ERR_OR_NULL(di)) { >>> - if (!di) >>> - ret = -ENOENT; > > > and then set it to ENOENT if it is NULL. So it should be > > if (ret >= 0) > ret = -ENOENT; > return ERR_PTR(-ENOENT); Here should be return ERR_PTR(ret); Will rework and send v2, thanks! > > and in the caller > > if (IS_ERR(di)) { > ret = PTR_ERR(di); > btrfs_abort_transaction(...); > break; > } > >
On 22.10.24 08:53, Yue Haibing wrote: >> and then set it to ENOENT if it is NULL. So it should be >> >> if (ret >= 0) >> ret = -ENOENT; >> return ERR_PTR(-ENOENT); > > Here should be > return ERR_PTR(ret); Yep of cause.
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c index 001c0c2f872c..cdb30ec7366a 100644 --- a/fs/btrfs/dir-item.c +++ b/fs/btrfs/dir-item.c @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path, if (ret > 0) ret = 0; - return ERR_PTR(ret); + return ret ? ERR_PTR(ret) : NULL; } struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
Return NULL instead of passing to ERR_PTR while ret is zero, this fix smatch warnings: fs/btrfs/dir-item.c:353 btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR' Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item") Signed-off-by: Yue Haibing <yuehaibing@huawei.com> --- fs/btrfs/dir-item.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)