linux-kernel - Re: [PATCH v3 3/6] xfs, dax: introduce 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] [day] [month] [year] [list]

Message-ID: <CAPcyv4jDNW=9dNPtmX2oiHhp9S7wr+w7DPONZsx-LnLhcLPesg@mail.gmail.com>
Date:   Mon, 26 Feb 2018 07:48:07 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-nvdimm@...ts.01.org,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        kbuild test robot <fengguang.wu@...el.com>
Subject: Re: [PATCH v3 3/6] xfs, dax: introduce IS_FSDAX()

On Mon, Feb 26, 2018 at 2:06 AM, Jan Kara <jack@...e.cz> wrote:
> On Fri 23-02-18 16:43:27, Dan Williams wrote:
>> Given that S_DAX is non-zero in the FS_DAX=n + DEV_DAX=y case, another
>> mechanism besides the plain IS_DAX() check to compile out dead
>> filesystem-dax code paths. Without IS_FSDAX() xfs will fail at link time
>> with:
>>
>>     ERROR: "dax_finish_sync_fault" [fs/xfs/xfs.ko] undefined!
>>     ERROR: "dax_iomap_fault" [fs/xfs/xfs.ko] undefined!
>>     ERROR: "dax_iomap_rw" [fs/xfs/xfs.ko] undefined!
>>
>> This compile failure was previously hidden by the fact that S_DAX was
>> erroneously defined to '0' in the FS_DAX=n + DEV_DAX=y case.
>>
>> Cc: "Darrick J. Wong" <darrick.wong@...cle.com>
>> Cc: linux-xfs@...r.kernel.org
>> Cc: <stable@...r.kernel.org>
>> Reported-by: kbuild test robot <fengguang.wu@...el.com>
>> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>
> As much as I appreciate that relying on compiler to optimize out dead
> branches results in nicer looking code this is an example where it
> backfires. Also having IS_DAX() and IS_FSDAX() doing almost the same, just
> not exactly the same, is IMHO a recipe for confusion (e.g. a casual reader
> could think why does ext4 get away with using IS_DAX while XFS has to use
> IS_FSDAX?). So I'd just prefer to handle this as is usual in other kernel
> areas - define empty stubs for all exported functions when CONFIG_FS_DAX is
> not enabled. That way code can stay without ugly ifdefs and we don't have
> to bother with IS_FSDAX vs IS_DAX distinction in filesystem code. Thoughts?
>

I think my patch is incomplete either way, because the current
IS_DAX() usages handle more than just compiling out calls to fs/dax.c
symbols. I.e. even if there were stubs for all fs/dax.c call outs call
there are still local usages of the helper. Lets kill IS_DAX() and
only have IS_FSDAX() and IS_DEVDAX() with the S_ISCHR() check. Any
issues with that?