[RFC PATCH v1 0/5] Ensure io_uring data structure consistentcy in liburing

[RFC PATCH v1 0/5] Ensure io_uring data structure consistentcy in liburing

[Ammar Faizi]

Hi,

This is an RFC for liburing-2.3.

## Introduction:
This series adds compile time assertions for liburing. They are taken
from the io_uring source in the kernel tree. The point of this series
is to make sure the shared struct is consistent between the kernel
space and user space.


## Implementation detail:
We use `static_assert()` macro from <assert.h> that can yield compile
error if the expression given to it evaluates to false. This way we
can create a `BUILD_BUG_ON()` macro that we usually use inside the
kernel. The assertions are placed inside a header file named
build_assert.h, this header is included via compile flag `-include`
when compiling the core liburing sources.


## How to maintain this?
This is pretty much easy to maintain, we just need to sync the
`BUILD_BUG_ON()` macro calls that check the shared struct from
io_uring. See patch #5 for detail.


## Patches summary:

  - Patch 1 is just a small code style cleanup.
  - Patch 2 is to add BUILD_BUG_ON() macro.
  - Patch 3 is to add sizeof_field() macro.
  - Patch 4 is to avoid macro redefinition warnings.
  - Patch 5 is the main part, it adds io_uring data structure
    assertions.


Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (5):
  lib: Don't indent in `#ifdef -> #define -> #endif` block
  lib: Add `BUILD_BUG_ON()` macro
  lib: Add `sizeof_field()` macro
  Avoid macro redefinition warnings
  Add io_uring data structure build assertion

 src/Makefile       |  3 ++-
 src/build_assert.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 src/lib.h          | 18 +++++++++++----
 src/queue.c        |  2 ++
 src/register.c     |  2 ++
 src/setup.c        |  2 ++
 src/syscall.c      |  2 ++
 7 files changed, 80 insertions(+), 6 deletions(-)
 create mode 100644 src/build_assert.h


base-commit: 4633a2d0fe9bd1f3dbb5b6d2788a08a264803146
-- 
Ammar Faizi

[RFC PATCH v1 1/5] lib: Don't indent in `#ifdef -> #define -> #endif` block

[Ammar Faizi]

Follow the surrounding code style.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/lib.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/lib.h b/src/lib.h
index 6672cc5..5dabd28 100644
--- a/src/lib.h
+++ b/src/lib.h
@@ -23,14 +23,14 @@
 
 
 #ifndef offsetof
-	#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
+#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
 #endif
 
 #ifndef container_of
-	#define container_of(PTR, TYPE, FIELD) ({			\
-		__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
-		(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
-	})
+#define container_of(PTR, TYPE, FIELD) ({			\
+	__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
+	(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
+})
 #endif
 
 void *__uring_malloc(size_t len);
-- 
Ammar Faizi

[RFC PATCH v1 2/5] lib: Add `BUILD_BUG_ON()` macro

[Ammar Faizi]

The first use case of this macro is for data structure assertion at
compile time. It's like what we do in the io_uring.c in the kernel.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/lib.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/lib.h b/src/lib.h
index 5dabd28..40e2817 100644
--- a/src/lib.h
+++ b/src/lib.h
@@ -5,6 +5,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <assert.h>
 
 #define __INTERNAL__LIBURING_LIB_H
 #if defined(__x86_64__) || defined(__i386__)
@@ -21,6 +22,9 @@
 #endif
 #undef __INTERNAL__LIBURING_LIB_H
 
+#ifndef BUILD_BUG_ON
+#define BUILD_BUG_ON(EXPR) static_assert(!(EXPR), "!(" #EXPR ") failed")
+#endif
 
 #ifndef offsetof
 #define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
-- 
Ammar Faizi

[RFC PATCH v1 3/5] lib: Add `sizeof_field()` macro

[Ammar Faizi]

The first use case of this macro is for data structure assertion at
compile time. It's like what we do in the io_uring.c in the kernel.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/lib.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/lib.h b/src/lib.h
index 40e2817..0db7499 100644
--- a/src/lib.h
+++ b/src/lib.h
@@ -26,6 +26,10 @@
 #define BUILD_BUG_ON(EXPR) static_assert(!(EXPR), "!(" #EXPR ") failed")
 #endif
 
