linux-ext4 - Re: [PATCH 2 2/2] xfs: fix rt

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]

Date:   Thu, 1 Feb 2018 17:13:40 -0700
From:   Dave Jiang <dave.jiang@...el.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     darrick.wong@...cle.com, linux-xfs@...r.kernel.org,
        ross.zwisler@...ux.intel.com, linux-ext4@...r.kernel.org,
        dan.j.williams@...el.com, linux-nvdimm@...ts.01.org
Subject: Re: [PATCH 2 2/2] xfs: fix rt_dev usage for DAX



On 02/01/2018 04:44 PM, Dave Chinner wrote:
> On Thu, Feb 01, 2018 at 01:33:05PM -0700, Dave Jiang wrote:
>> When using realtime device (rtdev) with xfs where the data device is not
>> DAX capable, two issues arise. One is when data device is not DAX but the
>> realtime device is DAX capable, we currently disable DAX.
>> After passing this check, we are also not marking the inode as DAX capable.
>> This change will allow DAX enabled if the data device or the realtime
>> device is DAX capable. S_DAX will be marked for the inode if the file is
>> residing on a DAX capable device. This will prevent the case of rtdev is not
>> DAX and data device is DAX to create realtime files.
> 
> I'm confused by this description. I'm not sure what is broken, nor
> what you are trying to fix.
> 
> I think what you want to do is enable DAX on RT devices separately
> to the data device and vice versa?
> 
> i.e. is this what you are trying to acheive?
> 
> datadev dax	rtdev dax		DAX enabled on
> -----------     ---------		--------------
> no		no			neither
> yes		no			datadev
> no		yes			rtdev
> yes		yes			both

^ Yes that's pretty much what I was trying to say. I probably should've
just provided the table above. Looks like Darrick supplied a much
cleaner patch. Although I don't think it addresses the concerns you have
with regards to dynamically changing the S_DAX flag.

> 
> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@...el.com>
>> Reported-by: Darrick Wong <darrick.wong@...cle.com>
>> ---
>>  fs/xfs/xfs_iops.c  |    3 ++-
>>  fs/xfs/xfs_super.c |    9 ++++++++-
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index 56475fcd76f2..ab352c325301 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -1204,7 +1204,8 @@ xfs_diflags_to_iflags(
>>  	    ip->i_mount->m_sb.sb_blocksize == PAGE_SIZE &&
>>  	    !xfs_is_reflink_inode(ip) &&
>>  	    (ip->i_mount->m_flags & XFS_MOUNT_DAX ||
>> -	     ip->i_d.di_flags2 & XFS_DIFLAG2_DAX))
>> +	     ip->i_d.di_flags2 & XFS_DIFLAG2_DAX) &&
>> +	    blk_queue_dax(bdev_get_queue(inode->i_sb->s_bdev)))
> 
> This does not discriminate between the rtdev or the data dev. This
> needs to call xfs_find_bdev_for_inode() to get the right device
> for the inode config.
> 
> Further, if we add or remove the RT flag to the inode at a later
> point in time (e.g. via ioctl) we also need to re-evaluate the S_DAX
> flag at that point in time.
> 
> Which brings me to the real problem here: dynamically changing the
> S_DAX flag is racy, dangerous and broken. It's not clear that this
> should be allowed at all as the inode may have already been mmap()d
> by the time the ioctl is called to set/clear the rt file state.
> 
> IOWs, right now we cannot support mixed DAX mode filesystems because
> the generic DAX code does not support dynamic changing of the DAX
> flag on an inode and so checking the block device state here is
> irrelevant....
> 
>>  		inode->i_flags |= S_DAX;
>>  }
>>  
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index e8a687232614..5ac478924dce 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1649,11 +1649,18 @@ xfs_fs_fill_super(
>>  		sb->s_flags |= SB_I_VERSION;
>>  
>>  	if (mp->m_flags & XFS_MOUNT_DAX) {
>> +		bool rtdev_is_dax = false;
>> +
>>  		xfs_warn(mp,
>>  		"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
>>  
>> +		if (mp->m_rtdev_targp->bt_daxdev)
>> +			if (bdev_dax_supported(mp->m_rtdev_targp->bt_bdev,
>> +					      sb->s_blocksize) == 0)
>> +				rtdev_is_dax = true;
> 
> .... as this code here needs to turn off DAX here if any device
> in the filesystem doesn't support DAX....
> 
> 
> FWIW, the logic in the code is terrible (not your fault, Dave).
> The logic reads
> 
> 	if (NOT bdev_dax_supported(rtdev)) then
> 		rtdev supports DAX
> 
> That also needs fixing - we're checking something that has a boolean
> return state (yes or no) and so it should define them in a way that
> makes the caller logic read cleanly....
> 
> Cheers,
> 
> Dave.
>