linux-kernel - Re: [PATCH v5 02/12] dax: introduce IS_DEVDAX() and IS

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:   Fri, 2 Mar 2018 10:37:40 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     linux-nvdimm <linux-nvdimm@...ts.01.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        stable <stable@...r.kernel.org>, Jan Kara <jack@...e.cz>,
        Christoph Hellwig <hch@....de>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 02/12] dax: introduce IS_DEVDAX() and IS_FSDAX()

On Fri, Mar 2, 2018 at 9:45 AM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> On Thu, Mar 01, 2018 at 07:53:44PM -0800, Dan Williams wrote:
>> The current IS_DAX() helper that checks if a file is in DAX mode serves
>> two purposes. It is a control flow branch condition for DAX vs
>> non-DAX paths and it is a mechanism to perform dead code elimination. The
>> dead code elimination is required in the CONFIG_FS_DAX=n case since
>> there are symbols in fs/dax.c that will be elided. While the
>> dead code elimination can be addressed with nop stubs for the fs/dax.c
>> symbols that does not address the need for a DAX control flow helper
>> where fs/dax.c symbols are not involved.
>>
>> Moreover, the control flow changes, in some cases, need to be cognizant
>> of whether the DAX file is a typical file or a Device-DAX special file.
>> Introduce IS_DEVDAX() and IS_FSDAX() to simultaneously address the
>> file-type control flow and dead-code elimination use cases. IS_DAX()
>> will be deleted after all sites are converted to use the file-type
>> specific helper.
>>
>> Note, this change is also a pre-requisite for fixing the definition of
>> the S_DAX inode flag in the CONFIG_FS_DAX=n + CONFIG_DEV_DAX=y case.
>> The flag needs to be defined, non-zero, if either DAX facility is
>> enabled.
>>
>> Cc: "Theodore Ts'o" <tytso@....edu>
>> Cc: Andreas Dilger <adilger.kernel@...ger.ca>
>> Cc: Alexander Viro <viro@...iv.linux.org.uk>
>> Cc: "Darrick J. Wong" <darrick.wong@...cle.com>
>> Cc: linux-xfs@...r.kernel.org
>> Cc: Matthew Wilcox <mawilcox@...rosoft.com>
>> Cc: Ross Zwisler <ross.zwisler@...ux.intel.com>
>> Cc: <stable@...r.kernel.org>
>> Fixes: dee410792419 ("/dev/dax, core: file operations and dax-mmap")
>> Reported-by: Jan Kara <jack@...e.cz>
>> Reviewed-by: Jan Kara <jack@...e.cz>
>> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>> ---
>>  include/linux/fs.h |   22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 79c413985305..bd0c46880572 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1909,6 +1909,28 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
>>  #define IS_WHITEOUT(inode)   (S_ISCHR(inode->i_mode) && \
>>                                (inode)->i_rdev == WHITEOUT_DEV)
>>
>> +static inline bool IS_DEVDAX(struct inode *inode)
>> +{
>> +     if (!IS_ENABLED(CONFIG_DEV_DAX))
>> +             return false;
>> +     if ((inode->i_flags & S_DAX) == 0)
>> +             return false;
>> +     if (!S_ISCHR(inode->i_mode))
>> +             return false;
>> +     return true;
>> +}
>> +
>> +static inline bool IS_FSDAX(struct inode *inode)
>> +{
>> +     if (!IS_ENABLED(CONFIG_FS_DAX))
>> +             return false;
>
> I echo Jan's complaint from the last round that the dead code
> elimination here is subtle, as compared to:
>
> #if IS_ENABLED(CONFIG_FS_DAX)
> static inline bool IS_FSDAX(struct inode *inode) { ... }
> #else
> # define IS_FSDAX(inode) (false)
> #endif
>
> But I guess even with that we're relying on dead code elimination higher
> up in the call stack...

If IS_FSDAX() was only a dead-code elimination mechanism rather than a
runtime branch condition then I agree. Otherwise I think IS_ENABLED()
is suitable and not subtle, especially when used in a header file.

>> +     if ((inode->i_flags & S_DAX) == 0)
>> +             return false;
>> +     if (S_ISCHR(inode->i_mode))
>> +             return false;
>
> I'm curious, do we have character devices with S_DAX set?

Yes, Device-DAX, see:

    ab68f2622136 /dev/dax, pmem: direct access to persistent memory

> I /think/ we're expecting that only block/char devices and files will
> ever have S_DAX set, so IS_FSDAX is only true for block devices and
> files.  Right?

We had S_DAX on block-devices for a short while, but deleted it and
went with the Device-DAX interface instead. So it's only regular files
and /dev/daxX.Y nodes these days.

> (A comment here about why S_ISCHR->false here would be helpful.)

Ok.