diff mbox series

[RFC,next] net: inet_sock.h: Avoid thousands of -Wflex-array-member-not-at-end warnings

Message ID ZzK-n_C2yl8mW2Tz@kspp (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,next] net: inet_sock.h: Avoid thousands of -Wflex-array-member-not-at-end warnings | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 14 this patch: 14
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 27 this patch: 27
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2058 this patch: 2058
netdev/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: Please use a blank line after function/struct/union/enum declarations WARNING: line length of 83 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0

Commit Message

Gustavo A. R. Silva Nov. 12, 2024, 2:34 a.m. UTC 
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally

So, in order to avoid ending up with a flexible-array member in the
middle of another struct, we use the `struct_group_tagged()` helper
to create a new tagged `struct ip_options_hdr`. This structure groups
together all the members of the flexible `struct ip_options` except
the flexible array. As a result, the array is effectively separated
from the rest of the members without modifying the memory layout of
the flexible structure.

However, the pattern that triggers the warnings addressed by this patch
differs from the usual case. Typically, the flexible structure is
immediately followed by another object in the composite structure. Here,
however, we have a well-defined composite structure with a flexible
structure right at its end, which is a correct code construct and has
no issues on its own. An instance of this composite structure
(`struct ip_options_rcu`) is then used within another structure
(`struct ip_options_data`) right before a fixed-size array (`char
data[40]`), as below:

/* Modified flexible structure with struct_group_tagged() */
struct ip_options {
	struct_group_tagged(ip_options_hdr, __hdr,
		...
	);
	unsigned char __data[];
};

/* Correct composite struct with flex struct at the end. */
struct ip_options_rcu {
	struct rcu_head rcu;
	struct ip_options opt; /* flexible structure --this is fine. */
};

struct ip_options_data {
        struct ip_options_rcu   opt; /* Conflicting object causing tons
				      * of -Wfamnae warnings --it ends in
				      * flexible-array member `__data`. */
        char                    data[40];
};

In this scenario, we have two levels of indirection before reaching the
flexible array: `p->opt->opt->__data`. As a result, we can't simply
change the type of the conflicting object `opt` to `struct ip_options_hdr`
as we might in a simpler case, because `opt` in `struct ip_options_data`
isn't a flexible structure itself but a composite structure
`struct ip_options_rcu` containing one (flexible structure).

So, we have a couple of alternatives to fix this issue:

a) The complex solution: Create an additional "header type" with the
   help of `struct_group_tagged()`, and introduce another
   flexible-array member, as follows:

   1) Create a new type `struct ip_options_rcu_hdr` using
      `struct_group_tagged()` within `struct ip_options_rcu`.

   2) Change the type of `opt` from `struct ip_options` to `struct
      ip_options_hdr`, removing the flexible array member from the newly
      created `struct ip_options_rcu_hdr`.

   3) Add a flexible-array member directly inside `struct ip_options_rcu`,
      so it still has a flexible-array member at the end --which was
      previously "removed" with the change from `struct ip_options opt;`
      to `struct ip_options_hdr opt;`:

|	 struct ip_options_rcu {
|	-       struct rcu_head rcu;
|	-       struct ip_options opt;
|	+       struct_group_tagged(ip_options_rcu_hdr, hdr,
|	+               struct rcu_head rcu;
|	+               struct ip_options_hdr opt;
|	+       );
|	+       unsigned char   __data[];
|	 };

    4) Now we can change the type of the conflicting object `opt` in
       `struct ip_options_data` from `struct ip_options_rcu` to `struct
       ip_options_rcu_hdr`, and with this silence the -Wfamnae warnings:

|	 struct ip_options_data {
|	-       struct ip_options_rcu   opt;
|	-       char                    data[40];
|	+       struct ip_options_rcu_hdr       opt;
|	+       char                            data[40];
|	 };

    5) Code churn: This approach may (it really does) require more
       adjustments and refactoring than usual due to multiple type
       changes and the extra flexible-array member.

b) A simpler solution: Inline `struct ip_options_rcu` directly into
   `struct ip_options_data` using an anonymous structure. See below:

    1) Expand `struct ip_options_rcu` with an anonymous structure named
       `opt`, preserving access to `struct ip_options_rcu` members via
       `opt`.

    2) Change the type of `opt` in the anonymous structure from `struct
       ip_options` to `struct ip_options_hdr`, excluding the flexible
       array and thus making the warnings go away:

|	 struct ip_options_data {
|	-       struct ip_options_rcu   opt;
|	+       struct {
|	+               struct rcu_head rcu;
|	+               struct ip_options_hdr opt;
|	+       } opt;
|		char                    data[40];
|	 };

    3) Code churn: This solution requires minimal refactoring beyond
       type casting instances of `opt` back to its original type `struct
       ip_options_rcu`, and a couple of `container_of()`s.

So, I've decided to implement solution b) and with this, fix 2554 of the
following warnings:

include/net/inet_sock.h:64:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Additionally, we want to ensure that when new members need to be added
to the flexible structure, they are always included within the newly
created tagged struct. For this, we use `static_assert()`. This ensures
that the memory layout for both the flexible structure and the new tagged
struct is the same after any changes.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 include/net/inet_sock.h | 40 ++++++++++++++++++++++++----------------
 net/ipv4/icmp.c         | 14 ++++++++++----
 net/ipv4/ip_output.c    |  7 +++++--
 net/ipv4/ping.c         |  2 +-
 net/ipv4/raw.c          |  2 +-
 net/ipv4/udp.c          |  2 +-
 6 files changed, 42 insertions(+), 25 deletions(-)

Comments

Jakub Kicinski Nov. 12, 2024, 2:51 a.m. UTC | #1 
On Mon, 11 Nov 2024 20:34:07 -0600 Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally

RFC is fine, but please don't post anything intended for merging until
we have clarity on the C++ vs uAPI headers issue.
diff mbox series

Patch

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index 56d8bc5593d3..ba4e0e2bbea0 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -37,23 +37,28 @@ 
  * @ts_needaddr - Need to record addr of outgoing dev
  */
 struct ip_options {
-	__be32		faddr;
-	__be32		nexthop;
-	unsigned char	optlen;
-	unsigned char	srr;
-	unsigned char	rr;
-	unsigned char	ts;
-	unsigned char	is_strictroute:1,
-			srr_is_hit:1,
-			is_changed:1,
-			rr_needaddr:1,
-			ts_needtime:1,
-			ts_needaddr:1;
-	unsigned char	router_alert;
-	unsigned char	cipso;
-	unsigned char	__pad2;
+	/* New members MUST be added within the struct_group() macro below. */
+	struct_group_tagged(ip_options_hdr, __hdr,
+		__be32		faddr;
+		__be32		nexthop;
+		unsigned char	optlen;
+		unsigned char	srr;
+		unsigned char	rr;
+		unsigned char	ts;
+		unsigned char	is_strictroute:1,
+				srr_is_hit:1,
+				is_changed:1,
+				rr_needaddr:1,
+				ts_needtime:1,
+				ts_needaddr:1;
+		unsigned char	router_alert;
+		unsigned char	cipso;
+		unsigned char	__pad2;
+	);
 	unsigned char	__data[];
 };
