linux-ext4 - Re: [PATCH v2] mmp: do not use O_DIRECT when working with regular file

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: <E16FB371-5DFC-4A10-A9E2-36541FCF7D30@gmail.com>
Date:   Fri, 19 Feb 2021 14:49:16 +0300
From:   Alexey Lyashkov <alexey.lyashkov@...il.com>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     Andreas Dilger <adilger@...ger.ca>,
        Artem Blagodarenko <artem.blagodarenko@...il.com>,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        Eric Sandeen <sandeen@...hat.com>
Subject: Re: [PATCH v2] mmp: do not use O_DIRECT when working with regular
 file

Lukas,

because e2fsprogs have an bad assumption about IO size for the O_DIRECT case.
and because library uses a code like
>>
set_block_size(1k);
seek(fs, 1);
read_block();
>>>
which caused an 1k read inside of 4k disk block size not aligned by block size, which is prohibited and caused an error report.

Reference to patch.
https://patchwork.ozlabs.org/project/linux-ext4/patch/20201023112659.1559-1-artem.blagodarenko@gmail.com/

Alex

> 19 февр. 2021 г., в 13:57, Lukas Czerner <lczerner@...hat.com> написал(а):
> 
> On Fri, Feb 19, 2021 at 01:08:17PM +0300, Alexey Lyashkov wrote:
>> Andreas,
>> 
>> What about to disable a O_DIRECT global on any block devices in the e2fsprogs library as this don’t work on 4k disk drives at all ?
>> Instead of fixing an O_DIRECT access with patches sends early.
> 
> Why would it not work at all ? This is a fix for a specific problem and
> I am not currently aware of ony other problems e2fsprogs should have
> with 4k sector size drives. Do you have a specific problem in mind ?
> 
> Thanks!
> -Lukas
> 
>> 
>> 
>> Alex
>> 
>>> 19 февр. 2021 г., в 1:20, Andreas Dilger <adilger@...ger.ca> написал(а):
>>> 
>>> On Feb 18, 2021, at 2:51 AM, Lukas Czerner <lczerner@...hat.com> wrote:
>>>> 
>>>> Currently the mmp block is read using O_DIRECT to avoid any caching that
>>>> may be done by the VM. However when working with regular files this
>>>> creates alignment issues when the device of the host file system has
>>>> sector size larger than the blocksize of the file system in the file
>>>> we're working with.
>>>> 
>>>> This can be reproduced with t_mmp_fail test when run on the device with
>>>> 4k sector size because the mke2fs fails when trying to read the mmp
>>>> block.
>>>> 
>>>> Fix it by disabling O_DIRECT when working with regular files. I don't
>>>> think there is any risk of doing so since the file system layer, unlike
>>>> shared block device, should guarantee cache consistency.
>>>> 
>>>> Signed-off-by: Lukas Czerner <lczerner@...hat.com>
>>>> Reviewed-by: Eric Sandeen <sandeen@...hat.com>
>>> 
>>> Reviewed-by: Andreas Dilger <adilger@...ger.ca>
>>> 
>>>> ---
>>>> v2: Fix comment - it avoids problems when the sector size is larger not
>>>>  smaller than blocksize
>>>> 
>>>> lib/ext2fs/mmp.c | 22 +++++++++++-----------
>>>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>>> 
>>>> diff --git a/lib/ext2fs/mmp.c b/lib/ext2fs/mmp.c
>>>> index c21ae272..cca2873b 100644
>>>> --- a/lib/ext2fs/mmp.c
>>>> +++ b/lib/ext2fs/mmp.c
>>>> @@ -57,21 +57,21 @@ errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf)
>>>> 	 * regardless of how the io_manager is doing reads, to avoid caching of
>>>> 	 * the MMP block by the io_manager or the VM.  It needs to be fresh. */
>>>> 	if (fs->mmp_fd <= 0) {
>>>> +		struct stat st;
>>>> 		int flags = O_RDWR | O_DIRECT;
>>>> 
>>>> -retry:
>>>> +		/*
>>>> +		 * There is no reason for using O_DIRECT if we're working with
>>>> +		 * regular file. Disabling it also avoids problems with
>>>> +		 * alignment when the device of the host file system has sector
>>>> +		 * size larger than blocksize of the fs we're working with.
>>>> +		 */
>>>> +		if (stat(fs->device_name, &st) == 0 &&
>>>> +		    S_ISREG(st.st_mode))
>>>> +			flags &= ~O_DIRECT;
>>>> +
>>>> 		fs->mmp_fd = open(fs->device_name, flags);
>>>> 		if (fs->mmp_fd < 0) {
>>>> -			struct stat st;
>>>> -
>>>> -			/* Avoid O_DIRECT for filesystem image files if open
>>>> -			 * fails, since it breaks when running on tmpfs. */
>>>> -			if (errno == EINVAL && (flags & O_DIRECT) &&
>>>> -			    stat(fs->device_name, &st) == 0 &&
>>>> -			    S_ISREG(st.st_mode)) {
>>>> -				flags &= ~O_DIRECT;
>>>> -				goto retry;
>>>> -			}
>>>> 			retval = EXT2_ET_MMP_OPEN_DIRECT;
>>>> 			goto out;
>>>> 		}
>>>> --
>>>> 2.26.2
>>>> 
>>> 
>>> 
>>> Cheers, Andreas
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>