Skip to contentSkip to conversation

uapi: net/ipv4: Use __DECLARE_FLEX_ARRAY() helper

PATCH Superseded Gustavo A. R. Silva 2 participants 3 messages Aug 30, 2022 – Aug 31, 2022

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 uapi: net/ipv4: Use __DECLARE_FLEX_ARRAY() helper

We now have a cleaner way to keep compatibility with user-space (a.k.a. not breaking it) when we need to keep in place a one-element array (for its use in user-space) together with a flexible-array member (for its use in kernel-space) without making it hard to read at the source level. This is through the use of the new __DECLARE_FLEX_ARRAY() helper macro.

The size and memory layout of the structure is preserved after the changes. See below.

Before changes:

$ pahole -C ip_msfilter net/ipv4/igmp.o
struct ip_msfilter {
	union {
		struct {
			__be32     imsf_multiaddr_aux;   /*     0     4 */
			__be32     imsf_interface_aux;   /*     4     4 */
			__u32      imsf_fmode_aux;       /*     8     4 */
			__u32      imsf_numsrc_aux;      /*    12     4 */
			__be32     imsf_slist[1];        /*    16     4 */
		};                                       /*     0    20 */
		struct {
			__be32     imsf_multiaddr;       /*     0     4 */
			__be32     imsf_interface;       /*     4     4 */
			__u32      imsf_fmode;           /*     8     4 */
			__u32      imsf_numsrc;          /*    12     4 */
			__be32     imsf_slist_flex[0];   /*    16     0 */
		};                                       /*     0    16 */
	};                                               /*     0    20 */
	/* size: 20, cachelines: 1, members: 1 */
	/* last cacheline: 20 bytes */
};

After changes:

$ pahole -C ip_msfilter net/ipv4/igmp.o
struct ip_msfilter {
	struct {
		__be32             imsf_multiaddr;       /*     0     4 */
		__be32             imsf_interface;       /*     4     4 */
		__u32              imsf_fmode;           /*     8     4 */
		__u32              imsf_numsrc;          /*    12     4 */
		union {
			__be32     imsf_slist[1];        /*    16     4 */
			struct {
				struct {
				} __empty_imsf_slist_flex; /*    16     0 */
				__be32 imsf_slist_flex[0]; /*    16     0 */
			};                               /*    16     0 */
		};                                       /*    16     4 */
	};                                               /*     0    20 */
	/* size: 20, cachelines: 1, members: 1 */
	/* last cacheline: 20 bytes */
};

In the past, we had to duplicate the whole original structure within a union, and update the names of all the members. Now, we just need to declare the flexible-array member to be used in kernel-space through the __DECLARE_FLEX_ARRAY() helper together with the one-element array, within a union. This makes the source code more clean and easier to read.

---
 include/uapi/linux/in.h | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
