[PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Anson Huang]

commit bfd694d5e21c ("mmc: core: Add tunable delay
before detecting card after card is inserted") adds
"u32 cd_debounce_delay_ms" to the last of mmc_gpio
struct and cause "char cd_label[0]" NOT work as string
pointer of card detect label, when "cat /proc/interrupts",
the devname for card detect gpio is incorrect as below:

144:          0  gpio-mxc  22 Edge      ▒
161:          0  gpio-mxc   7 Edge      ▒

Move the cd_label field down to fix this, and drop the
zero from the array size to prevent future similar bugs,
the result is correct as below:

144:          0  gpio-mxc  22 Edge      2198000.mmc cd
161:          0  gpio-mxc   7 Edge      2190000.mmc cd

Fixes: bfd694d5e21c ("mmc: core: Add tunable delay before detecting card after card is inserted")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
---
changes since V1:
	- Add fix tag;
	- Drop the zero from the array size, then gcc will have compiling error for such kind of issue next time.
 drivers/mmc/core/slot-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index ef05e00..2a83368 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -27,8 +27,8 @@ struct mmc_gpio {
 	bool override_cd_active_level;
 	irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
 	char *ro_label;
-	char cd_label[0];
 	u32 cd_debounce_delay_ms;
+	char cd_label[];
 };
 
 static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
-- 
2.7.4

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Linus Walleij]

On Mon, Jul 2, 2018 at 3:32 AM Anson Huang <Anson.Huang@nxp.com> wrote:

> commit bfd694d5e21c ("mmc: core: Add tunable delay
> before detecting card after card is inserted") adds
> "u32 cd_debounce_delay_ms" to the last of mmc_gpio
> struct and cause "char cd_label[0]" NOT work as string
> pointer of card detect label, when "cat /proc/interrupts",
> the devname for card detect gpio is incorrect as below:
>
> 144:          0  gpio-mxc  22 Edge      ▒
> 161:          0  gpio-mxc   7 Edge      ▒
>
> Move the cd_label field down to fix this, and drop the
> zero from the array size to prevent future similar bugs,
> the result is correct as below:
>
> 144:          0  gpio-mxc  22 Edge      2198000.mmc cd
> 161:          0  gpio-mxc   7 Edge      2190000.mmc cd
>
> Fixes: bfd694d5e21c ("mmc: core: Add tunable delay before detecting card after card is inserted")
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

Curious.

> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -27,8 +27,8 @@ struct mmc_gpio {
>         bool override_cd_active_level;
>         irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
>         char *ro_label;
> -       char cd_label[0];
>         u32 cd_debounce_delay_ms;
> +       char cd_label[];

Not that I am the smartest in the world when it comes to how the
C compilers work this out.

But isn't that equivalent to:
char *cd_label;
?

Look at this from drivers/mmc/core/slot-gpio.c:

ctx->ro_label = ctx->cd_label + len;
ctx->cd_debounce_delay_ms = 200;
snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
host->slot.handler_priv = ctx;
host->slot.cd_irq = -EINVAL;

So now that ro_label is char * that points len chars into the
struct and that is all fine as the ctx is allocated like that:

struct mmc_gpio *ctx = devm_kzalloc(host->parent,
                               sizeof(*ctx) + 2 * len, GFP_KERNEL);

And I've seen this being done to allocate a state with
some dynamic strings after it.

But I just don't like this, it seems fragile and now it broke.

What about this:

From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Mon, 2 Jul 2018 13:35:01 +0200
Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically

The use of string pointers in the MMC slot GPIO context is
pretty dubious, allocating some 2*len extra bytes for each
label of the ro and wp pins.

Tidy this up using kasprintf() with dynamic allocation of
labels for these strings.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/slot-gpio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index ef05e0039378..c417801b366e 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -27,7 +27,7 @@ struct mmc_gpio {
     bool override_cd_active_level;
     irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
     char *ro_label;
-    char cd_label[0];
+    char *cd_label;
     u32 cd_debounce_delay_ms;
 };

@@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host)
 {
     size_t len = strlen(dev_name(host->parent)) + 4;
     struct mmc_gpio *ctx = devm_kzalloc(host->parent,
-                sizeof(*ctx) + 2 * len,    GFP_KERNEL);
+                        sizeof(*ctx), GFP_KERNEL);

     if (ctx) {
-        ctx->ro_label = ctx->cd_label + len;
         ctx->cd_debounce_delay_ms = 200;
-        snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
-        snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
+        ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL,
+                "%s cd", dev_name(host->parent));
+        if (!ctx->cd_label)
+            return -ENOMEM;
+        ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL,
+                "%s ro", dev_name(host->parent));
+        if (!ctx->ro_label)
+            return -ENOMEM;
         host->slot.handler_priv = ctx;
         host->slot.cd_irq = -EINVAL;
     }
-- 
2.17.0

I can send it as a separate patch if you like it.

Yours,
Linus Walleij

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Fabio Estevam]

Hi Linus,