+static_assert(offsetof(struct ip_options, __data) == sizeof(struct ip_options_hdr),
+	      "struct member likely outside of struct_group_tagged()");
 
 struct ip_options_rcu {
 	struct rcu_head rcu;
@@ -61,7 +66,10 @@  struct ip_options_rcu {
 };
 
 struct ip_options_data {
-	struct ip_options_rcu	opt;
+	struct {
+		struct rcu_head rcu;
+		struct ip_options_hdr opt;
+	} opt;
 	char			data[40];
 };
 
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 33eec844a5a0..6a0a38c33cae 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -400,6 +400,9 @@  static void icmp_push_reply(struct sock *sk,
 
 static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 {
+	struct ip_options *opt =
+			container_of(&icmp_param->replyopts.opt.opt,
+				     struct ip_options, __hdr);
 	struct ipcm_cookie ipc;
 	struct rtable *rt = skb_rtable(skb);
 	struct net *net = dev_net(rt->dst.dev);
@@ -412,7 +415,7 @@  static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	int type = icmp_param->data.icmph.type;
 	int code = icmp_param->data.icmph.code;
 
-	if (ip_options_echo(net, &icmp_param->replyopts.opt.opt, skb))
+	if (ip_options_echo(net, opt, skb))
 		return;
 
 	/* Needed by both icmpv4_global_allow and icmp_xmit_lock */
@@ -436,7 +439,7 @@  static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	saddr = fib_compute_spec_dst(skb);
 
 	if (icmp_param->replyopts.opt.opt.optlen) {
-		ipc.opt = &icmp_param->replyopts.opt;
+		ipc.opt = (struct ip_options_rcu *)&icmp_param->replyopts.opt;
 		if (ipc.opt->opt.srr)
 			daddr = icmp_param->replyopts.opt.opt.faddr;
 	}
@@ -592,6 +595,7 @@  static struct rtable *icmp_route_lookup(struct net *net, struct flowi4 *fl4,
 void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 		 const struct ip_options *opt)
 {
+	struct ip_options *opt_aux;
 	struct iphdr *iph;
 	int room;
 	struct icmp_bxm icmp_param;
@@ -605,6 +609,8 @@  void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 	struct net *net;
 	struct sock *sk;
 
+	opt_aux = container_of(&icmp_param.replyopts.opt.opt, struct ip_options, __hdr);
+
 	if (!rt)
 		goto out;
 
@@ -719,7 +725,7 @@  void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 					   iph->tos;
 	mark = IP4_REPLY_MARK(net, skb_in->mark);
 
-	if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in, opt))
+	if (__ip_options_echo(net, opt_aux, skb_in, opt))
 		goto out_unlock;
 
 
@@ -736,7 +742,7 @@  void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 	inet_sk(sk)->tos = tos;
 	ipcm_init(&ipc);
 	ipc.addr = iph->saddr;
-	ipc.opt = &icmp_param.replyopts.opt;
+	ipc.opt = (struct ip_options_rcu *)&icmp_param.replyopts.opt;
 	ipc.sockc.mark = mark;
 
 	rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr,
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 0065b1996c94..17eeb3e09e95 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1604,6 +1604,7 @@  void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
 			   unsigned int len, u64 transmit_time, u32 txhash)
 {
 	struct ip_options_data replyopts;
+	struct ip_options *opt;
 	struct ipcm_cookie ipc;
 	struct flowi4 fl4;
 	struct rtable *rt = skb_rtable(skb);
@@ -1612,7 +1613,9 @@  void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
 	int err;
 	int oif;
 
-	if (__ip_options_echo(net, &replyopts.opt.opt, skb, sopt))
+	opt = container_of(&replyopts.opt.opt, struct ip_options, __hdr);
+
+	if (__ip_options_echo(net, opt, skb, sopt))
 		return;
 
 	ipcm_init(&ipc);
@@ -1620,7 +1623,7 @@  void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
 	ipc.sockc.transmit_time = transmit_time;
 
 	if (replyopts.opt.opt.optlen) {
-		ipc.opt = &replyopts.opt;
+		ipc.opt = (struct ip_options_rcu *)&replyopts.opt;
 
 		if (replyopts.opt.opt.srr)
 			daddr = replyopts.opt.opt.faddr;
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 619ddc087957..ef853dc3948f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -753,7 +753,7 @@  static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		if (inet_opt) {
 			memcpy(&opt_copy, inet_opt,
 			       sizeof(*inet_opt) + inet_opt->opt.optlen);
-			ipc.opt = &opt_copy.opt;
+			ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
 		}
 		rcu_read_unlock();
 	}
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 0e9e01967ec9..cea8a559132f 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -563,7 +563,7 @@  static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		if (inet_opt) {
 			memcpy(&opt_copy, inet_opt,
 			       sizeof(*inet_opt) + inet_opt->opt.optlen);
-			ipc.opt = &opt_copy.opt;
+			ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
 		}
 		rcu_read_unlock();
 	}
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 0e24916b39d4..6ff2329aa0fa 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1159,7 +1159,7 @@  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		if (inet_opt) {
 			memcpy(&opt_copy, inet_opt,
 			       sizeof(*inet_opt) + inet_opt->opt.optlen);
-			ipc.opt = &opt_copy.opt;
+			ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
 		}
 		rcu_read_unlock();
 	}