| Message ID | 1524660809-9065-1-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <nborisov@suse.com> > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <nborisov@suse.com> >> --- >> 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 majordomo@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <nborisov@suse.com> >>> --- >>> 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 majordomo@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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];
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 <nborisov@suse.com> --- fs/btrfs/ctree.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)