| Message ID | 1608511522-3100-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/hisilicon: Use pcim_enable_device() | expand |
On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote: > Using the managed function simplifies the error handling. After > unloading the driver, the PCI device should now get disabled as > well. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > index 02f3bd1..7159018 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > dev->pdev = pdev; > pci_set_drvdata(pdev, dev); > > - ret = pci_enable_device(pdev); > + ret = pcim_enable_device(pdev); > if (ret) { > drm_err(dev, "failed to enable pci device: %d\n", ret); > goto err_free; > @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > ret = hibmc_load(dev); > if (ret) { > drm_err(dev, "failed to load hibmc: %d\n", ret); > - goto err_disable; > + goto err_free; > } > > ret = drm_dev_register(dev, 0); > @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > > err_unload: > hibmc_unload(dev); > -err_disable: > - pci_disable_device(pdev); > err_free: > drm_dev_put(dev); The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc takes care of that already. I'm kinda suprised you don't have a refcount underrun already - do you test module unload with KASAN enabled? The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > > -- > 2.7.4 >
在 2020/12/22 6:02, Daniel Vetter 写道: > On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote: >> Using the managed function simplifies the error handling. After >> unloading the driver, the PCI device should now get disabled as >> well. >> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >> --- >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> index 02f3bd1..7159018 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >> dev->pdev = pdev; >> pci_set_drvdata(pdev, dev); >> >> - ret = pci_enable_device(pdev); >> + ret = pcim_enable_device(pdev); >> if (ret) { >> drm_err(dev, "failed to enable pci device: %d\n", ret); >> goto err_free; >> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >> ret = hibmc_load(dev); >> if (ret) { >> drm_err(dev, "failed to load hibmc: %d\n", ret); >> - goto err_disable; >> + goto err_free; >> } >> >> ret = drm_dev_register(dev, 0); >> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >> >> err_unload: >> hibmc_unload(dev); >> -err_disable: >> - pci_disable_device(pdev); >> err_free: >> drm_dev_put(dev); > The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc > takes care of that already. I'm kinda suprised you don't have a refcount > underrun already - do you test module unload with KASAN enabled? Thanks for helping to review the code，and kindly giving me advice。 this problem have been fixed。 c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free > The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cheers, Daniel > >> >> -- >> 2.7.4 >>
On Tue, Dec 22, 2020 at 1:38 AM tiantao (H) <tiantao6@huawei.com> wrote: > > > 在 2020/12/22 6:02, Daniel Vetter 写道: > > On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote: > >> Using the managed function simplifies the error handling. After > >> unloading the driver, the PCI device should now get disabled as > >> well. > >> > >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > >> --- > >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > >> index 02f3bd1..7159018 100644 > >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > >> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > >> dev->pdev = pdev; > >> pci_set_drvdata(pdev, dev); > >> > >> - ret = pci_enable_device(pdev); > >> + ret = pcim_enable_device(pdev); > >> if (ret) { > >> drm_err(dev, "failed to enable pci device: %d\n", ret); > >> goto err_free; > >> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > >> ret = hibmc_load(dev); > >> if (ret) { > >> drm_err(dev, "failed to load hibmc: %d\n", ret); > >> - goto err_disable; > >> + goto err_free; > >> } > >> > >> ret = drm_dev_register(dev, 0); > >> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev, > >> > >> err_unload: > >> hibmc_unload(dev); > >> -err_disable: > >> - pci_disable_device(pdev); > >> err_free: > >> drm_dev_put(dev); > > The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc > > takes care of that already. I'm kinda suprised you don't have a refcount > > underrun already - do you test module unload with KASAN enabled? > > Thanks for helping to review the code，and kindly giving me advice。 > > this problem have been fixed。 > > c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free Sorry, I was on an older tree. Note that this fix is incomplete, the drm_dev_put in the error path of hibmc_pci_probe still exists. -Daniel > > > The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cheers, Daniel > > > >> > >> -- > >> 2.7.4 > >> >
在 2020/12/22 16:27, Daniel Vetter 写道: > On Tue, Dec 22, 2020 at 1:38 AM tiantao (H) <tiantao6@huawei.com> wrote: >> >> 在 2020/12/22 6:02, Daniel Vetter 写道: >>> On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote: >>>> Using the managed function simplifies the error handling. After >>>> unloading the driver, the PCI device should now get disabled as >>>> well. >>>> >>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >>>> --- >>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++---- >>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> index 02f3bd1..7159018 100644 >>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >>>> dev->pdev = pdev; >>>> pci_set_drvdata(pdev, dev); >>>> >>>> - ret = pci_enable_device(pdev); >>>> + ret = pcim_enable_device(pdev); >>>> if (ret) { >>>> drm_err(dev, "failed to enable pci device: %d\n", ret); >>>> goto err_free; >>>> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >>>> ret = hibmc_load(dev); >>>> if (ret) { >>>> drm_err(dev, "failed to load hibmc: %d\n", ret); >>>> - goto err_disable; >>>> + goto err_free; >>>> } >>>> >>>> ret = drm_dev_register(dev, 0); >>>> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >>>> >>>> err_unload: >>>> hibmc_unload(dev); >>>> -err_disable: >>>> - pci_disable_device(pdev); >>>> err_free: >>>> drm_dev_put(dev); >>> The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc >>> takes care of that already. I'm kinda suprised you don't have a refcount >>> underrun already - do you test module unload with KASAN enabled? >> Thanks for helping to review the code，and kindly giving me advice。 >> >> this problem have been fixed。 >> >> c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free > Sorry, I was on an older tree. Note that this fix is incomplete, the > drm_dev_put in the error path of hibmc_pci_probe still exists. > -Daniel I also found the problem, has raised the patch in the internal review and testing, to confirm that there is no problem, it will be sent out. Thanks for the reminder. >>> The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> >>> Cheers, Daniel >>> >>>> -- >>>> 2.7.4 >>>> >
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index 02f3bd1..7159018 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, dev->pdev = pdev; pci_set_drvdata(pdev, dev); - ret = pci_enable_device(pdev); + ret = pcim_enable_device(pdev); if (ret) { drm_err(dev, "failed to enable pci device: %d\n", ret); goto err_free; @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, ret = hibmc_load(dev); if (ret) { drm_err(dev, "failed to load hibmc: %d\n", ret); - goto err_disable; + goto err_free; } ret = drm_dev_register(dev, 0); @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev, err_unload: hibmc_unload(dev); -err_disable: - pci_disable_device(pdev); err_free: drm_dev_put(dev);
Using the managed function simplifies the error handling. After unloading the driver, the PCI device should now get disabled as well. Signed-off-by: Tian Tao <tiantao6@hisilicon.com> --- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)