[PATCH RFC 0/5] pahole --with_embedded_flexible_array

[PATCH RFC 0/5] pahole --with_embedded_flexible_array

[Arnaldo Carvalho de Melo]

Hi Willy, Gustavo,

	Please take a look, there are still some odd cases where the
comment is not being emitted but _there_ is a flexible array inside the
struct marked as having a flexible array "in the middle" (by means of
an embedded struct that _has_ a flexible array.

	This helps with one other case Gustavo talked about at KR.

	Please take a look and give it some testing, please.

- Arnaldo

Arnaldo Carvalho de Melo (5):
  core: Move class__has_flexible_array() from pahole to the core
  core: Cache info about flexible arrays in class__has_flexible_array()
  core: Introduce class__has_embedded_flexible_array()
  fprintf: Add a comment if a member type has an embedded flexible array
  pahole: Introduce --with_embedded_flexible_array

 dwarves.c          | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 dwarves.h          |  6 ++++
 dwarves_fprintf.c  | 12 ++++++--
 man-pages/pahole.1 |  4 +++
 pahole.c           | 36 ++++++++--------------
 5 files changed, 109 insertions(+), 26 deletions(-)

-- 
2.46.2

[PATCH 1/5] core: Move class__has_flexible_array() from pahole to the core

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

We want to cache that info, look if there are embedded fields of structs
with flexible arrays, etc.

Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarves.c | 23 +++++++++++++++++++++++
 dwarves.h |  1 +
 pahole.c  | 23 -----------------------
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/dwarves.c b/dwarves.c
index 6e6a9e69a61d1e37..3d91e629277a329f 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -1575,6 +1575,29 @@ void lexblock__add_label(struct lexblock *block, struct label *label)
 	lexblock__add_tag(block, &label->ip.tag);
 }
 
+bool class__has_flexible_array(struct class *class, const struct cu *cu)
+{
+	struct class_member *member = type__last_member(&class->type);
+
+	if (member == NULL)
+		return false;
+
+	struct tag *type = cu__type(cu, member->tag.type);
+
+	if (type->tag != DW_TAG_array_type)
+		return false;
+
+	struct array_type *array = tag__array_type(type);
+
+	if (array->dimensions > 1)
+		return false;
+
+	if (array->nr_entries == NULL || array->nr_entries[0] == 0)
+		return true;
+
+	return false;
+}
+
 const struct class_member *class__find_bit_hole(const struct class *class,
 					    const struct class_member *trailer,
 						const uint16_t bit_hole_size)
diff --git a/dwarves.h b/dwarves.h
index 2bb0eed461d08d51..757fc05dd3363bf1 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1454,6 +1454,7 @@ static inline int class__is_struct(const struct class *cls)
 	return tag__is_struct(&cls->type.namespace.tag);
 }
 
+bool class__has_flexible_array(struct class *class, const struct cu *cu);
 void class__find_holes(struct class *cls);
 int class__has_hole_ge(const struct class *cls, const uint16_t size);
 
diff --git a/pahole.c b/pahole.c
index 766835d93a1f3d1e..8bdfaf3e84c407d7 100644
--- a/pahole.c
+++ b/pahole.c
@@ -725,29 +725,6 @@ static int class__packable(struct class *class, struct cu *cu)
 	return 0;
 }
 
-static bool class__has_flexible_array(struct class *class, struct cu *cu)
-{
-	struct class_member *member = type__last_member(&class->type);
-
-	if (member == NULL)
-		return false;
-
-	struct tag *type = cu__type(cu, member->tag.type);
-
-	if (type->tag != DW_TAG_array_type)
-		return false;
-
-	struct array_type *array = tag__array_type(type);
-
-	if (array->dimensions > 1)
-		return false;
-
-	if (array->nr_entries == NULL || array->nr_entries[0] == 0)
-		return true;
-
-	return false;
-}
-
 static struct class *class__filter(struct class *class, struct cu *cu,
 				   uint32_t tag_id)
 {
-- 
2.46.2

[PATCH 2/5] core: Cache info about flexible arrays in class__has_flexible_array()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Just saving some calculations, to be in line with class->holes_searched,
etc.

Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarves.c | 12 +++++++++++-
 dwarves.h |  2 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/dwarves.c b/dwarves.c
index 3d91e629277a329f..f5bc9cb27adee649 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -1575,7 +1575,7 @@ void lexblock__add_label(struct lexblock *block, struct label *label)
 	lexblock__add_tag(block, &label->ip.tag);
 }
 
-bool class__has_flexible_array(struct class *class, const struct cu *cu)
+static bool __class__has_flexible_array(struct class *class, const struct cu *cu)
 {
 	struct class_member *member = type__last_member(&class->type);
 
@@ -1598,6 +1598,16 @@ bool class__has_flexible_array(struct class *class, const struct cu *cu)
 	return false;
 }
 
+bool class__has_flexible_array(struct class *class, const struct cu *cu)
+{
+	if (!class->flexible_array_verified) {
+		class->has_flexible_array = __class__has_flexible_array(class, cu);
+		class->flexible_array_verified = true;
+	}
+
+	return class->has_flexible_array;
+}
+
 const struct class_member *class__find_bit_hole(const struct class *class,
 					    const struct class_member *trailer,
 						const uint16_t bit_hole_size)
diff --git a/dwarves.h b/dwarves.h
index 757fc05dd3363bf1..bcf701706c61b4f5 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1412,6 +1412,8 @@ struct class {
 	uint8_t		 pre_bit_hole;
 	uint8_t		 bit_padding;
 	bool		 holes_searched;
+	bool		 flexible_array_verified;
+	bool		 has_flexible_array;
 	bool		 is_packed;
 	void		 *priv;
 };
-- 
2.46.2

[PATCH 3/5] core: Introduce class__has_embedded_flexible_array()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

That we will use both when pretty printing structs, as a new comment,
and with a 'pahole --with_embedded_flexible_array' to show structs with
embedded structs that have flexible arrays and thus should be used with
super extra care.

Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarves.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 dwarves.h |  3 +++
 2 files changed, 47 insertions(+)

diff --git a/dwarves.c b/dwarves.c
index f5bc9cb27adee649..2ac82956bd5ea45d 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -1737,6 +1737,50 @@ void class__find_holes(struct class *class)
 	class->holes_searched = true;
 }
 
+bool class__has_embedded_flexible_array(struct class *cls, const struct cu *cu)
+{
+	struct type *ctype = &cls->type;
+	struct class_member *pos;
+
+	if (!tag__is_struct(class__tag(cls)))
+		return false;
+
+	if (cls->embedded_flexible_array_searched)
+		return cls->has_embedded_flexible_array;
+
+	type__for_each_member(ctype, pos) {
+		/* XXX for now just skip these */
+		if (pos->tag.tag == DW_TAG_inheritance &&
+		    pos->virtuality == DW_VIRTUALITY_virtual)
+			continue;
+
+		if (pos->is_static)
+			continue;
+
+		struct tag *member_type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
+		if (member_type == NULL)
+			continue;
+
+		if (!tag__is_struct(member_type))
+			continue;
+
+		cls->has_embedded_flexible_array = class__has_flexible_array(tag__class(member_type), cu);
+		if (cls->has_embedded_flexible_array)
+			break;
+
+		if (member_type == class__tag(cls))
+			continue;
+
+		cls->has_embedded_flexible_array = class__has_embedded_flexible_array(tag__class(member_type), cu);
+		if (cls->has_embedded_flexible_array)
+			break;
+	}
+
+	cls->embedded_flexible_array_searched = true;
+
+	return cls->has_embedded_flexible_array;
+}
+
 static size_t type__natural_alignment(struct type *type, const struct cu *cu);
 
 size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
diff --git a/dwarves.h b/dwarves.h
index bcf701706c61b4f5..f147b2bcfd8a153e 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1413,7 +1413,9 @@ struct class {
 	uint8_t		 bit_padding;
 	bool		 holes_searched;
 	bool		 flexible_array_verified;
+	bool		 embedded_flexible_array_searched;
 	bool		 has_flexible_array;
+	bool		 has_embedded_flexible_array;
 	bool		 is_packed;
 	void		 *priv;
 };
@@ -1456,6 +1458,7 @@ static inline int class__is_struct(const struct class *cls)
 	return tag__is_struct(&cls->type.namespace.tag);
 }
 
+bool class__has_embedded_flexible_array(struct class *cls, const struct cu *cu);
 bool class__has_flexible_array(struct class *class, const struct cu *cu);
 void class__find_holes(struct class *cls);
 int class__has_hole_ge(const struct class *cls, const uint16_t size);