+#ifndef sizeof_field
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#endif
+
 #ifndef offsetof
 #define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
 #endif
-- 
Ammar Faizi

[RFC PATCH v1 4/5] Avoid macro redefinition warnings

[Ammar Faizi]

This is a prep patch. The next patch will cause macro redefinition
warnings if we don't have this patch.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/queue.c    | 2 ++
 src/register.c | 2 ++
 src/setup.c    | 2 ++
 src/syscall.c  | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/src/queue.c b/src/queue.c
index ce0ecf6..800ae0c 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -1,5 +1,7 @@
 /* SPDX-License-Identifier: MIT */
+#ifndef _POSIX_C_SOURCE
 #define _POSIX_C_SOURCE 200112L
+#endif
 
 #include "lib.h"
 #include "syscall.h"
diff --git a/src/register.c b/src/register.c
index 993c450..f19a720 100644
--- a/src/register.c
+++ b/src/register.c
@@ -1,5 +1,7 @@
 /* SPDX-License-Identifier: MIT */
+#ifndef _POSIX_C_SOURCE
 #define _POSIX_C_SOURCE 200112L
+#endif
 
 #include "lib.h"
 #include "syscall.h"
diff --git a/src/setup.c b/src/setup.c
index d2adc7f..e28560d 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -1,5 +1,7 @@
 /* SPDX-License-Identifier: MIT */
+#ifndef _DEFAULT_SOURCE
 #define _DEFAULT_SOURCE
+#endif
 
 #include "lib.h"
 #include "syscall.h"
diff --git a/src/syscall.c b/src/syscall.c
index 362f1f5..b10f8b0 100644
--- a/src/syscall.c
+++ b/src/syscall.c
@@ -1,5 +1,7 @@
 /* SPDX-License-Identifier: MIT */
+#ifndef _DEFAULT_SOURCE
 #define _DEFAULT_SOURCE
