[PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get()

[Eric Sandeen]

OK, thanks Dave. It seemed like it was probably too simple…

> On Jan 30, 2016, at 3:06 PM, Dave Chinner <david at fromorbit.com> wrote:
> 
>> On Fri, Jan 29, 2016 at 09:26:28PM -0600, Eric Sandeen wrote:
>> Commit eef334e added an ASSERT that the inode was locked in
>> some way in xfs_bmapi_read(), but on realtime paths through
>> xfs_rtbuf_get() this isn't the case; fix that.
>> 
>> Reported-by: Ross Zwisler <ross.zwisler at linux.intel.com>
>> Signed-off-by: Eric Sandeen <sandeen at redhat.com>
>> ---
>> 
>> I think we need the data_map_shared gyrations here, but not certain...
>> 
>> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
>> index 9b59ffa..e6da0b2 100644
>> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
>> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
>> @@ -57,11 +57,14 @@ xfs_rtbuf_get(
>>    xfs_inode_t    *ip;        /* bitmap or summary inode */
>>    xfs_bmbt_irec_t    map;
>>    int        nmap = 1;
>> +    int        lock_mode;
>>    int        error;        /* error value */
>> 
>>    ip = issum ? mp->m_rsumip : mp->m_rbmip;
>> 
>> +    lock_mode = xfs_ilock_data_map_shared(ip);
>>    error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
>> +    xfs_iunlock(ip, lock_mode);
> 
> I've looked into this recently and didn't think up a simple answer
> to the problem. I didn't spend much time on it because it's nowhere
> near the top of my priority list because it only affects debug
> kernels as the summary inode is effectively protected by the bitmap
> inode exclusion during allocation.
> 
> That said, the above change is not safe because xfs_rtbuf_get() can
> be called with the bitmap inode lock already held. e.g:
> 
>  xfs_bmap_rtalloc
>    xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);   << locks bitmap
>    xfs_rtallocate_extent
>      xfs_rtallocate_extent_exact
>    xfs_rtcheck_range
>      xfs_rtbuf_get(issum = 0)
>        xfs_ilock_data_map_shared(mp->m_rbmip)
>        <self deadlock>
> 
> The issue here is that the summary inode is not locked early on in
> the transaction, so modifications are done with it unlocked.
> 
>  xfs_bmap_rtalloc
>    xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);   << locks bitmap
>    xfs_rtallocate_extent
>      xfs_rtallocate_extent_size
>        xfs_rtget_summary
>          xfs_rtmodify_summary_int
>        xfs_rtbuf_get
>          xfs_bmapi_read(summary inode)    << unlocked summary
> 
> The only path through which the summary inode is locked for
> modification is the growfs path (xfs_rtcopy_summary()), so all the
> other paths that modify/access the summary inode also need to be
> locked at a higher level before calling into the summary functions.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david at fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>