Re: [PATCH] btrfs: Fix passing 0 to ERR_PTR in btrfs_search_dir_index_item()

[Johannes Thumshirn]

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@xxxxxxxxxx>
>>> ---
>>>    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;
}