+#endif
 
 /*
  * Functions in this file require libc, only build them when we use libc.
-- 
Ammar Faizi

[RFC PATCH v1 5/5] Add io_uring data structure build assertion

[Ammar Faizi]

Ensure io_uring data structure consistent between the kernel and user
space. These assertions are taken from io_uring.c in the kernel.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/Makefile       |  3 ++-
 src/build_assert.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 src/build_assert.h

diff --git a/src/Makefile b/src/Makefile
index 12cf49f..aed3c40 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -7,7 +7,8 @@ libdevdir ?= $(prefix)/lib
 
 CPPFLAGS ?=
 override CPPFLAGS += -D_GNU_SOURCE \
-	-Iinclude/ -include ../config-host.h
+	-Iinclude/ -include ../config-host.h \
+	-include build_assert.h
 CFLAGS ?= -g -O2 -Wall -Wextra -fno-stack-protector
 override CFLAGS += -Wno-unused-parameter -Wno-sign-compare -DLIBURING_INTERNAL
 SO_CFLAGS=-fPIC $(CFLAGS)
diff --git a/src/build_assert.h b/src/build_assert.h
new file mode 100644
index 0000000..5b2a9c6
--- /dev/null
+++ b/src/build_assert.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef LIBURING_BUILD_ASSERT_H
+#define LIBURING_BUILD_ASSERT_H
+
+#include "liburing/io_uring.h"
+#include "lib.h"
+
+static inline __attribute__((__unused__)) void io_uring_build_assert(void)
+{
+#define __BUILD_BUG_VERIFY_ELEMENT(stype, eoffset, etype, ename) do { \
+	BUILD_BUG_ON(offsetof(stype, ename) != eoffset); \
+	BUILD_BUG_ON(sizeof(etype) != sizeof_field(stype, ename)); \
+} while (0)
+
+#define BUILD_BUG_SQE_ELEM(eoffset, etype, ename) \
+	__BUILD_BUG_VERIFY_ELEMENT(struct io_uring_sqe, eoffset, etype, ename)
+	BUILD_BUG_ON(sizeof(struct io_uring_sqe) != 64);
+	BUILD_BUG_SQE_ELEM(0,  __u8,   opcode);
+	BUILD_BUG_SQE_ELEM(1,  __u8,   flags);
+	BUILD_BUG_SQE_ELEM(2,  __u16,  ioprio);
+	BUILD_BUG_SQE_ELEM(4,  __s32,  fd);
+	BUILD_BUG_SQE_ELEM(8,  __u64,  off);
+	BUILD_BUG_SQE_ELEM(8,  __u64,  addr2);
+	BUILD_BUG_SQE_ELEM(16, __u64,  addr);
+	BUILD_BUG_SQE_ELEM(16, __u64,  splice_off_in);
+	BUILD_BUG_SQE_ELEM(24, __u32,  len);
+	BUILD_BUG_SQE_ELEM(28,     __kernel_rwf_t, rw_flags);
+	BUILD_BUG_SQE_ELEM(28, /* compat */   int, rw_flags);
+	BUILD_BUG_SQE_ELEM(28, /* compat */ __u32, rw_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  fsync_flags);
+	BUILD_BUG_SQE_ELEM(28, /* compat */ __u16,  poll_events);
+	BUILD_BUG_SQE_ELEM(28, __u32,  poll32_events);
+	BUILD_BUG_SQE_ELEM(28, __u32,  sync_range_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  msg_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  timeout_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  accept_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  cancel_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  open_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  statx_flags);
+	BUILD_BUG_SQE_ELEM(28, __u32,  fadvise_advice);
+	BUILD_BUG_SQE_ELEM(28, __u32,  splice_flags);
+	BUILD_BUG_SQE_ELEM(32, __u64,  user_data);
+	BUILD_BUG_SQE_ELEM(40, __u16,  buf_index);
+	BUILD_BUG_SQE_ELEM(40, __u16,  buf_group);
+	BUILD_BUG_SQE_ELEM(42, __u16,  personality);
+	BUILD_BUG_SQE_ELEM(44, __s32,  splice_fd_in);
+	BUILD_BUG_SQE_ELEM(44, __u32,  file_index);
+	BUILD_BUG_SQE_ELEM(48, __u64,  addr3);
+
+	BUILD_BUG_ON(sizeof(struct io_uring_files_update) !=
+		     sizeof(struct io_uring_rsrc_update));
+	BUILD_BUG_ON(sizeof(struct io_uring_rsrc_update) >
+		     sizeof(struct io_uring_rsrc_update2));
+}
+
+#endif
-- 
Ammar Faizi

Re: [RFC PATCH v1 5/5] Add io_uring data structure build assertion

[Pavel Begunkov]

On 6/6/22 07:12, Ammar Faizi wrote:
> Ensure io_uring data structure consistent between the kernel and user
> space. These assertions are taken from io_uring.c in the kernel.

I don't see why would we do that. io_uring.h is only intended to be
copied from the kernel's uapi without some weird changes in structs,
I really really hope nobody will be trying to modify it separately.

But the real downside is that we'll need to maintain a full
duplicate of it and keep updating both for no good reason.
What do I miss?

> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>   src/Makefile       |  3 ++-
>   src/build_assert.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 59 insertions(+), 1 deletion(-)
>   create mode 100644 src/build_assert.h
> 
> diff --git a/src/Makefile b/src/Makefile
> index 12cf49f..aed3c40 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -7,7 +7,8 @@ libdevdir ?= $(prefix)/lib
>   
>   CPPFLAGS ?=
>   override CPPFLAGS += -D_GNU_SOURCE \
> -	-Iinclude/ -include ../config-host.h
> +	-Iinclude/ -include ../config-host.h \
> +	-include build_assert.h
>   CFLAGS ?= -g -O2 -Wall -Wextra -fno-stack-protector
>   override CFLAGS += -Wno-unused-parameter -Wno-sign-compare -DLIBURING_INTERNAL
>   SO_CFLAGS=-fPIC $(CFLAGS)
> diff --git a/src/build_assert.h b/src/build_assert.h
> new file mode 100644
> index 0000000..5b2a9c6
> --- /dev/null
> +++ b/src/build_assert.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef LIBURING_BUILD_ASSERT_H
> +#define LIBURING_BUILD_ASSERT_H
> +
> +#include "liburing/io_uring.h"
> +#include "lib.h"
> +
> +static inline __attribute__((__unused__)) void io_uring_build_assert(void)
> +{
> +#define __BUILD_BUG_VERIFY_ELEMENT(stype, eoffset, etype, ename) do { \
> +	BUILD_BUG_ON(offsetof(stype, ename) != eoffset); \
> +	BUILD_BUG_ON(sizeof(etype) != sizeof_field(stype, ename)); \
> +} while (0)
> +
> +#define BUILD_BUG_SQE_ELEM(eoffset, etype, ename) \
> +	__BUILD_BUG_VERIFY_ELEMENT(struct io_uring_sqe, eoffset, etype, ename)
> +	BUILD_BUG_ON(sizeof(struct io_uring_sqe) != 64);
> +	BUILD_BUG_SQE_ELEM(0,  __u8,   opcode);
> +	BUILD_BUG_SQE_ELEM(1,  __u8,   flags);
> +	BUILD_BUG_SQE_ELEM(2,  __u16,  ioprio);
> +	BUILD_BUG_SQE_ELEM(4,  __s32,  fd);
> +	BUILD_BUG_SQE_ELEM(8,  __u64,  off);
> +	BUILD_BUG_SQE_ELEM(8,  __u64,  addr2);
> +	BUILD_BUG_SQE_ELEM(16, __u64,  addr);
> +	BUILD_BUG_SQE_ELEM(16, __u64,  splice_off_in);
> +	BUILD_BUG_SQE_ELEM(24, __u32,  len);
> +	BUILD_BUG_SQE_ELEM(28,     __kernel_rwf_t, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */   int, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */ __u32, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  fsync_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */ __u16,  poll_events);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  poll32_events);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  sync_range_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  msg_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  timeout_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  accept_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  cancel_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  open_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  statx_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  fadvise_advice);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  splice_flags);
> +	BUILD_BUG_SQE_ELEM(32, __u64,  user_data);
> +	BUILD_BUG_SQE_ELEM(40, __u16,  buf_index);
> +	BUILD_BUG_SQE_ELEM(40, __u16,  buf_group);
> +	BUILD_BUG_SQE_ELEM(42, __u16,  personality);
> +	BUILD_BUG_SQE_ELEM(44, __s32,  splice_fd_in);
> +	BUILD_BUG_SQE_ELEM(44, __u32,  file_index);
> +	BUILD_BUG_SQE_ELEM(48, __u64,  addr3);
> +
> +	BUILD_BUG_ON(sizeof(struct io_uring_files_update) !=
> +		     sizeof(struct io_uring_rsrc_update));
> +	BUILD_BUG_ON(sizeof(struct io_uring_rsrc_update) >
> +		     sizeof(struct io_uring_rsrc_update2));
> +}
> +
> +#endif

-- 
Pavel Begunkov

Re: [RFC PATCH v1 0/5] Ensure io_uring data structure consistentcy in liburing

[Hao Xu]

On 6/6/22 14:12, Ammar Faizi wrote:
> 
> Hi,
> 
> This is an RFC for liburing-2.3.
> 
> ## Introduction:
> This series adds compile time assertions for liburing. They are taken
> from the io_uring source in the kernel tree. The point of this series
> is to make sure the shared struct is consistent between the kernel
> space and user space.
> 
> 
> ## Implementation detail:
> We use `static_assert()` macro from <assert.h> that can yield compile
> error if the expression given to it evaluates to false. This way we
> can create a `BUILD_BUG_ON()` macro that we usually use inside the
> kernel. The assertions are placed inside a header file named
> build_assert.h, this header is included via compile flag `-include`
> when compiling the core liburing sources.
> 
> 
> ## How to maintain this?
> This is pretty much easy to maintain, we just need to sync the
> `BUILD_BUG_ON()` macro calls that check the shared struct from
> io_uring. See patch #5 for detail.
> 
> 

Looks good to me,
Acked-by: Hao Xu <howeyxu@tencent.com>