[PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Liu Bo]

0, 1 and <0 can be returned by btrfs_next_leaf(), and when <0 is
returned, path->nodes[0] could be NULL, log_dir_items lacks such a
check for <0 and we may run into a null pointer dereference panic.

Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize synchronous operations")
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
v2: Add Fixes tag and reviewed-by.

 fs/btrfs/tree-log.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4344577..4ee9431 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3518,8 +3518,11 @@ static noinline int log_dir_items(struct btrfs_trans_handle *trans,
 		 * from this directory and from this transaction
 		 */
 		ret = btrfs_next_leaf(root, path);
-		if (ret == 1) {
-			last_offset = (u64)-1;
+		if (ret) {
+			if (ret == 1)
+				last_offset = (u64)-1;
+			else
+				err = ret;
 			goto done;
 		}
 		btrfs_item_key_to_cpu(path->nodes[0], &tmp, path->slots[0]);
-- 
1.8.3.1

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Sasha Levin]

Hi.

[This is an automated email]

This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 9.9156)

The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 

v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Build OK!
v4.4.126: Build OK!

Please let us know if you'd like to have this patch included in a stable tree.

--
Thanks.
Sasha

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[David Sterba]

On Tue, Apr 03, 2018 at 01:59:47AM +0800, Liu Bo wrote:
> 0, 1 and <0 can be returned by btrfs_next_leaf(), and when <0 is
> returned, path->nodes[0] could be NULL, log_dir_items lacks such a
> check for <0 and we may run into a null pointer dereference panic.
> 
> Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize synchronous operations")
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---

Added to next, thanks.

> v2: Add Fixes tag and reviewed-by.
> 
>  fs/btrfs/tree-log.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4344577..4ee9431 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3518,8 +3518,11 @@ static noinline int log_dir_items(struct btrfs_trans_handle *trans,
>  		 * from this directory and from this transaction
>  		 */
>  		ret = btrfs_next_leaf(root, path);
> -		if (ret == 1) {
> -			last_offset = (u64)-1;
> +		if (ret) {
> +			if (ret == 1)
> +				last_offset = (u64)-1;
> +			else
> +				err = ret;

I wonder if we could find some more consistent if/else pattern of the
error handling for this function. Each caller cares about something else
so it's hard to tell from a quick look which part is the expected one.

Something like:

if (ret < 0) {
	unexpected error
} else if (ret > 0 ) {
	no more leaves, probably a terminating condition
} else {
	more leaves to scan, possibly this can be ommitted in most cases
}

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[David Sterba]

On Thu, Apr 05, 2018 at 04:42:34PM +0000, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]
> 
> This commit has been processed by the -stable helper bot and determined
> to be a high probability candidate for -stable trees. (score: 9.9156)
> 
> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 
> 
> v4.15.15: Build OK!
> v4.14.32: Build OK!
> v4.9.92: Build OK!
> v4.4.126: Build OK!
> 
> Please let us know if you'd like to have this patch included in a stable tree.

Yes, in this case we expect that the Fixes: tag will let the patch flow
to stable after it gets applied to master.

The automated stable candidate patch scanning would be helpful in cases
where the Fixes tag is not identified or we forget to add it. I don't
mind helping to train the bot, so I'll try respond to the messages.

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Sasha Levin]

On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote:
>On Thu, Apr 05, 2018 at 04:42:34PM +0000, Sasha Levin wrote:
>> Hi.
>>
>> [This is an automated email]
>>
>> This commit has been processed by the -stable helper bot and determined
>> to be a high probability candidate for -stable trees. (score: 9.9156)
>>
>> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126,
>>
>> v4.15.15: Build OK!
>> v4.14.32: Build OK!
>> v4.9.92: Build OK!
>> v4.4.126: Build OK!
>>
>> Please let us know if you'd like to have this patch included in a stable tree.
>
>Yes, in this case we expect that the Fixes: tag will let the patch flow
>to stable after it gets applied to master.
>
>The automated stable candidate patch scanning would be helpful in cases
>where the Fixes tag is not identified or we forget to add it. I don't
>mind helping to train the bot, so I'll try respond to the messages.

Just to clarify, having just the "Fixes:" tag is not necessarily an
indicator that a patch should go into -stable.

For example, if I fix up documentation and add a Fixes: tag to point to
the commit that added the original documentation, it's not -stable
material since we don't take documentation patches. Or, if the patch
that the new commit fixes didn't make it into any releases, it's not
stable material either.

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Greg KH]

On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote:
> On Thu, Apr 05, 2018 at 04:42:34PM +0000, Sasha Levin wrote:
> > Hi.
> > 
> > [This is an automated email]
> > 
> > This commit has been processed by the -stable helper bot and determined
> > to be a high probability candidate for -stable trees. (score: 9.9156)
> > 
> > The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 
> > 
> > v4.15.15: Build OK!
> > v4.14.32: Build OK!
> > v4.9.92: Build OK!
> > v4.4.126: Build OK!
> > 
> > Please let us know if you'd like to have this patch included in a stable tree.
> 
> Yes, in this case we expect that the Fixes: tag will let the patch flow
> to stable after it gets applied to master.

Fixes: tags almost never cause that to happen, unless I am bored and go
digging for them.

Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly if you want a patch applied to the stable
tree.

thanks,

greg k-h

Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items

[Nikolay Borisov]

On  5.04.2018 20:02, David Sterba wrote:
> On Tue, Apr 03, 2018 at 01:59:47AM +0800, Liu Bo wrote:
>> 0, 1 and <0 can be returned by btrfs_next_leaf(), and when <0 is
>> returned, path->nodes[0] could be NULL, log_dir_items lacks such a
>> check for <0 and we may run into a null pointer dereference panic.
>>
>> Fixes: e02119d5a7b4 ("Btrfs: Add a write ahead tree log to optimize synchronous operations")
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
> 
> Added to next, thanks.
> 
>> v2: Add Fixes tag and reviewed-by.
>>
>>  fs/btrfs/tree-log.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 4344577..4ee9431 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -3518,8 +3518,11 @@ static noinline int log_dir_items(struct btrfs_trans_handle *trans,
>>  		 * from this directory and from this transaction
>>  		 */
>>  		ret = btrfs_next_leaf(root, path);
>> -		if (ret == 1) {
>> -			last_offset = (u64)-1;
>> +		if (ret) {
>> +			if (ret == 1)
>> +				last_offset = (u64)-1;
>> +			else
>> +				err = ret;
> 
> I wonder if we could find some more consistent if/else pattern of the
> error handling for this function. Each caller cares about something else
> so it's hard to tell from a quick look which part is the expected one.
> 
> Something like:
> 
> if (ret < 0) {
> 	unexpected error
> } else if (ret > 0 ) {
> 	no more leaves, probably a terminating condition
> } else {
> 	more leaves to scan, possibly this can be ommitted in most cases
> }

I agree this is better, generally when I see disparate if branches but
which pertain to the same 'ret' value I tend to rewrite them to follow
if/else if/else. This makes the code 'semantically closer' (for the lack
of a better term).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>