linux-ext4 - Re: [PATCH] fs: i_version mntopt gets visible through /proc/mounts

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: <4cbb5cbe-feb4-2166-0634-29041a41a8dc@sandeen.net>
Date:   Wed, 17 Jun 2020 13:28:11 -0500
From:   Eric Sandeen <sandeen@...deen.net>
To:     "J. Bruce Fields" <bfields@...ldses.org>
Cc:     "Darrick J. Wong" <darrick.wong@...cle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Masayoshi Mizuma <msys.mizuma@...il.com>,
        Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Masayoshi Mizuma <m.mizuma@...fujitsu.com>,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-xfs <linux-xfs@...r.kernel.org>
Subject: Re: [PATCH] fs: i_version mntopt gets visible through /proc/mounts



On 6/17/20 1:18 PM, J. Bruce Fields wrote:
> On Wed, Jun 17, 2020 at 12:55:24PM -0500, Eric Sandeen wrote:
>> On 6/17/20 12:24 PM, Darrick J. Wong wrote:
>>> On Wed, Jun 17, 2020 at 12:14:28PM -0500, Eric Sandeen wrote:
>>>>
>>>>
>>>> On 6/17/20 10:58 AM, J. Bruce Fields wrote:
>>>>> On Wed, Jun 17, 2020 at 01:03:14AM -0700, Christoph Hellwig wrote:
>>>>>> On Tue, Jun 16, 2020 at 04:21:23PM -0400, Masayoshi Mizuma wrote:
>>>>>>> From: Masayoshi Mizuma <m.mizuma@...fujitsu.com>
>>>>>>>
>>>>>>> /proc/mounts doesn't show 'i_version' even if iversion
>>>>>>> mount option is set to XFS.
>>>>>>>
>>>>>>> iversion mount option is a VFS option, not ext4 specific option.
>>>>>>> Move the handler to show_sb_opts() so that /proc/mounts can show
>>>>>>> 'i_version' on not only ext4 but also the other filesystem.
>>>>>>
>>>>>> SB_I_VERSION is a kernel internal flag.  XFS doesn't have an i_version
>>>>>> mount option.
>>>>>
>>>>> It probably *should* be a kernel internal flag, but it seems to work as
>>>>> a mount option too.
>>>>
>>>> Not on XFS AFAICT:
>>>>
>>>> [600280.685810] xfs: Unknown parameter 'i_version'
>>>
>>> Yeah, because the mount option is 'iversion', not 'i_version'.  Even if
>>
>> unless you're ext4:
>>
>> {Opt_i_version, "i_version"},
>>
>> ok "iversion" is what mount(8) takes and translates into MS_I_VERSION (thanks Darrick)
>>
>> # strace -vv -emount mount -oloop,iversion fsfile mnt
>> mount("/dev/loop0", "/tmp/mnt", "xfs", MS_I_VERSION, NULL) = 0
>>
>> FWIW, mount actually seems to pass what it finds in /proc/mounts back in on remount for ext4:
>>
>> # strace -vv -emount mount -o remount mnt
>> mount("/dev/loop0", "/tmp/mnt", 0x55bfcbdca150, MS_REMOUNT|MS_RELATIME, "seclabel,i_version,data=ordered") = 0
>>
>> but it still looks unhandled on remount.  Perhaps if /proc/mounts exposed
>> "iversion" (not "i_version") then mount -o remount would DTRT.
> 
> I'd rather just eliminate the option, to the extent possible.
> 
> It was only ever a mount option since it caused a performance regression
> in some filesystems, but I *think* that was addressed by Jeff Layton's
> work (f02a9ad1f15d "fs: handle inode->i_version more efficiently").
> 
> XFS in particular is just using this flag to tell knfsd that it should
> use i_version.  I don't think it was really intended for userspace to be
> able to turn this off.

but mount(8) has already exposed this interface:

       iversion
              Every time the inode is modified, the i_version field will be incremented.

       noiversion
              Do not increment the i_version inode field.

so now what?

FWIW, exporting "iversion" in /proc/mounts will 

1) tell us whether the SB_I_VERSION is or is not in fact set on the fs, and
2) will make remount DTRT because mount(8) will properly parse it and send it
   back in via the "flags" var during remount.

# mount -o loop fsfile mnt
# grep mnt /proc/mounts 
/dev/loop0 /tmp/mnt xfs rw,iversion,seclabel,relatime,attr2,inode64,logbufs=8,logbsize=32k,noquota 0 0

# strace -vv -emount mount -o remount mnt
mount("/dev/loop0", "/tmp/mnt", 0x55c11d0e3150, MS_REMOUNT|MS_RELATIME|MS_I_VERSION, "seclabel,attr2,inode64,logbufs=8"...) = 0

# grep mnt /proc/mounts 
/dev/loop0 /tmp/mnt xfs rw,iversion,seclabel,relatime,attr2,inode64,logbufs=8,logbsize=32k,noquota 0 0

ext4 i_version will just map back to iversion:

# mount -o i_version,loop fsfile mnt
# grep mnt /proc/mounts
/dev/loop0 /tmp/mnt ext4 rw,iversion,seclabel,relatime 0 0
# mount -o remount mnt
# grep mnt /proc/mounts
/dev/loop0 /tmp/mnt ext4 rw,iversion,seclabel,relatime 0 0

One wrinkle is that xfs will not honor "noiversion" currently but that's a different question.

-Eric