f:jacob.e.keller AND ( s:struct_size OR s:"flexible array" )

* [PATCH v2] mux: convert mux_chip->mux to flexible array
@ 2024-02-26 21:29 68% Jacob Keller
  0 siblings, 0 replies; 24+ results
From: Jacob Keller @ 2024-02-26 21:29 UTC (permalink / raw)
  To: Kees Cook, linux-i2c, linux-kernel
  Cc: gustavoars, Peter Rosin, Jesse Brandeburg, Jacob Keller

The mux_chip structure size is over allocated to additionally include both
the array of mux controllers as well as a device specific private area.
The controllers array is then pointed to by assigning mux_chip->mux to the
first block of extra memory, while the private area is extracted via
mux_chip_priv() and points to the area just after the controllers.

The size of the mux_chip allocation uses direct multiplication and addition
rather than the <linux/overflow.h> helpers. In addition, the mux_chip->mux
struct member wastes space by having to store the pointer as part of the
structures.

Convert struct mux_chip to use a flexible array member for the mux
controller array. Use struct_size() and size_add() to compute the size of
the structure while protecting against overflow.

After converting the mux pointer, notice that two 4-byte holes remain in
the structure layout due to the alignment requirements for the dev
sub-structure and the ops pointer.

These can be easily fixed through re-ordering the id field to the 4-byte
hole just after the controllers member.

This changes the layout from:

struct mux_chip {
        unsigned int               controllers;          /*     0     4 */

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

        struct mux_control *       mux;                  /*     8     8 */
        struct device              dev __attribute__((__aligned__(8))); /*    16  1488 */

        /* XXX last struct has 3 bytes of padding */

        /* --- cacheline 23 boundary (1472 bytes) was 32 bytes ago --- */
        int                        id;                   /*  1504     4 */

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

        const struct mux_control_ops  * ops;             /*  1512     8 */

        /* size: 1520, cachelines: 24, members: 5 */
        /* sum members: 1512, holes: 2, sum holes: 8 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 48 bytes */
} __attribute__((__aligned__(8)));

To the following:

struct mux_chip {
        unsigned int               controllers;          /*     0     4 */
        int                        id;                   /*     4     4 */
        struct device              dev __attribute__((__aligned__(8))); /*     8  1488 */

        /* XXX last struct has 3 bytes of padding */

        /* --- cacheline 23 boundary (1472 bytes) was 24 bytes ago --- */
        const struct mux_control_ops  * ops;             /*  1496     8 */
        struct mux_control         mux[];                /*  1504     0 */

        /* size: 1504, cachelines: 24, members: 5 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 32 bytes */
} __attribute__((__aligned__(8)));

This both removes risk of overflowing and performing an under-allocation,
as well as saves 16 bytes of otherwise wasted space for every mux_chip.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1:
* Rebased and updated the commit message slightly.

 drivers/mux/core.c         |  7 +++----
 include/linux/mux/driver.h | 10 +++++-----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..9225abca7897 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -98,13 +98,12 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
 	if (WARN_ON(!dev || !controllers))
 		return ERR_PTR(-EINVAL);
 
-	mux_chip = kzalloc(sizeof(*mux_chip) +
-			   controllers * sizeof(*mux_chip->mux) +
-			   sizeof_priv, GFP_KERNEL);
+	mux_chip = kzalloc(size_add(struct_size(mux_chip, mux, controllers),
+				    sizeof_priv),
+			   GFP_KERNEL);
 	if (!mux_chip)
 		return ERR_PTR(-ENOMEM);
 
-	mux_chip->mux = (struct mux_control *)(mux_chip + 1);
 	mux_chip->dev.class = &mux_class;
 	mux_chip->dev.type = &mux_type;
 	mux_chip->dev.parent = dev;
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..84dc0d3e79d6 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -56,18 +56,18 @@ struct mux_control {
 /**
  * struct mux_chip -	Represents a chip holding mux controllers.
  * @controllers:	Number of mux controllers handled by the chip.
- * @mux:		Array of mux controllers that are handled.
- * @dev:		Device structure.
  * @id:			Used to identify the device internally.
+ * @dev:		Device structure.
  * @ops:		Mux controller operations.
+ * @mux:		Flexible array of mux controllers that are handled.
  */
 struct mux_chip {
 	unsigned int controllers;
-	struct mux_control *mux;
-	struct device dev;
 	int id;
-
+	struct device dev;
 	const struct mux_control_ops *ops;
+
+	struct mux_control mux[];
 };
 
 #define to_mux_chip(x) container_of((x), struct mux_chip, dev)

base-commit: 45ec2f5f6ed3ec3a79ba1329ad585497cdcbe663
-- 
2.41.0


^ permalink raw reply related	[relevance 68%]