-- 
2.46.2

[PATCH 4/5] fprintf: Add a comment if a member type has an embedded flexible array

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

For instance:

  acme@x1:~/git/pahole$ pahole cgroup_root
  struct cgroup_root {
          struct kernfs_root *       kf_root;              /*     0     8 */
          unsigned int               subsys_mask;          /*     8     4 */
          int                        hierarchy_id;         /*    12     4 */
          struct list_head           root_list;            /*    16    16 */
          struct callback_head       rcu;                  /*    32    16 */

          /* XXX 16 bytes hole, try to pack */

          /* --- cacheline 1 boundary (64 bytes) --- */
          struct cgroup              cgrp __attribute__((__aligned__(64))); /*    64  1984 */

          /* XXX last struct has a flexible array, 3 holes */

          /* --- cacheline 32 boundary (2048 bytes) --- */
          struct cgroup *            cgrp_ancestor_storage; /*  2048     8 */
          atomic_t                   nr_cgrps;             /*  2056     4 */
          unsigned int               flags;                /*  2060     4 */
          char                       release_agent_path[4096]; /*  2064  4096 */
          /* --- cacheline 96 boundary (6144 bytes) was 16 bytes ago --- */
          char                       name[64];             /*  6160    64 */

          /* size: 6272, cachelines: 98, members: 11 */
          /* sum members: 6208, holes: 1, sum holes: 16 */
          /* padding: 48 */
          /* member types with holes: 1, total: 3 */
          /* forced alignments: 1, forced holes: 1, sum forced holes: 16 */
  } __attribute__((__aligned__(64)));
  acme@x1:~/git/pahole$

And then:

  acme@x1:~/git/pahole$ pahole cgroup | tail -20
  	/* XXX last struct has 1 hole */

  	/* --- cacheline 30 boundary (1920 bytes) was 32 bytes ago --- */
  	atomic_t                   congestion_count;     /*  1952     4 */
  	struct cgroup_freezer_state freezer;             /*  1956    16 */

  	/* XXX last struct has 1 hole */
  	/* XXX 4 bytes hole, try to pack */

  	struct bpf_local_storage * bpf_cgrp_storage;     /*  1976     8 */
  	/* --- cacheline 31 boundary (1984 bytes) --- */
  	struct cgroup *            ancestors[];          /*  1984     0 */

  	/* size: 1984, cachelines: 31, members: 42 */
  	/* sum members: 1952, holes: 3, sum holes: 32 */
  	/* member types with holes: 3, total: 3 */
  	/* paddings: 1, sum paddings: 4 */
  	/* forced alignments: 1 */
  };

  acme@x1:~/git/pahole$

Care must be taken on how to use that cgroup->ancestors field since
cgroup_root->cgrp is in the middle of 'struct cgroup_root', so accesses
to cgroup->ancestors[1] would be accessing other fields of cgroup_root
after cgrp, i.e. further analysis and checks in the code handling those
structs is needed.

