Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1560507893-42553-1-git-send-email-john.garry@huawei.com> <CAErSpo6jRVDaVvH+9XvzUoEUbHi9_6u+5PcVGVDAmre6Qd0WeQ@mail.gmail.com>
In-Reply-To: <CAErSpo6jRVDaVvH+9XvzUoEUbHi9_6u+5PcVGVDAmre6Qd0WeQ@mail.gmail.com>
From:   Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri, 14 Jun 2019 08:23:51 -0500
Message-ID: <CAErSpo6qaMc1O7vgcuCwdDbe4QBcOw83wd7PbuUVS+7GDPgK9Q@mail.gmail.com>
Subject: Re: [PATCH v2] bus: hisi_lpc: Don't use devm_kzalloc() to allocate
 logical PIO range
To:     John Garry <john.garry@huawei.com>
Cc:     xuwei5@huawei.com, linuxarm@huawei.com, arm@kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Joe Perches <joe@perches.com>,
        "zhichang.yuan" <zhichang.yuan02@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

[try another address for Zhichang; first one bounced]

On Fri, Jun 14, 2019 at 8:20 AM Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> [+cc Zhichang, logic_pio author]
>
> On Fri, Jun 14, 2019 at 5:26 AM John Garry <john.garry@huawei.com> wrote:
> >
> > If, after registering a logical PIO range, the driver probe later fails,
> > the logical PIO range memory will be released automatically.
> >
> > This causes an issue, in that the logical PIO range is not unregistered
> > (that is not supported) and the released range memory may be later
> > referenced
> >
> > Allocate the logical PIO range with kzalloc() to avoid this potential
> > issue.
> >
> > Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> > Signed-off-by: John Garry <john.garry@huawei.com>
> > ---
> >
> > Change to v1:
> > - add comment, as advised by Joe Perches
> >
> > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> > index 19d7b6ff2f17..5f0130a693fe 100644
> > --- a/drivers/bus/hisi_lpc.c
> > +++ b/drivers/bus/hisi_lpc.c
> > @@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >         if (IS_ERR(lpcdev->membase))
> >                 return PTR_ERR(lpcdev->membase);
> >
> > -       range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> > +       /* Logical PIO may reference 'range' memory even if the probe fails */
> > +       range = kzalloc(sizeof(*range), GFP_KERNEL);
>
> This doesn't feel like the correct way to fix this.  If the probe
> fails, everything done by the probe should be undone, including the
> allocation and registration of "range".  I don't know what the best
> mechanism is, but I'm skeptical about this one.
>
> >         if (!range)
> >                 return -ENOMEM;
> >
> > @@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >         ret = logic_pio_register_range(range);
> >         if (ret) {
> >                 dev_err(dev, "register IO range failed (%d)!\n", ret);
> > +               kfree(range);
> >                 return ret;
> >         }
> >         lpcdev->io_host = range;
> > --
> > 2.17.1
> >