Re: [net-next,v24,1/6] eea: introduce PCI framework

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eea: introduce PCI framework

Add basic driver framework for the Alibaba Elastic Ethernet Adapter(EEA).
This commit implements the EEA PCI probe functionality.

> diff --git a/drivers/net/ethernet/alibaba/Kconfig b/drivers/net/ethernet/alibaba/Kconfig
> --- /dev/null
> +++ b/drivers/net/ethernet/alibaba/Kconfig
> @@ -0,0 +1,28 @@
> +#
> +# Alibaba network device configuration
> +#
> +
> +config NET_VENDOR_ALIBABA
> +	bool "Alibaba Devices"
> +	default y
> +	help
> +	  If you have a network (Ethernet) device belonging to this class, say Y.
> +
> +	  Note that the answer to this question doesn't directly affect the
> +	  kernel: saying N will just cause the configurator to skip all
> +	  the questions about Alibaba devices. If you say Y, you will be asked
> +	  for your specific device in the following questions.
> +
> +if NET_VENDOR_ALIBABA
> +
> +config EEA
> +	tristate "Alibaba Elastic Ethernet Adapter support"
> +	depends on PCI_MSI
> +	depends on 64BIT
> +	select PAGE_POOL
> +	help
> +	  This driver supports Alibaba Elastic Ethernet Adapter"
                                                                 ^
This isn't a bug, but the help text ends with an unmatched trailing quote.
This should likely be a period instead.

[ ... ]

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_pci.c b/drivers/net/ethernet/alibaba/eea/eea_pci.c
> --- /dev/null
> +++ b/drivers/net/ethernet/alibaba/eea/eea_pci.c

[ ... ]

> +static int eea_init_device(struct eea_device *edev)
> +{
> +	int err;
> +
> +	err = eea_device_reset(edev);
> +	if (err)
> +		return err;
> +
> +	eea_pci_io_set_status(edev, BIT(0) | BIT(1));
> +
> +	err = eea_negotiate(edev);
> +	if (err)
> +		goto err;
> +
> +	/* do net device probe ... */
> +
> +	return 0;
> +err:
> +	eea_add_status(edev, EEA_S_FAILED);
> +	return err;
> +}
> +
> +static int __eea_pci_probe(struct pci_dev *pci_dev,
> +			   struct eea_pci_device *ep_dev)
> +{
> +	int err;
> +
> +	pci_set_drvdata(pci_dev, ep_dev);
> +
> +	err = eea_pci_setup(pci_dev, ep_dev);
> +	if (err)
> +		return err;
> +
> +	err = eea_init_device(&ep_dev->edev);
> +	if (err)
> +		goto err_pci_rel;

If eea_init_device() fails here, can this create a use-after-free issue?

The function sets pci_set_drvdata() at the beginning, then if
eea_init_device() fails, the error path jumps to err_pci_rel which
calls eea_pci_release_resource() but doesn't clear the driver data.
Control then returns to eea_pci_probe():

eea_pci_probe()->__eea_pci_probe():
    pci_set_drvdata(pci_dev, ep_dev);     // sets driver data
    err = eea_init_device(&ep_dev->edev);
    if (err)
        goto err_pci_rel;                 // jumps on failure

err_pci_rel:
    eea_pci_release_resource(ep_dev);     // doesn't clear driver data
    return err;                           // returns to eea_pci_probe

Back in eea_pci_probe:
    err = __eea_pci_probe(pci_dev, ep_dev);
    if (err)
        kfree(ep_dev);                    // frees memory, driver data
                                          // still points to freed memory

Would it make sense to either move pci_set_drvdata() after successful
initialization, or add pci_set_drvdata(pci_dev, NULL) in the error path?

> +
> +	return 0;
> +
> +err_pci_rel:
> +	eea_pci_release_resource(ep_dev);
> +	return err;
> +}

[ ... ]

> +static int eea_pci_probe(struct pci_dev *pci_dev,
> +			 const struct pci_device_id *id)
> +{
> +	struct eea_pci_device *ep_dev;
> +	struct eea_device *edev;
> +	int err;
> +
> +	ep_dev = kzalloc(sizeof(*ep_dev), GFP_KERNEL);
> +	if (!ep_dev)
> +		return -ENOMEM;
> +
> +	edev = &ep_dev->edev;
> +
> +	edev->ep_dev = ep_dev;
> +	edev->dma_dev = &pci_dev->dev;
> +
> +	ep_dev->pci_dev = pci_dev;
> +
> +	err = __eea_pci_probe(pci_dev, ep_dev);
> +	if (err)
> +		kfree(ep_dev);
> +
> +	return err;
> +}
-- 
pw-bot: cr