Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 14 Oct 2020 16:08:46 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Coiby Xu <coiby.xu@gmail.com>
Cc:     devel@driverdev.osuosl.org,
        Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" 
        <GR-Linux-NIC-Dev@marvell.com>,
        Manish Chopra <manishc@marvell.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Shung-Hsi Yu <shung-hsi.yu@suse.com>,
        open list <linux-kernel@vger.kernel.org>,
        Benjamin Poirier <benjamin.poirier@gmail.com>,
        "open list:QLOGIC QLGE 10Gb ETHERNET DRIVER" <netdev@vger.kernel.org>
Subject: Re: [PATCH v2 2/7] staging: qlge: Initialize devlink health dump
 framework
Message-ID: <20201014130846.GU1042@kadam>
References: <20201014104306.63756-1-coiby.xu@gmail.com>
 <20201014104306.63756-3-coiby.xu@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201014104306.63756-3-coiby.xu@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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.

>  
> -	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;

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().

Anyway, let's not introduce new leaks at least.

regards,
dan carpenter