On Thu, Feb 11, 2021 at 01:25:52PM +0200, Nikolay Borisov wrote: > > > On 11.02.21 г. 0:50 ч., David Sterba wrote: > > On Tue, Jan 26, 2021 at 09:30:45AM -0500, Josef Bacik wrote: > >> On 1/26/21 4:02 AM, Nikolay Borisov wrote: > >>> On 25.01.21 г. 23:42 ч., Josef Bacik wrote: > >>>> In __btrfs_return_cluster_to_free_space we will bail doing the cleanup > >>>> of the cluster if the block group we passed in doesn't match the block > >>>> group on the cluster. However we drop a reference to block_group, as > >>>> the cluster holds a reference to the block group while it's attached to > >>>> the cluster. If cluster->block_group != block_group however then this > >>>> is an extra put, which means we'll go negative and free this block group > >>>> down the line, leading to a UAF. > >>> > >>> Was this found by code inspection or did you hit in production. Also why > >>> in btrfs_remove_free_space_cache just before > >>> __btrfs_return_cluster_to_free_space there is: > >>> > >> > >> It was found in production sort of halfway. I was doing something for > >> WhatsApp > >> and had to convert our block group reference counting to the refcount > >> stuff so I > >> could find where I made a mistake. Turns out this was where the problem > >> was, my > >> stuff had just made it way more likely to happen. I don't have the stack > >> trace > >> because this was like 6 months ago, I'm going through all my WhatsApp > >> magic and > >> getting them actually usable for upstream. > >> > >>> WARN_ON(cluster->block_group != block_group); > >>> > >>> IMO this patch should also remove the WARN_ON if it's a valid condition > >>> to have the passed bg be different than the one in the cluster. Also > >>> that WARN_ON is likely racy since it's done outside of cluster->lock. > >>> > >> > >> Yup that's in a follow up thing, I wanted to get the actual fix out before > >> I got > >> distracted by my mountain of meetings this week. Thanks, > > > > Removing the WARN_ON in a separate patch sounds ok to me, this patch > > clearly fixes the refcounting bug, the warning condition is the same but > > would need a different reasoning. > > > > Nikolay, if you're ok with current patch version let me know if you want > > a rev-by added. > > > > > Codewise I'm fine with it. However just had another read of the commit > message and I think it could be rewritten to be somewhat simpler: > > It's wrong calling btrfs_put_block_group in > __btrfs_return_cluster_to_free_space if the block group passed is > different than the block group the cluster represents. As this means the > cluster doesn't have a reference to the passed block group. This results > in double put and an UAF. > > What prompted me is that the 2nd and 3rd sentences read somewhat awkward > due to starting with 'However'
Ok, updated, thanks. I left the last paragraph "Fix that ...".