On Mon, Jul 2, 2018 at 8:42 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

> What about this:
>
> From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Mon, 2 Jul 2018 13:35:01 +0200
> Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically
>
> The use of string pointers in the MMC slot GPIO context is
> pretty dubious, allocating some 2*len extra bytes for each
> label of the ro and wp pins.
>
> Tidy this up using kasprintf() with dynamic allocation of
> labels for these strings.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Your proposed solution also worked fine, thanks.

RE: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Anson Huang]

Hi, Linus

Anson Huang
Best Regards!


> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Monday, July 2, 2018 7:42 PM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Masahiro Yamada
> <yamada.masahiro@socionext.com>; Adrian Hunter
> <adrian.hunter@intel.com>; evgreen@chromium.org; Shawn Lin
> <shawn.lin@rock-chips.com>; Fabio Estevam <fabio.estevam@nxp.com>;
> linux-mmc <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio
> struct
> 
> On Mon, Jul 2, 2018 at 3:32 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> 
> > commit bfd694d5e21c ("mmc: core: Add tunable delay before detecting
> > card after card is inserted") adds
> > "u32 cd_debounce_delay_ms" to the last of mmc_gpio struct and cause
> > "char cd_label[0]" NOT work as string pointer of card detect label,
> > when "cat /proc/interrupts", the devname for card detect gpio is
> > incorrect as below:
> >
> > 144:          0  gpio-mxc  22 Edge      ▒
> > 161:          0  gpio-mxc   7 Edge      ▒
> >
> > Move the cd_label field down to fix this, and drop the zero from the
> > array size to prevent future similar bugs, the result is correct as
> > below:
> >
> > 144:          0  gpio-mxc  22 Edge      2198000.mmc cd
> > 161:          0  gpio-mxc   7 Edge      2190000.mmc cd
> >
> > Fixes: bfd694d5e21c ("mmc: core: Add tunable delay before detecting
> > card after card is inserted")
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Curious.
> 
> > --- a/drivers/mmc/core/slot-gpio.c
> > +++ b/drivers/mmc/core/slot-gpio.c
> > @@ -27,8 +27,8 @@ struct mmc_gpio {
> >         bool override_cd_active_level;
> >         irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
> >         char *ro_label;
> > -       char cd_label[0];
> >         u32 cd_debounce_delay_ms;
> > +       char cd_label[];
> 
> Not that I am the smartest in the world when it comes to how the C compilers
> work this out.
> 
> But isn't that equivalent to:
> char *cd_label;
> ?

I think C compiler will check structure if it has flexible array, the flexible array much be at end
of a structure, and its size can be dynamic changed by mallocing a memory for this structure,
there are many such case in kernel. If the flexible array is NOT at the end of a struct,
error " error: flexible array member not at end of struct" will come out by C compiler.

> 
> Look at this from drivers/mmc/core/slot-gpio.c:
> 
> ctx->ro_label = ctx->cd_label + len;
> ctx->cd_debounce_delay_ms = 200;
> snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
> snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> host->slot.handler_priv = ctx;
> host->slot.cd_irq = -EINVAL;
> 
> So now that ro_label is char * that points len chars into the struct and that is all
> fine as the ctx is allocated like that:
> 
> struct mmc_gpio *ctx = devm_kzalloc(host->parent,
>                                sizeof(*ctx) + 2 * len, GFP_KERNEL);
> 
> And I've seen this being done to allocate a state with some dynamic strings
> after it.
> 
> But I just don't like this, it seems fragile and now it broke.
 
I think the difference of current implementation and using two string pointers are,
current implementation can only alloc memory for mmc_gpio structure once, the ro_label's address
can be got by cd_label's address. If using two string pointers for ro_label and cd_label,
we have to alloc twice for them separately.

> 
> What about this:
> 
> From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00
> 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Mon, 2 Jul 2018 13:35:01 +0200
> Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically
> 
> The use of string pointers in the MMC slot GPIO context is pretty dubious,
> allocating some 2*len extra bytes for each label of the ro and wp pins.
> 
> Tidy this up using kasprintf() with dynamic allocation of labels for these strings.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/slot-gpio.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c index
> ef05e0039378..c417801b366e 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -27,7 +27,7 @@ struct mmc_gpio {
>      bool override_cd_active_level;
>      irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
>      char *ro_label;
> -    char cd_label[0];
> +    char *cd_label;
>      u32 cd_debounce_delay_ms;
>  };
> 
> @@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host)  {
>      size_t len = strlen(dev_name(host->parent)) + 4;
 
len is no need any more.

>      struct mmc_gpio *ctx = devm_kzalloc(host->parent,
> -                sizeof(*ctx) + 2 * len,    GFP_KERNEL);
> +                        sizeof(*ctx), GFP_KERNEL);
> 
>      if (ctx) {
> -        ctx->ro_label = ctx->cd_label + len;
>          ctx->cd_debounce_delay_ms = 200;
> -        snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
> -        snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> +        ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL,
> +                "%s cd", dev_name(host->parent));
> +        if (!ctx->cd_label)
> +            return -ENOMEM;
> +        ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL,
> +                "%s ro", dev_name(host->parent));
> +        if (!ctx->ro_label)
> +            return -ENOMEM;
>          host->slot.handler_priv = ctx;
>          host->slot.cd_irq = -EINVAL;
>      }
> --
> 2.17.0
> 
> I can send it as a separate patch if you like it.

