[PATCH V2] xfs_io: Fix fscrypt macros ordering

[PATCH V2] xfs_io: Fix fscrypt macros ordering

[cem]

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>
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- Remove dangling leftover comment
	- define FS_IOC_GET_ENCRYPTION_POLICY_EX on it's own block.

Bill, as the updates for the V2 are trivial, I'm keeping your RwB,
hopefuly you agree :)

 io/encrypt.c | 67 ++++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/io/encrypt.c b/io/encrypt.c
index 79061b07c..333ec03df 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.
@@ -102,13 +73,9 @@ struct fscrypt_policy_v1 {
 
 #define FSCRYPT_POLICY_V2		2
 #define FSCRYPT_KEY_IDENTIFIER_SIZE	16
-/* struct fscrypt_policy_v2 was defined earlier */
 
 #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 +119,40 @@ 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 */
+};
+
+#endif
+
+#ifndef FS_IOC_GET_ENCRYPTION_POLICY_EX
+#  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

Re: [PATCH V2] xfs_io: Fix fscrypt macros ordering

[Waldemar Brodkorb]

Hi Carlos,
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>
> Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> V2:
> 	- Remove dangling leftover comment
> 	- define FS_IOC_GET_ENCRYPTION_POLICY_EX on it's own block.

Thanks for the V2. I tested it on the failing toolchains and it
works now. You can add a 
Tested-by: Waldemar Brodkorb <wbx@openadk.org>

best regards
 Waldemar

Re: [PATCH V2] xfs_io: Fix fscrypt macros ordering

[Eric Biggers]

On Sat, Aug 17, 2024 at 11:32:48AM +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>
> Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> V2:
> 	- Remove dangling leftover comment
> 	- define FS_IOC_GET_ENCRYPTION_POLICY_EX on it's own block.
> 
> Bill, as the updates for the V2 are trivial, I'm keeping your RwB,
> hopefuly you agree :)
> 
>  io/encrypt.c | 67 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 33 deletions(-)

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric