Re: [PATCH] btrfs: Change bit number of BTRFS_FS_BALANCE_RUNNING
On 25.04.2018 18:18, Anand Jain wrote:
>
>
> On 04/25/2018 09:16 PM, David Sterba wrote:
>> On Wed, Apr 25, 2018 at 03:53:29PM +0300, Nikolay Borisov wrote:
>>> Commit ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
>>> which introduced this bit assigned it number 17. Unfortunately this bit
>>> is already occupied by the BTRFS_FS_NEED_ASYNC_COMMIT bit.
>
>
>>> This was
>>> causing bit 17 to be cleared while __btrfs_balance was running which
>>> resulted in fs_info->balance_ctl being deleted
>
> Any idea which thread deleted the fs_info::balance_ctl while balance
> was still running?
So what happened is that the test's btrfs balance stop ioctl was allowed
to proceed while __btrfs_balance was running. And this was due to bit 17
being cleared by btrfs_commit_transaction.
>
> Thanks for catching.
> -Anand
>
>
>>> while we have balance
>>> running. This manifested in an UAF crash. Fix it by putting the
>>> definition of the newly introduced balance bit after NEED_ASYNC_COMMIT
>>> and giving it number 18.
>>>
>>> Fixes: ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
>>
>> Uh, thanks for catching it. The bit was free when the volume mutex
>> removal patchset was in development, but the number 17 got used by the
>> recent qgroup patch and I did not adjust it afterwards.
>>
>>> Signed-off-by: Nikolay Borisov
>>> ---
>>> fs/btrfs/ctree.h | 12 ++--
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 59998d5f6985..5a7893d7c668 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -733,16 +733,16 @@ struct btrfs_delayed_root;
>>> */
>>> #define BTRFS_FS_EXCL_OP 16
>>> /*
>>> - * Indicate that balance has been set up from the ioctl and is in
>>> the main
>>> - * phase. The fs_info::balance_ctl is initialized.
>>> - */
>>> -#define BTRFS_FS_BALANCE_RUNNING 17
>>> -
>>> -/*
>>> * To info transaction_kthread we need an immediate commit so it
>>> doesn't
>>> * need to wait for commit_interval
>>> */
>>> #define BTRFS_FS_NEED_ASYNC_COMMIT 17
>>> +/*
>>> + * Indicate that balance has been set up from the ioctl and is in
>>> the main
>>> + * phase. The fs_info::balance_ctl is initialized.
>>> + */
>>> +#define BTRFS_FS_BALANCE_RUNNING 18
>>
>> I'll fold the fix so we don't have an intermediate breakage in the
>> history.
>> --
>> 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] btrfs: Change bit number of BTRFS_FS_BALANCE_RUNNING
On 04/25/2018 09:16 PM, David Sterba wrote:
On Wed, Apr 25, 2018 at 03:53:29PM +0300, Nikolay Borisov wrote:
Commit ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
which introduced this bit assigned it number 17. Unfortunately this bit
is already occupied by the BTRFS_FS_NEED_ASYNC_COMMIT bit.
This was
causing bit 17 to be cleared while __btrfs_balance was running which
resulted in fs_info->balance_ctl being deleted
Any idea which thread deleted the fs_info::balance_ctl while balance
was still running?
Thanks for catching.
-Anand
while we have balance
running. This manifested in an UAF crash. Fix it by putting the
definition of the newly introduced balance bit after NEED_ASYNC_COMMIT
and giving it number 18.
Fixes: ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
Uh, thanks for catching it. The bit was free when the volume mutex
removal patchset was in development, but the number 17 got used by the
recent qgroup patch and I did not adjust it afterwards.
Signed-off-by: Nikolay Borisov
---
fs/btrfs/ctree.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 59998d5f6985..5a7893d7c668 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -733,16 +733,16 @@ struct btrfs_delayed_root;
*/
#define BTRFS_FS_EXCL_OP 16
/*
- * Indicate that balance has been set up from the ioctl and is in the main
- * phase. The fs_info::balance_ctl is initialized.
- */
-#define BTRFS_FS_BALANCE_RUNNING 17
-
-/*
* To info transaction_kthread we need an immediate commit so it doesn't
* need to wait for commit_interval
*/
#define BTRFS_FS_NEED_ASYNC_COMMIT17
+/*
+ * Indicate that balance has been set up from the ioctl and is in the main
+ * phase. The fs_info::balance_ctl is initialized.
+ */
+#define BTRFS_FS_BALANCE_RUNNING 18
I'll fold the fix so we don't have an intermediate breakage in the
history.
--
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] btrfs: Change bit number of BTRFS_FS_BALANCE_RUNNING
On Wed, Apr 25, 2018 at 03:53:29PM +0300, Nikolay Borisov wrote:
> Commit ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
> which introduced this bit assigned it number 17. Unfortunately this bit
> is already occupied by the BTRFS_FS_NEED_ASYNC_COMMIT bit. This was
> causing bit 17 to be cleared while __btrfs_balance was running which
> resulted in fs_info->balance_ctl being deleted while we have balance
> running. This manifested in an UAF crash. Fix it by putting the
> definition of the newly introduced balance bit after NEED_ASYNC_COMMIT
> and giving it number 18.
>
> Fixes: ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
Uh, thanks for catching it. The bit was free when the volume mutex
removal patchset was in development, but the number 17 got used by the
recent qgroup patch and I did not adjust it afterwards.
> Signed-off-by: Nikolay Borisov
> ---
> fs/btrfs/ctree.h | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 59998d5f6985..5a7893d7c668 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -733,16 +733,16 @@ struct btrfs_delayed_root;
> */
> #define BTRFS_FS_EXCL_OP 16
> /*
> - * Indicate that balance has been set up from the ioctl and is in the main
> - * phase. The fs_info::balance_ctl is initialized.
> - */
> -#define BTRFS_FS_BALANCE_RUNNING 17
> -
> -/*
> * To info transaction_kthread we need an immediate commit so it doesn't
> * need to wait for commit_interval
> */
> #define BTRFS_FS_NEED_ASYNC_COMMIT 17
> +/*
> + * Indicate that balance has been set up from the ioctl and is in the main
> + * phase. The fs_info::balance_ctl is initialized.
> + */
> +#define BTRFS_FS_BALANCE_RUNNING 18
I'll fold the fix so we don't have an intermediate breakage in the
history.
--
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] btrfs: Change bit number of BTRFS_FS_BALANCE_RUNNING
Commit ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
which introduced this bit assigned it number 17. Unfortunately this bit
is already occupied by the BTRFS_FS_NEED_ASYNC_COMMIT bit. This was
causing bit 17 to be cleared while __btrfs_balance was running which
resulted in fs_info->balance_ctl being deleted while we have balance
running. This manifested in an UAF crash. Fix it by putting the
definition of the newly introduced balance bit after NEED_ASYNC_COMMIT
and giving it number 18.
Fixes: ddd93ef3b9d6 ("btrfs: track running balance in a simpler way")
Signed-off-by: Nikolay Borisov
---
fs/btrfs/ctree.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 59998d5f6985..5a7893d7c668 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -733,16 +733,16 @@ struct btrfs_delayed_root;
*/
#define BTRFS_FS_EXCL_OP 16
/*
- * Indicate that balance has been set up from the ioctl and is in the main
- * phase. The fs_info::balance_ctl is initialized.
- */
-#define BTRFS_FS_BALANCE_RUNNING 17
-
-/*
* To info transaction_kthread we need an immediate commit so it doesn't
* need to wait for commit_interval
*/
#define BTRFS_FS_NEED_ASYNC_COMMIT 17
+/*
+ * Indicate that balance has been set up from the ioctl and is in the main
+ * phase. The fs_info::balance_ctl is initialized.
+ */
+#define BTRFS_FS_BALANCE_RUNNING 18
+
struct btrfs_fs_info {
u8 fsid[BTRFS_FSID_SIZE];
--
2.7.4
--
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