This comes from discussions with Willy Tarreau after Gustavo A. R.
Silva's presentation at Kernel Recipes'2024:

  https://speakerdeck.com/ennael/enhancing-spatial-safety-fixing-thousands-of-wflex-array-member-not-at-end-warnings?slide=11

Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarves_fprintf.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index ffbe6fbd3f5c405a..94a152183b0725cf 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -1513,7 +1513,7 @@ static size_t class__fprintf_member_type_holes(struct class *class, const struct
 	size_t printed = 0;
 	uint16_t padding;
 	uint8_t nr_holes, nr_bit_holes, bit_padding;
-	bool first = true;
+	bool first = true, has_embedded_flexible_array;
 	/*
 	 * We may not yet have looked for holes and paddings in this member's
 	 * struct type.
@@ -1525,8 +1525,9 @@ static size_t class__fprintf_member_type_holes(struct class *class, const struct
 	bit_padding = class->bit_padding;
 	nr_holes = class->nr_holes;
 	nr_bit_holes = class->nr_bit_holes;
+	has_embedded_flexible_array = class__has_embedded_flexible_array(class, cu);
 
-	if (!padding && !bit_padding && !nr_holes && !nr_bit_holes)
+	if (!padding && !bit_padding && !nr_holes && !nr_bit_holes && !has_embedded_flexible_array)
 		return 0;
 
 	if (!(*newline)++) {
@@ -1536,11 +1537,16 @@ static size_t class__fprintf_member_type_holes(struct class *class, const struct
 
 	printed += fprintf(fp, "\n%.*s/* XXX last struct has", conf->indent, tabs);
 
+	if (has_embedded_flexible_array) {
+		printed += fprintf(fp, " a flexible array");
+		first = false;
+	}
+
 	if (padding) {
 		++holes->nr_paddings;
 		holes->sum_paddings += padding;
 
-		printed += fprintf(fp, " %d byte%s of padding", padding, padding != 1 ? "s" : "");
+		printed += fprintf(fp, "%s %d byte%s of padding", first ? "" : ",", padding, padding != 1 ? "s" : "");
 		first = false;
 	}
 
-- 
2.46.2

[PATCH 5/5] pahole: Introduce --with_embedded_flexible_array

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

To print structs that have as one of its members a struct with a
flexible array, i.e. the outer script has a flexible array in its
middle, which is dangerous.

One example should illustrate this nicely:

  ⬢[acme@toolbox pahole]$ pahole --with_embedded_flexible_array --sizes | wc -l
  103
  ⬢[acme@toolbox pahole]$ pahole --with_embedded_flexible_array --sizes | tail
  acpi_nhlt_format_config	44	0
  xts_request_ctx	168	0
  dbc_port	512	1
  ethtool_link_usettings	96	0
  pci_setup_rom	64	1
  lirc_fh	112	1
  lrw_request_ctx	96	0
  xt_standard_target	40	0
  xt_error_target	64	0
  bpf_ctx_convert	7280	0
  ⬢[acme@toolbox pahole]$ pahole lirc_fh
  struct lirc_fh {
  	struct list_head           list;                 /*     0    16 */
  	struct rc_dev *            rc;                   /*    16     8 */
  	int                        carrier_low;          /*    24     4 */

  	/* XXX 4 bytes hole, try to pack */

  	struct {
  		union {
  			struct __kfifo kfifo;            /*    32    24 */
  			unsigned int * type;             /*    32     8 */
  			const unsigned int  * const_type; /*    32     8 */
  			char *     rectype;              /*    32     8 */
  			unsigned int * ptr;              /*    32     8 */
  			const unsigned int  * ptr_const; /*    32     8 */
  		};                                       /*    32    24 */
  		unsigned int       buf[];                /*    56     0 */
  	} rawir;                                         /*    32    24 */
  	struct {
  		union {
  			struct __kfifo kfifo;            /*    56    24 */
  			struct lirc_scancode * type;     /*    56     8 */
  			const struct lirc_scancode  * const_type; /*    56     8 */
  			char *     rectype;              /*    56     8 */
  			struct lirc_scancode * ptr;      /*    56     8 */
  			const struct lirc_scancode  * ptr_const; /*    56     8 */
  		};                                       /*    56    24 */
  		/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
  		struct lirc_scancode buf[];              /*    80     0 */
  	} scancodes;                                     /*    56    24 */
  	wait_queue_head_t          wait_poll;            /*    80    24 */
  	u8                         send_mode;            /*   104     1 */
  	u8                         rec_mode;             /*   105     1 */

  	/* size: 112, cachelines: 2, members: 8 */
  	/* sum members: 102, holes: 1, sum holes: 4 */
  	/* padding: 6 */
  	/* last cacheline: 48 bytes */
  };

  ⬢[acme@toolbox pahole]$