include/uapi/linux/in.h +7 -13
index 14168225cecd..fa4dc8f8f081 100644
@@ -188,20 +188,14 @@ struct ip_mreq_source {
 };
 
 struct ip_msfilter {
-	union {
-		struct {
-			__be32		imsf_multiaddr_aux;
-			__be32		imsf_interface_aux;
-			__u32		imsf_fmode_aux;
-			__u32		imsf_numsrc_aux;
+	struct {
+		__be32		imsf_multiaddr;
+		__be32		imsf_interface;
+		__u32		imsf_fmode;
+		__u32		imsf_numsrc;
+		union {
 			__be32		imsf_slist[1];
-		};
-		struct {
-			__be32		imsf_multiaddr;
-			__be32		imsf_interface;
-			__u32		imsf_fmode;
-			__u32		imsf_numsrc;
-			__be32		imsf_slist_flex[];
+			__DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex);
 		};
 	};
 };
Signature · 1 line
2.34.1
#1 lore raw
Quoted 75 lines · Gustavo A. R. Silva

We now have a cleaner way to keep compatibility with user-space (a.k.a. not breaking it) when we need to keep in place a one-element array (for its use in user-space) together with a flexible-array member (for its use in kernel-space) without making it hard to read at the source level. This is through the use of the new __DECLARE_FLEX_ARRAY() helper macro.

The size and memory layout of the structure is preserved after the changes. See below.

Before changes:

$ pahole -C ip_msfilter net/ipv4/igmp.o
struct ip_msfilter {
	union {
		struct {
			__be32     imsf_multiaddr_aux;   /*     0     4 */
			__be32     imsf_interface_aux;   /*     4     4 */
			__u32      imsf_fmode_aux;       /*     8     4 */
			__u32      imsf_numsrc_aux;      /*    12     4 */
			__be32     imsf_slist[1];        /*    16     4 */
		};                                       /*     0    20 */
		struct {
			__be32     imsf_multiaddr;       /*     0     4 */
			__be32     imsf_interface;       /*     4     4 */
			__u32      imsf_fmode;           /*     8     4 */
			__u32      imsf_numsrc;          /*    12     4 */
			__be32     imsf_slist_flex[0];   /*    16     0 */
		};                                       /*     0    16 */
	};                                               /*     0    20 */
	/* size: 20, cachelines: 1, members: 1 */
	/* last cacheline: 20 bytes */
};

After changes:

$ pahole -C ip_msfilter net/ipv4/igmp.o
struct ip_msfilter {
	struct {
		__be32             imsf_multiaddr;       /*     0     4 */
		__be32             imsf_interface;       /*     4     4 */
		__u32              imsf_fmode;           /*     8     4 */
		__u32              imsf_numsrc;          /*    12     4 */
		union {
			__be32     imsf_slist[1];        /*    16     4 */
			struct {
				struct {
				} __empty_imsf_slist_flex; /*    16     0 */
				__be32 imsf_slist_flex[0]; /*    16     0 */
			};                               /*    16     0 */
		};                                       /*    16     4 */
	};                                               /*     0    20 */
	/* size: 20, cachelines: 1, members: 1 */
	/* last cacheline: 20 bytes */
};

In the past, we had to duplicate the whole original structure within a union, and update the names of all the members. Now, we just need to declare the flexible-array member to be used in kernel-space through the __DECLARE_FLEX_ARRAY() helper together with the one-element array, within a union. This makes the source code more clean and easier to read.

---
 include/uapi/linux/in.h | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
include/uapi/linux/in.h +0 -0
index 14168225cecd..fa4dc8f8f081 100644

include/uapi/linux/in.h struct ip_mreq_source { · line 191

Quoted 10 lines
include/uapi/linux/in.h +1 -6
@@ -188,20 +188,14 @@ struct ip_mreq_source {
 };
 
 struct ip_msfilter {
-	union {
-		struct {
-			__be32		imsf_multiaddr_aux;
-			__be32		imsf_interface_aux;
-			__u32		imsf_fmode_aux;
-			__u32		imsf_numsrc_aux;
+	struct {

I don't think this internal anonymous struct is needed any more?

Quoted 7 lines
include/uapi/linux/in.h +5 -1
+		__be32		imsf_multiaddr;
+		__be32		imsf_interface;
+		__u32		imsf_fmode;
+		__u32		imsf_numsrc;
+		union {
 			__be32		imsf_slist[1];
-		};

include/uapi/linux/in.h struct ip_mreq_source { · line 201

Quoted 10 lines
include/uapi/linux/in.h +1 -6
-		struct {
-			__be32		imsf_multiaddr;
-			__be32		imsf_interface;
-			__u32		imsf_fmode;
-			__u32		imsf_numsrc;
-			__be32		imsf_slist_flex[];
+			__DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex);
 		};
 	};
 };

I.e. can't this now just be:

struct ip_msfilter {
	__be32          imsf_multiaddr;
	__be32          imsf_interface;
	__u32           imsf_fmode;
	__u32           imsf_numsrc;
	union {
		__be32          imsf_slist[1];
		__DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex);
	};
};
Quoted 3 lines
Signature · 1 line
2.34.1
Signature · 1 line
Kees Cook
#2 lore raw
Quoted 10 lines · Kees Cook

include/uapi/linux/in.h struct ip_mreq_source { · line 191

Quoted 8 lines

struct ip_msfilter {

include/uapi/linux/in.h +1 -6
-	union {
-		struct {
-			__be32		imsf_multiaddr_aux;
-			__be32		imsf_interface_aux;
-			__u32		imsf_fmode_aux;
-			__u32		imsf_numsrc_aux;
+	struct {

I don't think this internal anonymous struct is needed any more?

yes, aaargh... copy/paste error D:

I'll respin right away.

Thanks!
--
Gustavo