I think either way is OK, since flexible array is used in kernel code quite commonly,
so I prefer to make code change as small as possible, the original patch can also prevent
similar bug in future. And like below commit Fabio pointed out, it also has same kind
of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks.

Anson.

> 
> Yours,
> Linus Walleij

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Fabio Estevam]

On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote:

> I think either way is OK, since flexible array is used in kernel code quite commonly,
> so I prefer to make code change as small as possible, the original patch can also prevent
> similar bug in future. And like below commit Fabio pointed out, it also has same kind
> of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks.

I am also fine with this patch or the one from Linus.

Maybe Anson's patch could be applied to 4.18-rc as a bug fix and
Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement?

Ulf,

What would you prefer?

Thanks

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Ulf Hansson]

On 3 July 2018 at 04:25, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote:
>
>> I think either way is OK, since flexible array is used in kernel code quite commonly,
>> so I prefer to make code change as small as possible, the original patch can also prevent
>> similar bug in future. And like below commit Fabio pointed out, it also has same kind
>> of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks.
>
> I am also fine with this patch or the one from Linus.
>
> Maybe Anson's patch could be applied to 4.18-rc as a bug fix and
> Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement?

Good idea!

I have queued up Anson's v2 patch as a fix for 4.18.

Linus, can please re-post a re-based version of your change on top!? I
guess you can add Anson's and Fabio's reviewed by tags for it as well.

Kind regards
Uffe

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Linus Walleij]

On Tue, Jul 3, 2018 at 7:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 3 July 2018 at 04:25, Fabio Estevam <festevam@gmail.com> wrote:
> > On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote:
> >
> >> I think either way is OK, since flexible array is used in kernel code quite commonly,
> >> so I prefer to make code change as small as possible, the original patch can also prevent
> >> similar bug in future. And like below commit Fabio pointed out, it also has same kind
> >> of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks.
> >
> > I am also fine with this patch or the one from Linus.
> >
> > Maybe Anson's patch could be applied to 4.18-rc as a bug fix and
> > Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement?
>
> Good idea!
>
> I have queued up Anson's v2 patch as a fix for 4.18.
>
> Linus, can please re-post a re-based version of your change on top!? I
> guess you can add Anson's and Fabio's reviewed by tags for it as well.

Sure thing! I'll fold in Andy's suggested fix as well.

Yours,
Linus Walleij

Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio struct

[Andy Shevchenko]

On Mon, Jul 2, 2018 at 2:42 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jul 2, 2018 at 3:32 AM Anson Huang <Anson.Huang@nxp.com> wrote:

>> -       char cd_label[0];
>> +       char cd_label[];
>
> Not that I am the smartest in the world when it comes to how the
> C compilers work this out.
>
> But isn't that equivalent to:
> char *cd_label;
> ?

No.

sizeof(foo) == bar in the original code and with your change it would be
sizeof(foo) == bar + sizeof(void *)

On top of that, the actual memory is going in one chunk and requires
only one allocation.

But, I like your below stuff, though comments

> From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Mon, 2 Jul 2018 13:35:01 +0200
> Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically
>
> The use of string pointers in the MMC slot GPIO context is
> pretty dubious, allocating some 2*len extra bytes for each
> label of the ro and wp pins.
>
> Tidy this up using kasprintf() with dynamic allocation of
> labels for these strings.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/slot-gpio.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index ef05e0039378..c417801b366e 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -27,7 +27,7 @@ struct mmc_gpio {
>      bool override_cd_active_level;
>      irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
>      char *ro_label;
> -    char cd_label[0];
> +    char *cd_label;
>      u32 cd_debounce_delay_ms;
>  };
>
> @@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host)
>  {

>      size_t len = strlen(dev_name(host->parent)) + 4;

Do you need this still?

>      struct mmc_gpio *ctx = devm_kzalloc(host->parent,
> -                sizeof(*ctx) + 2 * len,    GFP_KERNEL);
> +                        sizeof(*ctx), GFP_KERNEL);
>
>      if (ctx) {
> -        ctx->ro_label = ctx->cd_label + len;
>          ctx->cd_debounce_delay_ms = 200;
> -        snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
> -        snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> +        ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL,
> +                "%s cd", dev_name(host->parent));
> +        if (!ctx->cd_label)
> +            return -ENOMEM;
> +        ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL,
> +                "%s ro", dev_name(host->parent));
> +        if (!ctx->ro_label)
> +            return -ENOMEM;
>          host->slot.handler_priv = ctx;
>          host->slot.cd_irq = -EINVAL;
>      }

-- 
With Best Regards,
Andy Shevchenko