Re: [PATCH] xfs_io: Fix fscrypt macros ordering

[Bill O'Donnell <bodonnel@redhat.com>]

On Fri, Aug 16, 2024 at 09:39:38PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> We've been reported a failure to build xfsprogs within buildroot's
> environment when they tried to upgrade xfsprogs from 6.4 to 6.9:
> 
> encrypt.c:53:36: error: 'FSCRYPT_KEY_IDENTIFIER_SIZE' undeclared
> here (not in a function)
>         53 |         __u8
> master_key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE];
>            |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>      encrypt.c:61:42: error: field 'v1' has incomplete type
>         61 |                 struct fscrypt_policy_v1 v1;
>            |                                          ^~
> 
> They were using a kernel version without FS_IOC_GET_ENCRYPTION_POLICY_EX
> set and OVERRIDE_SYSTEM_FSCRYPT_POLICY_V2 was unset.
> This combination caused xfsprogs to attempt to define fscrypt_policy_v2
> locally, which uses:
> 	__u8 master_key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE];
> 
> The problem is FSCRYPT_KEY_IDENTIFIER_SIZE is only after this block of
> code, so we need to define it earlier.
> 
> This also attempts to use fscrypt_policy_v1, which is defined only
> later.
> 
> To fix this, just reorder both ifdef blocks, but we need to move the
> definition of FS_IOC_GET_ENCRYPTION_POLICY_EX to the later, otherwise,
> the later definitions won't be enabled causing havoc.
> 
> Fixes: e97caf714697a ("xfs_io/encrypt: support specifying crypto data unit size")
> Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> ---
>  io/encrypt.c | 64 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/io/encrypt.c b/io/encrypt.c
> index 79061b07c..97abb964e 100644
> --- a/io/encrypt.c
> +++ b/io/encrypt.c
> @@ -35,35 +35,6 @@
>  #define FS_IOC_GET_ENCRYPTION_POLICY		_IOW('f', 21, struct fscrypt_policy)
>  #endif
>  
> -/*
> - * Since the log2_data_unit_size field was added later than fscrypt_policy_v2
> - * itself, we may need to override the system definition to get that field.
> - * And also fscrypt_get_policy_ex_arg since it contains fscrypt_policy_v2.
> - */
> -#if !defined(FS_IOC_GET_ENCRYPTION_POLICY_EX) || \
> -	defined(OVERRIDE_SYSTEM_FSCRYPT_POLICY_V2)
> -#undef fscrypt_policy_v2
> -struct fscrypt_policy_v2 {
> -	__u8 version;
> -	__u8 contents_encryption_mode;
> -	__u8 filenames_encryption_mode;
> -	__u8 flags;
> -	__u8 log2_data_unit_size;
> -	__u8 __reserved[3];
> -	__u8 master_key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE];
> -};
> -
> -#undef fscrypt_get_policy_ex_arg
> -struct fscrypt_get_policy_ex_arg {
> -	__u64 policy_size; /* input/output */
> -	union {
> -		__u8 version;
> -		struct fscrypt_policy_v1 v1;
> -		struct fscrypt_policy_v2 v2;
> -	} policy; /* output */
> -};
> -#endif
> -
>  /*
>   * Second batch of ioctls (Linux headers v5.4+), plus some renamings from FS_ to
>   * FSCRYPT_.  We don't bother defining the old names here.
> @@ -106,9 +77,6 @@ struct fscrypt_policy_v1 {
>  
>  #define FSCRYPT_MAX_KEY_SIZE		64
>  
> -#define FS_IOC_GET_ENCRYPTION_POLICY_EX		_IOWR('f', 22, __u8[9]) /* size + version */
> -/* struct fscrypt_get_policy_ex_arg was defined earlier */
> -
>  #define FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR	1
>  #define FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER	2
>  struct fscrypt_key_specifier {
> @@ -152,6 +120,38 @@ struct fscrypt_get_key_status_arg {
>  
>  #endif /* !FS_IOC_GET_ENCRYPTION_POLICY_EX */
>  
> +/*
> + * Since the log2_data_unit_size field was added later than fscrypt_policy_v2
> + * itself, we may need to override the system definition to get that field.
> + * And also fscrypt_get_policy_ex_arg since it contains fscrypt_policy_v2.
> + */
> +#if !defined(FS_IOC_GET_ENCRYPTION_POLICY_EX) || \
> +	defined(OVERRIDE_SYSTEM_FSCRYPT_POLICY_V2)
> +#undef fscrypt_policy_v2
> +struct fscrypt_policy_v2 {
> +	__u8 version;
> +	__u8 contents_encryption_mode;
> +	__u8 filenames_encryption_mode;
> +	__u8 flags;
> +	__u8 log2_data_unit_size;
> +	__u8 __reserved[3];
> +	__u8 master_key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE];
> +};
> +
> +#undef fscrypt_get_policy_ex_arg
> +struct fscrypt_get_policy_ex_arg {
> +	__u64 policy_size; /* input/output */
> +	union {
> +		__u8 version;
> +		struct fscrypt_policy_v1 v1;
> +		struct fscrypt_policy_v2 v2;
> +	} policy; /* output */
> +};
> +
> +#define FS_IOC_GET_ENCRYPTION_POLICY_EX		_IOWR('f', 22, __u8[9]) /* size + version */
> +
> +#endif
> +
>  /*
>   * Since the key_id field was added later than struct fscrypt_add_key_arg
>   * itself, we may need to override the system definition to get that field.
> -- 
> 2.46.0
> 
>