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

From: Anson Huang
Date: Mon Jul 02 2018 - 22:13:25 EST

Hi, Linus

Anson Huang
Best Regards!


> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx]
> Sent: Monday, July 2, 2018 7:42 PM
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>; Masahiro Yamada
> <yamada.masahiro@xxxxxxxxxxxxx>; Adrian Hunter
> <adrian.hunter@xxxxxxxxx>; evgreen@xxxxxxxxxxxx; Shawn Lin
> <shawn.lin@xxxxxxxxxxxxxx>; Fabio Estevam <fabio.estevam@xxxxxxx>;
> linux-mmc <linux-mmc@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> dl-linux-imx <linux-imx@xxxxxxx>
> 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@xxxxxxx>
> 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@xxxxxxx>
> > Tested-by: Fabio Estevam <fabio.estevam@xxxxxxx>
>
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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