Another case:

  ⬢[acme@toolbox pahole]$ pahole -E ethtool_link_usettings
  struct ethtool_link_usettings {
  	struct ethtool_link_settings {
  		/* typedef __u32 */ unsigned int       cmd;                              /*     0     4 */
  		/* typedef __u32 */ unsigned int       speed;                            /*     4     4 */
  		/* typedef __u8 */ unsigned char      duplex;                            /*     8     1 */
  		/* typedef __u8 */ unsigned char      port;                              /*     9     1 */
  		/* typedef __u8 */ unsigned char      phy_address;                       /*    10     1 */
  		/* typedef __u8 */ unsigned char      autoneg;                           /*    11     1 */
  		/* typedef __u8 */ unsigned char      mdio_support;                      /*    12     1 */
  		/* typedef __u8 */ unsigned char      eth_tp_mdix;                       /*    13     1 */
  		/* typedef __u8 */ unsigned char      eth_tp_mdix_ctrl;                  /*    14     1 */
  		/* typedef __s8 */ signed char        link_mode_masks_nwords;            /*    15     1 */
  		/* typedef __u8 */ unsigned char      transceiver;                       /*    16     1 */
  		/* typedef __u8 */ unsigned char      master_slave_cfg;                  /*    17     1 */
  		/* typedef __u8 */ unsigned char      master_slave_state;                /*    18     1 */
  		/* typedef __u8 */ unsigned char      rate_matching;                     /*    19     1 */
  		/* typedef __u32 */ unsigned int       reserved[7];                      /*    20    28 */
  		/* typedef __u32 */ unsigned int       link_mode_masks[];                /*    48     0 */
  	} base; /*     0    48 */
  	struct {
  		/* typedef __u32 */ unsigned int       supported[4];                     /*    48    16 */
  		/* --- cacheline 1 boundary (64 bytes) --- */
  		/* typedef __u32 */ unsigned int       advertising[4];                   /*    64    16 */
  		/* typedef __u32 */ unsigned int       lp_advertising[4];                /*    80    16 */
  	} link_modes;                                                                    /*    48    48 */

  	/* size: 96, cachelines: 2, members: 2 */
  	/* last cacheline: 32 bytes */
  };

  ⬢[acme@toolbox pahole]$

Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 man-pages/pahole.1 |  4 ++++
 pahole.c           | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index 7c1a69ad23690c75..19dfe676c9e30509 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -397,6 +397,10 @@ for instance when using the \fB\-\-reorganize\fR option.
 .B \-P, \-\-with_flexible_array
 Show only structs that have a flexible array.
 
+.TP
+.B \-P, \-\-with_embedded_flexible_array
+Show only structs that have an embedded flexible array, i.e. a contained struct that has flexible arrays, a flexible array "in the middle".
+
 .TP
 .B \-q, \-\-quiet
 Be quieter.
diff --git a/pahole.c b/pahole.c
index 8bdfaf3e84c407d7..b9e97ef15e6c732e 100644
--- a/pahole.c
+++ b/pahole.c
@@ -66,6 +66,7 @@ static uint16_t nr_bit_holes;
 static uint16_t hole_size_ge;
 static uint8_t show_packable;
 static bool show_with_flexible_array;
+static bool show_with_embedded_flexible_array;
 static uint8_t global_verbose;
 static uint8_t recursive;
 static size_t cacheline_size;
@@ -825,6 +826,9 @@ static struct class *class__filter(struct class *class, struct cu *cu,
 	if (show_with_flexible_array && !class__has_flexible_array(class, cu))
 		return NULL;
 
+	if (show_with_embedded_flexible_array && !class__has_embedded_flexible_array(class, cu))
+		return NULL;
+
 	return class;
 }
 
@@ -1226,6 +1230,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_running_kernel_vmlinux 346
 #define ARG_padding_ge		   347
 #define ARG_padding		   348
+#define ARGP_with_embedded_flexible_array 349
 
 /* --btf_features=feature1[,feature2,..] allows us to specify
  * a list of requested BTF features or "default" to enable all default
@@ -1505,6 +1510,11 @@ static const struct argp_option pahole__options[] = {
 		.key  = ARGP_with_flexible_array,
 		.doc  = "show only structs with a flexible array",
 	},
+	{
+		.name = "with_embedded_flexible_array",
+		.key  = ARGP_with_embedded_flexible_array,
+		.doc  = "show only structs with an embedded flexible array (contaning a struct that has a flexible array)",
+	},
 	{
 		.name = "expand_types",
 		.key  = 'E',
@@ -2006,6 +2016,9 @@ static error_t pahole__options_parser(int key, char *arg,
 		parse_btf_features("all", false);	break;
 	case ARGP_with_flexible_array:
 		show_with_flexible_array = true;	break;
+	case ARGP_with_embedded_flexible_array:
+		just_structs = true;
+		show_with_embedded_flexible_array = true; break;
 	case ARGP_prettify_input_filename:
 		prettify_input_filename = arg;		break;
 	case ARGP_sort_output:
-- 
2.46.2