Split out _usb_eth_start() from _usb_eth_init() and usb_eth_stop() from _usb_eth_halt(). Now _usb_eth_init() only initialises and registers the gadget device, which _usb_eth_halt() reverses, and together are used for probing and removing the device. The _usb_eth_start() and _usb_eth_stop() functions connect and disconnect the gadget as expected by the start()/stop() callbacks.

Previously the gadget device was probed on every start() and removed on every stop(), which is inconsistent with other DM_ETH drivers. For non-DM gadget drivers the old behaviour has been retained.

Signed-off-by: Niel Fourie lusus@denx.de Cc: Marek Vasut marex@denx.de Cc: Lukasz Majewski lukma@denx.de Cc: Ramon Fried rfried.dev@gmail.com --- drivers/usb/gadget/ether.c | 58 +++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 43aec7ffa70..a75b4eeb5bb 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -2305,15 +2305,11 @@ fail:

/*-------------------------------------------------------------------------*/ static void _usb_eth_halt(struct ether_priv *priv); +static void _usb_eth_stop(struct ether_priv *priv);

static int _usb_eth_init(struct ether_priv *priv) { - struct eth_dev *dev = &priv->ethdev; - struct usb_gadget *gadget; - unsigned long ts; int ret; - unsigned long timeout = USB_CONNECT_TIMEOUT; - ret = usb_gadget_initialize(0); if (ret) return ret; @@ -2353,14 +2349,26 @@ static int _usb_eth_init(struct ether_priv *priv) priv->eth_driver.resume = eth_resume; if (usb_gadget_register_driver(&priv->eth_driver) < 0) goto fail; + return 0; +fail: + _usb_eth_halt(priv); + return -1; +}

- dev->network_started = 0; +static int _usb_eth_start(struct ether_priv *priv) +{ + unsigned long ts; + unsigned long timeout = USB_CONNECT_TIMEOUT; + struct eth_dev *dev = &priv->ethdev; + + if (!dev->gadget) + return -1;

+ dev->network_started = 0; packet_received = 0; packet_sent = 0;

- gadget = dev->gadget; - usb_gadget_connect(gadget); + usb_gadget_connect(dev->gadget);

if (env_get("cdc_connect_timeout")) timeout = dectoul(env_get("cdc_connect_timeout"), NULL) * CONFIG_SYS_HZ; @@ -2378,6 +2386,7 @@ static int _usb_eth_init(struct ether_priv *priv) rx_submit(dev, dev->rx_req, 0); return 0; fail: + _usb_eth_stop(priv); _usb_eth_halt(priv); return -1; } @@ -2457,11 +2466,10 @@ static int _usb_eth_recv(struct ether_priv *priv) return 0; }

-static void _usb_eth_halt(struct ether_priv *priv) +static void _usb_eth_stop(struct ether_priv *priv) { struct eth_dev *dev = &priv->ethdev;

- /* If the gadget not registered, simple return */ if (!dev->gadget) return;

@@ -2485,6 +2493,14 @@ static void _usb_eth_halt(struct ether_priv *priv) usb_gadget_handle_interrupts(0); dev->network_started = 0; } +} + +static void _usb_eth_halt(struct ether_priv *priv) +{ + struct eth_dev *dev = &priv->ethdev; + + if (!dev->gadget) + return;

usb_gadget_unregister_driver(&priv->eth_driver); usb_gadget_release(0); @@ -2493,9 +2509,13 @@ static void _usb_eth_halt(struct ether_priv *priv) #ifndef CONFIG_DM_ETH static int usb_eth_init(struct eth_device *netdev, struct bd_info *bd) { + int ret; struct ether_priv *priv = (struct ether_priv *)netdev->priv;

- return _usb_eth_init(priv); + ret = _usb_eth_init(priv); + if (!ret) + ret = _usb_eth_start(priv); + return ret; }

static int usb_eth_send(struct eth_device *netdev, void *packet, int length) @@ -2536,6 +2556,7 @@ void usb_eth_halt(struct eth_device *netdev) { struct ether_priv *priv = (struct ether_priv *)netdev->priv;

+ _usb_eth_stop(priv); _usb_eth_halt(priv); }

@@ -2559,7 +2580,7 @@ static int usb_eth_start(struct udevice *dev) { struct ether_priv *priv = dev_get_priv(dev);

- return _usb_eth_init(priv); + return _usb_eth_start(priv); }

static int usb_eth_send(struct udevice *dev, void *packet, int length) @@ -2609,7 +2630,7 @@ static void usb_eth_stop(struct udevice *dev) { struct ether_priv *priv = dev_get_priv(dev);

- _usb_eth_halt(priv); + _usb_eth_stop(priv); }

static int usb_eth_probe(struct udevice *dev) @@ -2623,6 +2644,16 @@ static int usb_eth_probe(struct udevice *dev) get_ether_addr(CONFIG_USBNET_DEV_ADDR, pdata->enetaddr); eth_env_set_enetaddr("usbnet_devaddr", pdata->enetaddr);

+ return _usb_eth_init(priv); + + return 0; +} + +static int usb_eth_remove(struct udevice *dev) +{ + struct ether_priv *priv = dev_get_priv(dev); + + _usb_eth_halt(priv); return 0; }

@@ -2658,6 +2689,7 @@ U_BOOT_DRIVER(eth_usb) = { .name = "usb_ether", .id = UCLASS_ETH, .probe = usb_eth_probe, + .remove = usb_eth_remove, .ops = &usb_eth_ops, .priv_auto = sizeof(struct ether_priv), .plat_auto = sizeof(struct eth_pdata),

On 12/12/22 16:29, Niel Fourie wrote:

In eth_halt(), reread and revalidate priv after calling stop(), as it may have been freed, leaving a dangling pointer.

In the ethernet gadget implementation, the gadget device gets probed during start() and removed during stop(), which includes freeing `uclass_priv_` to which `priv` is pointing. Writing to `priv` after stop() may corrupt the `fd` member of `struct malloc_chunk`, which represents the freed block, and could cause hard-to-debug crashes on subsequent calls to malloc()/free().

Signed-off-by: Niel Fourie lusus@denx.de Cc: Ramon Fried rfried.dev@gmail.com Cc: Marek Vasut marex@denx.de Cc: Lukasz Majewski lukma@denx.de

---

Changes for v2:

• Revalidate priv instead of changing state before stop()
• Added explanational comment

This patch my be dropped if the patch which addresses the root cause ("usb: gadget: ether: split start/stop from init/halt") is accepted.

net/eth-uclass.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c index f41da4b37b3..7d5783b5cab 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -341,8 +341,11 @@ void eth_halt(void) priv = dev_get_uclass_priv(current); if (!priv || !priv->running) return;

This shouldn't be here.

eth_get_ops(current)->stop(current);

• /* Ethernet gadget frees priv during stop, workaround until fixed... */
• priv = dev_get_uclass_priv(current);
• if (!priv || !priv->running)

• return;
  

  priv->state = ETH_STATE_PASSIVE; priv->running = false; }

My understanding of discussion in [PATCH] usb: gadget: ether: split start/stop from init/halt is that this patch is not needed with the aforementioned one applied.