linux-ext4 - Re: [PATCH 00/25] fscrypt: add some higher-level helper functions

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: <20170921174705.GA29643@gmail.com>
Date:   Thu, 21 Sep 2017 10:47:05 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, "Theodore Y . Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Michael Halcrow <mhalcrow@...gle.com>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH 00/25] fscrypt: add some higher-level helper functions

Hi Dave,

On Thu, Sep 21, 2017 at 04:45:02PM +1000, Dave Chinner wrote:
> fscrypto: clean up include file mess
> 
> From: Dave Chinner <dchinner@...hat.com>
> 
> Filesystems have to include different header files based on whether
> they are compiled with encryption support or not. That's nasty and
> messy.
> 
> Instead, rationalise the headers so we have a single include
> fscrypt.h and let it decide what internal implementation to include
> based on the __FS_HAS_ENCRYPTION define. Filesystems set
> __FS_HAS_ENCRYPTION before including linux/fscrypt.h if they are
> built with encryption support.
> 
> Add guards to prevent fscrypt_supp.h and fscrypt_notsupp.h from
> being directly included by filesystems.

This looks good; we probably should have done it that way originally.  This will
allow us to have the inline functions like fscrypt_prepare_rename() defined in
fscrypt.h, and then have supp/notsupp versions of __fscrypt_prepare_rename()
instead --- so common checks like for IS_ENCRYPTED() will be in one place only.

One nit:

> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +#define __FS_HAS_ENCRYPTION 1
> +#endif
> +#include <linux/fscrypt.h>

How about doing

	#define __FS_HAS_ENCRYPTION IS_ENABLED(CONFIG_EXT4_FS_ENCRYPTION)

(and likewise for f2fs and ubifs), then checking '#if __FS_HAS_ENCRYPTION'
rather than '#ifdef __FS_HAS_ENCRYPTION'?

Eric