Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
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
>> Signed-off-by: Liu Bo
>> ---
>
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote: > On Thu, Apr 05, 2018 at 04:42:34PM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
On Thu, Apr 05, 2018 at 07:11:14PM +0200, David Sterba wrote: >On Thu, Apr 05, 2018 at 04:42:34PM +, 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.-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
On Thu, Apr 05, 2018 at 04:42:34PM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
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
> Signed-off-by: Liu Bo
> ---
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
}
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
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-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Btrfs: fix NULL pointer dereference in log_dir_items
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
Signed-off-by: Liu Bo
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
