btrfs_lookup_block_group may fail and return NULL. The fix goes
to out when it fails to avoid NULL pointer dereference.
Signed-off-by: Kangjie Lu <[email protected]>
---
fs/btrfs/extent-tree.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 994f0cc41799..b1e7985bcb9d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
pin = 0;
cache = btrfs_lookup_block_group(fs_info, buf->start);
+ if (!cache)
+ goto out;
if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
pin_down_extent(fs_info, cache, buf->start,
--
2.17.1
On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.
Actually no, in this case btrfs_lookup_block_group must never fail
because if we have an allocated eb then it must have been allocated from
a bg.
>
> Signed-off-by: Kangjie Lu <[email protected]>
> ---
> fs/btrfs/extent-tree.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..b1e7985bcb9d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>
> pin = 0;
> cache = btrfs_lookup_block_group(fs_info, buf->start);
> + if (!cache)
> + goto out;
>
> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
> pin_down_extent(fs_info, cache, buf->start,
>
On 2019/3/14 下午3:50, Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.
>
> Signed-off-by: Kangjie Lu <[email protected]>
> ---
> fs/btrfs/extent-tree.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..b1e7985bcb9d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>
> pin = 0;
> cache = btrfs_lookup_block_group(fs_info, buf->start);
> + if (!cache)
> + goto out;
The check itself is OK.
Reviewed-by: Qu Wenruo <[email protected]>
The problem is, here we're freeing a tree block, if there is no block
group for it, we shouldn't be able to read the extent buffer out.
So it's near impossible to hit. (Unless there is some other things wrong)
Thanks,
Qu
>
> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
> pin_down_extent(fs_info, cache, buf->start,
>
On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>
>
> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>> to out when it fails to avoid NULL pointer dereference.
>
> Actually no, in this case btrfs_lookup_block_group must never fail
> because if we have an allocated eb then it must have been allocated from
> a bg.
Yep, that's the normal case.
However I'm wondering if it's possible to get a bad eb which is cached.
Then we could hit such situation.
So I still believe being safe here still makes sense, especially who
knows future fuzzed image will be.
Thanks,
Qu
>
>>
>> Signed-off-by: Kangjie Lu <[email protected]>
>> ---
>> fs/btrfs/extent-tree.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 994f0cc41799..b1e7985bcb9d 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>
>> pin = 0;
>> cache = btrfs_lookup_block_group(fs_info, buf->start);
>> + if (!cache)
>> + goto out;
>>
>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>> pin_down_extent(fs_info, cache, buf->start,
>>
On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
>
>
> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>
>>
>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>> to out when it fails to avoid NULL pointer dereference.
>>
>> Actually no, in this case btrfs_lookup_block_group must never fail
>> because if we have an allocated eb then it must have been allocated from
>> a bg.
>
> Yep, that's the normal case.
>
> However I'm wondering if it's possible to get a bad eb which is cached.
>
> Then we could hit such situation.
>
> So I still believe being safe here still makes sense, especially who
> knows future fuzzed image will be.
Then I'd rather have ASSERT(cache)
>
> Thanks,
> Qu
>
>>
>>>
>>> Signed-off-by: Kangjie Lu <[email protected]>
>>> ---
>>> fs/btrfs/extent-tree.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 994f0cc41799..b1e7985bcb9d 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>
>>> pin = 0;
>>> cache = btrfs_lookup_block_group(fs_info, buf->start);
>>> + if (!cache)
>>> + goto out;
>>>
>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>> pin_down_extent(fs_info, cache, buf->start,
>>>
>
btrfs_lookup_block_group may fail and return NULL. The fix uses
assert to ensure cache is not NULL.
Signed-off-by: Kangjie Lu <[email protected]>
---
V2: use assert as suggested by Nikolay Borisov <[email protected]>
---
fs/btrfs/extent-tree.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 994f0cc41799..80d7c272d282 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7303,6 +7303,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
pin = 0;
cache = btrfs_lookup_block_group(fs_info, buf->start);
+ ASSERT(cache);
if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
pin_down_extent(fs_info, cache, buf->start,
--
2.17.1
On 14.03.19 г. 10:13 ч., Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix uses
> assert to ensure cache is not NULL.
>
> Signed-off-by: Kangjie Lu <[email protected]>
Reviewed-by: Nikolay Borisov <[email protected]>
>
> ---
> V2: use assert as suggested by Nikolay Borisov <[email protected]>
> ---
> fs/btrfs/extent-tree.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..80d7c272d282 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>
> pin = 0;
> cache = btrfs_lookup_block_group(fs_info, buf->start);
> + ASSERT(cache);
>
> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
> pin_down_extent(fs_info, cache, buf->start,
>
On 2019/3/14 下午4:03, Nikolay Borisov wrote:
>
>
> On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>>
>>>
>>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>>> to out when it fails to avoid NULL pointer dereference.
>>>
>>> Actually no, in this case btrfs_lookup_block_group must never fail
>>> because if we have an allocated eb then it must have been allocated from
>>> a bg.
>>
>> Yep, that's the normal case.
>>
>> However I'm wondering if it's possible to get a bad eb which is cached.
>>
>> Then we could hit such situation.
>>
>> So I still believe being safe here still makes sense, especially who
>> knows future fuzzed image will be.
>
> Then I'd rather have ASSERT(cache)
Isn't assert() a bad idea for production build without assert() support?
Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> Signed-off-by: Kangjie Lu <[email protected]>
>>>> ---
>>>> fs/btrfs/extent-tree.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 994f0cc41799..b1e7985bcb9d 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>>
>>>> pin = 0;
>>>> cache = btrfs_lookup_block_group(fs_info, buf->start);
>>>> + if (!cache)
>>>> + goto out;
>>>>
>>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>> pin_down_extent(fs_info, cache, buf->start,
>>>>
>>
On 14.03.19 г. 11:15 ч., Qu Wenruo wrote:
> Isn't assert() a bad idea for production build without assert() support?
As already discussed this is invariant of the code, if this invariant is
broken then in production builds we'd likely crash (which is good since
we want to understand why and not silently handle it). In debug builds
the assert will be triggered.
On 2019/3/14 4:02 PM, Qu Wenruo wrote:
>
>
> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>
>>
>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>> to out when it fails to avoid NULL pointer dereference.
>>
>> Actually no, in this case btrfs_lookup_block_group must never fail
>> because if we have an allocated eb then it must have been allocated from
>> a bg.
>
> Yep, that's the normal case.
>
> However I'm wondering if it's possible to get a bad eb which is cached.
>
> Then we could hit such situation.
>
> So I still believe being safe here still makes sense, especially who
> knows future fuzzed image will be.
Plus one.
Personally, I'd rather like the version 1.
Thanks,
Su
>
> Thanks,
> Qu
>
>>
>>>
>>> Signed-off-by: Kangjie Lu <[email protected]>
>>> ---
>>> fs/btrfs/extent-tree.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 994f0cc41799..b1e7985bcb9d 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>
>>> pin = 0;
>>> cache = btrfs_lookup_block_group(fs_info, buf->start);
>>> + if (!cache)
>>> + goto out;
>>>
>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>> pin_down_extent(fs_info, cache, buf->start,
>>>
On 3/14/19 4:15 AM, Qu Wenruo wrote:
>
> On 2019/3/14 下午4:03, Nikolay Borisov wrote:
>>
>> On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
>>>
>>> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>>>
>>>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>>>> to out when it fails to avoid NULL pointer dereference.
>>>> Actually no, in this case btrfs_lookup_block_group must never fail
>>>> because if we have an allocated eb then it must have been allocated from
>>>> a bg.
>>> Yep, that's the normal case.
>>>
>>> However I'm wondering if it's possible to get a bad eb which is cached.
>>>
>>> Then we could hit such situation.
>>>
>>> So I still believe being safe here still makes sense, especially who
>>> knows future fuzzed image will be.
>> Then I'd rather have ASSERT(cache)
> Isn't assert() a bad idea for production build without assert() support?
I also agree with that, in general, assert should not be used in
production runs. The first patch might be better.
>
> Thanks,
> Qu
>
>>> Thanks,
>>> Qu
>>>
>>>>> Signed-off-by: Kangjie Lu <[email protected]>
>>>>> ---
>>>>> fs/btrfs/extent-tree.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>> index 994f0cc41799..b1e7985bcb9d 100644
>>>>> --- a/fs/btrfs/extent-tree.c
>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>>>
>>>>> pin = 0;
>>>>> cache = btrfs_lookup_block_group(fs_info, buf->start);
>>>>> + if (!cache)
>>>>> + goto out;
>>>>>
>>>>> if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>>> pin_down_extent(fs_info, cache, buf->start,
>>>>>
On Thu, Mar 14, 2019 at 09:54:07AM +0200, Nikolay Borisov wrote:
>
>
> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
> > btrfs_lookup_block_group may fail and return NULL. The fix goes
> > to out when it fails to avoid NULL pointer dereference.
>
> Actually no, in this case btrfs_lookup_block_group must never fail
> because if we have an allocated eb then it must have been allocated from
> a bg.
>
Agreed, we only get to btrfs_free_tree_block() if we are actually deleting the
extent buffer. We would have had to read in the extent buffer first to get
here, which would have failed if there was no block group. We can't get into
this situation with a specifically crafted file system to exploit this as we'd
bail out well before we could get to btrfs_free_tree_block(). Adding an
ASSERT() makes sure developers aren't doing anything stupid, but again we'd have
to be doing something _super_ stupid to hit it. Thanks,
Josef
On Thu, Mar 14, 2019 at 02:50:40AM -0500, Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.
The subject, changelog and code change are not valid anymore after the
discussion. Please update them accordingly and resend. Thanks.