[PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Zheyu Ma]

The driver should release resources in reverse order, i.e., the
resources requested first should be released last, and the driver
should adjust the order of error handling code by this rule.

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++-----------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2100.c b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
index 2ace2b27ecad..b10d10660eb8 100644
--- a/drivers/net/wireless/intel/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
@@ -6166,7 +6166,7 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
 		printk(KERN_WARNING DRV_NAME
 		       "Error calling ioremap.\n");
 		err = -EIO;
-		goto fail;
+		goto out;
 	}
 
 	/* allocate and initialize our net_device */
@@ -6175,36 +6175,33 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
 		printk(KERN_WARNING DRV_NAME
 		       "Error calling ipw2100_alloc_device.\n");
 		err = -ENOMEM;
-		goto fail;
+		goto fail_iounmap;
 	}
 
+	priv = libipw_priv(dev);
+	pci_set_master(pci_dev);
+	pci_set_drvdata(pci_dev, priv);
+
 	/* set up PCI mappings for device */
 	err = pci_enable_device(pci_dev);
 	if (err) {
 		printk(KERN_WARNING DRV_NAME
 		       "Error calling pci_enable_device.\n");
-		return err;
+		goto fail_dev;
 	}
 
-	priv = libipw_priv(dev);
-
-	pci_set_master(pci_dev);
-	pci_set_drvdata(pci_dev, priv);
-
 	err = dma_set_mask(&pci_dev->dev, DMA_BIT_MASK(32));
 	if (err) {
 		printk(KERN_WARNING DRV_NAME
 		       "Error calling pci_set_dma_mask.\n");
-		pci_disable_device(pci_dev);
-		return err;
+		goto fail_disable;
 	}
 
 	err = pci_request_regions(pci_dev, DRV_NAME);
 	if (err) {
 		printk(KERN_WARNING DRV_NAME
 		       "Error calling pci_request_regions.\n");
-		pci_disable_device(pci_dev);
-		return err;
+		goto fail_disable;
 	}
 
 	/* We disable the RETRY_TIMEOUT register (0x41) to keep
@@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
 out:
 	return err;
 
-      fail_unlock:
+fail_unlock:
 	mutex_unlock(&priv->action_mutex);
-      fail:
+fail:
+	pci_release_regions(pci_dev);
+fail_disable:
+	pci_disable_device(pci_dev);
+fail_dev:
 	if (dev) {
 		if (registered >= 2)
 			unregister_netdev(dev);
@@ -6334,11 +6335,8 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
 
 		free_libipw(dev, 0);
 	}
-
+fail_iounmap:
 	pci_iounmap(pci_dev, ioaddr);
-
-	pci_release_regions(pci_dev);
-	pci_disable_device(pci_dev);
 	goto out;
 }
 
-- 
2.25.1

Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Kalle Valo]

Zheyu Ma <zheyuma97@gmail.com> wrote:

> The driver should release resources in reverse order, i.e., the
> resources requested first should be released last, and the driver
> should adjust the order of error handling code by this rule.
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>

Can someone review this, please?

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220409062449.3752252-1-zheyuma97@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Stanislav Yakovlev]

On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> The driver should release resources in reverse order, i.e., the
> resources requested first should be released last, and the driver
> should adjust the order of error handling code by this rule.
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++-----------
>  1 file changed, 16 insertions(+), 18 deletions(-)
>
[Skipped]

> @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
>  out:
>         return err;
>
> -      fail_unlock:
> +fail_unlock:
>         mutex_unlock(&priv->action_mutex);
> -      fail:
> +fail:
> +       pci_release_regions(pci_dev);
> +fail_disable:
> +       pci_disable_device(pci_dev);
We can't move these functions before the following block.

> +fail_dev:
>         if (dev) {
>                 if (registered >= 2)
>                         unregister_netdev(dev);
This block continues with a function call to ipw2100_hw_stop_adapter
which assumes that device is still accessible via pci bus.

> @@ -6334,11 +6335,8 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
>
>                 free_libipw(dev, 0);
>         }
> -
> +fail_iounmap:
>         pci_iounmap(pci_dev, ioaddr);
> -
> -       pci_release_regions(pci_dev);
> -       pci_disable_device(pci_dev);
>         goto out;
>  }
>
> --
> 2.25.1
>

Stanislav.

Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Zheyu Ma]

On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev
<stas.yakovlev@gmail.com> wrote:
>
> On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote:
> >
> > The driver should release resources in reverse order, i.e., the
> > resources requested first should be released last, and the driver
> > should adjust the order of error handling code by this rule.
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >  drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++-----------
> >  1 file changed, 16 insertions(+), 18 deletions(-)
> >
> [Skipped]
>
> > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev,
> >  out:
> >         return err;
> >
> > -      fail_unlock:
> > +fail_unlock:
> >         mutex_unlock(&priv->action_mutex);
> > -      fail:
> > +fail:
> > +       pci_release_regions(pci_dev);
> > +fail_disable:
> > +       pci_disable_device(pci_dev);
> We can't move these functions before the following block.
>
> > +fail_dev:
> >         if (dev) {
> >                 if (registered >= 2)
> >                         unregister_netdev(dev);
> This block continues with a function call to ipw2100_hw_stop_adapter
> which assumes that device is still accessible via pci bus.

Thanks for your reminder, but the existing error handling does need to
be revised, I got the following warning when the probing fails at
pci_resource_flags():

[   20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44 pci_iounmap+0x40/0x50
[   20.716583] RIP: 0010:pci_iounmap+0x40/0x50
[   20.726342]  <TASK>
[   20.726550]  ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100]

Since I am not familiar with the ipw2100, could someone give me some
advice to fix this.

Thanks,
Zheyu Ma

Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Stanislav Yakovlev]

Hi Zheyu,

On 18/04/2022, Zheyu Ma <zheyuma97@gmail.com> wrote:
> On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev
> <stas.yakovlev@gmail.com> wrote:
>>
>> On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote:
>> >
>> > The driver should release resources in reverse order, i.e., the
>> > resources requested first should be released last, and the driver
>> > should adjust the order of error handling code by this rule.
>> >
>> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
>> > ---
>> >  drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++-----------
>> >  1 file changed, 16 insertions(+), 18 deletions(-)
>> >
>> [Skipped]
>>
>> > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev
>> > *pci_dev,
>> >  out:
>> >         return err;
>> >
>> > -      fail_unlock:
>> > +fail_unlock:
>> >         mutex_unlock(&priv->action_mutex);
>> > -      fail:
>> > +fail:
>> > +       pci_release_regions(pci_dev);
>> > +fail_disable:
>> > +       pci_disable_device(pci_dev);
>> We can't move these functions before the following block.
>>
>> > +fail_dev:
>> >         if (dev) {
>> >                 if (registered >= 2)
>> >                         unregister_netdev(dev);
>> This block continues with a function call to ipw2100_hw_stop_adapter
>> which assumes that device is still accessible via pci bus.
>
> Thanks for your reminder, but the existing error handling does need to
> be revised, I got the following warning when the probing fails at
> pci_resource_flags():
>
> [   20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44
> pci_iounmap+0x40/0x50
> [   20.716583] RIP: 0010:pci_iounmap+0x40/0x50
> [   20.726342]  <TASK>
> [   20.726550]  ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100]
>
> Since I am not familiar with the ipw2100, could someone give me some
> advice to fix this.

Could you please rebuild the kernel with IPW2100_DEBUG config option
enabled, rerun the test and post your results here? Also, please post
the output of "lspci -v" here.

Stanislav.

Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()

[Zheyu Ma]

On Sat, Apr 23, 2022 at 3:25 AM Stanislav Yakovlev
<stas.yakovlev@gmail.com> wrote:
>
> Hi Zheyu,
>
> On 18/04/2022, Zheyu Ma <zheyuma97@gmail.com> wrote:
> > On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev
> > <stas.yakovlev@gmail.com> wrote:
> >>
> >> On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote:
> >> >
> >> > The driver should release resources in reverse order, i.e., the
> >> > resources requested first should be released last, and the driver
> >> > should adjust the order of error handling code by this rule.
> >> >
> >> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> >> > ---
> >> >  drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++-----------
> >> >  1 file changed, 16 insertions(+), 18 deletions(-)
> >> >
> >> [Skipped]
> >>
> >> > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev
> >> > *pci_dev,
> >> >  out:
> >> >         return err;
> >> >
> >> > -      fail_unlock:
> >> > +fail_unlock:
> >> >         mutex_unlock(&priv->action_mutex);
> >> > -      fail:
> >> > +fail:
> >> > +       pci_release_regions(pci_dev);
> >> > +fail_disable:
> >> > +       pci_disable_device(pci_dev);
> >> We can't move these functions before the following block.
> >>
> >> > +fail_dev:
> >> >         if (dev) {
> >> >                 if (registered >= 2)
> >> >                         unregister_netdev(dev);
> >> This block continues with a function call to ipw2100_hw_stop_adapter
> >> which assumes that device is still accessible via pci bus.
> >
> > Thanks for your reminder, but the existing error handling does need to
> > be revised, I got the following warning when the probing fails at
> > pci_resource_flags():
> >
> > [   20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44
> > pci_iounmap+0x40/0x50
> > [   20.716583] RIP: 0010:pci_iounmap+0x40/0x50
> > [   20.726342]  <TASK>
> > [   20.726550]  ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100]
> >
> > Since I am not familiar with the ipw2100, could someone give me some
> > advice to fix this.
>
> Could you please rebuild the kernel with IPW2100_DEBUG config option
> enabled, rerun the test and post your results here? Also, please post
> the output of "lspci -v" here.

Sorry for the late response.
I have rebuilt the kernel with IPW2100_DEBUG, and got the following result:

[   29.469624] libipw: 802.11 data/management/control stack, git-1.1.13
[   29.470034] libipw: Copyright (C) 2004-2005 Intel Corporation
<jketreno@linux.intel.com>
[   29.477455] ipw2100: Intel(R) PRO/Wireless 2100 Network Driver, git-1.2.2
[   29.477833] ipw2100: Copyright(c) 2003-2006 Intel Corporation
[   29.478197] ipw2100Error calling ioremap.
[   29.478398] ------------[ cut here ]------------
[   29.478630] Bad IO access at port 0x0 ()
[   29.478834] WARNING: CPU: 0 PID: 304 at lib/iomap.c:44 pci_iounmap+0x40/0x50
[   29.481116] RIP: 0010:pci_iounmap+0x40/0x50
[   29.485282] Call Trace:
[   29.485407]  <TASK>
[   29.485514]  ipw2100_pci_init_one+0x192/0x20c0 [ipw2100]
[   29.487496]  local_pci_probe+0x13f/0x200

Actually, I made a special virtual device to test this driver, not
real hardware, so the "lspci" results are irrelevant here.
I injected a software fault in the pci_iomap() function to force the
driver to fail here, and then the driver goto "fail" tag, and under
the "fail" tag the pci_iounmap() is called, resulting in a crash.
In fact such error handling is incorrect and we should not release
resources that have not been requested yet.

Thanks,
Zheyu Ma