linux-ext4 - Re: [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71eecee3-3fc9-d18d-8553-d8326f6d73b3@redhat.com>
Date:   Tue, 1 Dec 2020 14:50:03 -0600
From:   Eric Sandeen <sandeen@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Miklos Szeredi <mszeredi@...hat.com>,
        Ira Weiny <ira.weiny@...el.com>,
        David Howells <dhowells@...hat.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-man <linux-man@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        xfs <linux-xfs@...r.kernel.org>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Xiaoli Feng <xifeng@...hat.com>
Subject: Re: [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to
 filesystems

On 12/1/20 2:04 PM, Linus Torvalds wrote:
> On Tue, Dec 1, 2020 at 8:59 AM Eric Sandeen <sandeen@...hat.com> wrote:
>>
>> It's a bit odd to set STATX_ATTR_DAX into the statx attributes in the VFS;
>> while the VFS can detect the current DAX state, it is the filesystem which
>> actually sets S_DAX on the inode, and the filesystem is the place that
>> knows whether DAX is something that the "filesystem actually supports" [1]
>> so that the statx attributes_mask can be properly set.
>>
>> So, move STATX_ATTR_DAX attribute setting to the individual dax-capable
>> filesystems, and update the attributes_mask there as well.
> 
> I'm not really understanding the logic behind this.
> 
> The whole IS_DAX(inode) thing exists in various places outside the
> low-level filesystem, why shouldn't stat() do this?
> 
> If IS_DAX() is incorrect, then we have much bigger problems than some
> stat results. We have core functions like generic_file_read_iter() etc
> all making actual behavioral judgements on IS_DAX().

It's not incorrect, I didn't mean to imply that. Current code does accurately
set the DAX flag in the statx attributes.
 
> And if IS_DAX() is correct, then why shouldn't this just be done in
> generic code? Why move it to every individual filesystem?

At the end of the day, it's because only the individual filesystems can
manage the dax flag in the statx attributes_mask. (That's only place that
knows if dax "is available" in general, as opposed to being set on a specific
inode) So if they have to do that, they may as well set the actual attribute
as well, like they do for every other flag they manage...

I mean, we could leave the statx->attributes setting in the vfs, and add
the statx->attributes_mask setting to each dax-capable filesystem. That just
felt a bit asymmetric vs. the way every other filesystem-specific flag gets
handled.

-Eric