All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jacob Keller <jacob.e.keller@intel.com>
To: Peter Rosin <peda@axentia.se>
Cc: linux-kernel@vger.kernel.org, Jacob Keller <jacob.e.keller@intel.com>
Subject: [PATCH -next] mux: convert mux_chip->mux to flexible array
Date: Wed, 22 Feb 2023 17:42:21 -0800	[thread overview]
Message-ID: <20230223014221.1710307-1-jacob.e.keller@intel.com> (raw)

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  1400 */

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

        /* --- cacheline 22 boundary (1408 bytes) was 8 bytes ago --- */
        int                        id;                   /*  1416     4 */

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

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

        /* size: 1432, cachelines: 23, members: 5 */
        /* sum members: 1424, holes: 2, sum holes: 8 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 24 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  1400 */

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

        /* --- cacheline 22 boundary (1408 bytes) --- */
        const struct mux_control_ops  * ops;             /*  1408     8 */
        struct mux_control         mux[];                /*  1416     0 */

        /* size: 1416, cachelines: 23, members: 5 */
        /* paddings: 1, sum paddings: 3 */
        /* forced alignments: 1 */
        /* last cacheline: 8 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>
---
 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 49bedbe6316c..3e26b9911cc2 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -100,13 +100,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: 307e14c039063f0c9bd7a18a7add8f940580dcc9
-- 
I found this while developing a coccinelle patch that helps detect potential
code that could be converted to struct_size() and noticed this weird case
that could be a flexible array and save memory.

2.39.1.405.gd4c25cc71f83


             reply	other threads:[~2023-02-23  1:42 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-23  1:42 Jacob Keller [this message]
2023-02-27 20:28 ` [PATCH -next] mux: convert mux_chip->mux to flexible array Jesse Brandeburg
2024-02-19  5:04   ` Kees Cook
2024-02-20 21:27     ` Jacob Keller
2024-02-23 23:52       ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230223014221.1710307-1-jacob.e.keller@intel.com \
    --to=jacob.e.keller@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peda@axentia.se \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.