Re: [PATCH v2 2/7] staging: qlge: Initialize devlink health dump framework

[Coiby Xu]

On Wed, Oct 14, 2020 at 04:08:46PM +0300, Dan Carpenter wrote:
> On Wed, Oct 14, 2020 at 06:43:01PM +0800, Coiby Xu wrote:
> >  static int qlge_probe(struct pci_dev *pdev,
> >  		      const struct pci_device_id *pci_entry)
> >  {
> >  	struct net_device *ndev = NULL;
> >  	struct qlge_adapter *qdev = NULL;
> > +	struct devlink *devlink;
> >  	static int cards_found;
> >  	int err = 0;
> >
> > -	ndev = alloc_etherdev_mq(sizeof(struct qlge_adapter),
> > +	devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_adapter));
> > +	if (!devlink)
> > +		return -ENOMEM;
> > +
> > +	qdev = devlink_priv(devlink);
> > +
> > +	ndev = alloc_etherdev_mq(sizeof(struct qlge_netdev_priv),
> >  				 min(MAX_CPUS,
> >  				     netif_get_num_default_rss_queues()));
> >  	if (!ndev)
> > -		return -ENOMEM;
> > +		goto devlink_free;
> >
> > -	err = qlge_init_device(pdev, ndev, cards_found);
> > -	if (err < 0) {
> > -		free_netdev(ndev);
> > -		return err;
>
> In the old code, if qlge_init_device() fails then it frees "ndev".
>
> > -	}
> > +	qdev->ndev = ndev;
> > +	err = qlge_init_device(pdev, qdev, cards_found);
> > +	if (err < 0)
> > +		goto devlink_free;
>
> But the patch introduces a new resource leak.

Thank you for spotting this issue!

> > -	qdev = netdev_priv(ndev);
> >  	SET_NETDEV_DEV(ndev, &pdev->dev);
> >  	ndev->hw_features = NETIF_F_SG |
> >  		NETIF_F_IP_CSUM |
> > @@ -4611,8 +4619,14 @@ static int qlge_probe(struct pci_dev *pdev,
> >  		qlge_release_all(pdev);
> >  		pci_disable_device(pdev);
> >  		free_netdev(ndev);
> > -		return err;
> > +		goto devlink_free;
> >  	}
> > +
> > +	err = devlink_register(devlink, &pdev->dev);
> > +	if (err)
> > +		goto devlink_free;
> > +
> > +	qlge_health_create_reporters(qdev);
> >  	/* Start up the timer to trigger EEH if
> >  	 * the bus goes dead
> >  	 */
> > @@ -4623,6 +4637,10 @@ static int qlge_probe(struct pci_dev *pdev,
> >  	atomic_set(&qdev->lb_count, 0);
> >  	cards_found++;
> >  	return 0;
> > +
> > +devlink_free:
> > +	devlink_free(devlink);
> > +	return err;
> >  }
>
> The best way to write error handling code is keep tracke of the most
> recent allocation which was allocated successfully.
>
> 	one = alloc();
> 	if (!one)
> 		return -ENOMEM;  //  <-- nothing allocated successfully
>
> 	two = alloc();
> 	if (!two) {
> 		ret = -ENOMEM;
> 		goto free_one; // <-- one was allocated successfully
>                               // Notice that the label name says what
> 			       // the goto does.
> 	}
>
> 	three = alloc();
> 	if (!three) {
> 		ret = -ENOMEM;
> 		goto free_two; // <-- two allocated, two freed.
> 	}
>
> 	...
>
> 	return 0;
>
> free_two:
> 	free(two);
> free_one:
> 	free(one);
>
> 	return ret;
Thank you for teaching me this pattern!

> In the old code qlge_probe() freed things before returning, and that's
> fine if there is only two allocations in the function but when there are
> three or more allocations, then use gotos to unwind.
>
> Ideally there would be a ql_deinit_device() function to mirror the
> ql_init_device() function.  The ql_init_device() is staging quality
> code with leaks and bad label names.  It should be re-written to free
> things one step at a time instead of calling ql_release_all().
I'll see how I can improve ql_init_device. Thank you for the suggestion!

> Anyway, let's not introduce new leaks at least.
>
> regards,
> dan carpenter

--
Best regards,
Coiby