linux-kernel - Re: [PATCH] btrfs: let btrfs_delete_unused

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b5c2852a-fe0a-834e-0fcd-2b61b5c47088@fb.com>
Date:   Fri, 2 Sep 2016 09:35:31 -0400
From:   Josef Bacik <jbacik@...com>
To:     Naohiro Aota <naohiro.aota@...t.com>, <linux-btrfs@...r.kernel.org>
CC:     Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs: let btrfs_delete_unused_bgs() to clean relocated
 bgs

On 09/02/2016 03:46 AM, Naohiro Aota wrote:
> Currently, btrfs_relocate_chunk() is removing relocated BG by itself. But
> the work can be done by btrfs_delete_unused_bgs() (and it's better since it
> trim the BG). Let's dedupe the code.
>
> While btrfs_delete_unused_bgs() is already hitting the relocated BG, it
> skip the BG since the BG has "ro" flag set (to keep balancing BG intact).
> On the other hand, btrfs cannot drop "ro" flag here to prevent additional
> writes. So this patch make use of "removed" flag.
> btrfs_delete_unused_bgs() now detect the flag to distinguish whether a
> read-only BG is relocating or not.
>

This seems racey to me.  We remove the last part of the block group, it ends up 
on the unused_bgs_list, we process this list, see that removed isn't set and we 
skip it, then later we set removed, but it's too late.  I think the right way is 
to actually do a transaction, set ->removed, manually add it to the 
unused_bgs_list if it's not already, then end the transaction.  This way we are 
guaranteed to have the bg on the list when it is ready to be removed.  This is 
my analysis after looking at it for 10 seconds after being awake for like 30 
minutes so if I'm missing something let me know.  Thanks,

Josef