On Mon, Sep 04, 2023 at 03:14:30PM +0200, megi xff wrote: > > Hi, > > On Fri, Nov 04, 2022 at 02:10:30PM +0100, Sascha Hauer wrote: > > ing-List: linux-kernel@xxxxxxxxxxxxxxx > > > > The UDC is not a suitable parent of the net device as the UDC can > > change or vanish during the lifecycle of the ethernet gadget. This > > can be illustrated with the following: > > > > mkdir -p /sys/kernel/config/usb_gadget/mygadget > > cd /sys/kernel/config/usb_gadget/mygadget > > mkdir -p configs/c.1/strings/0x409 > > echo "C1:Composite Device" > configs/c.1/strings/0x409/configuration > > mkdir -p functions/ecm.usb0 > > ln -s functions/ecm.usb0 configs/c.1/ > > echo "dummy_udc.0" > UDC > > rmmod dummy_hcd > > > > The 'rmmod' removes the UDC from the just created gadget, leaving > > the still existing net device with a no longer existing parent. > > I have an even simpler reproducer on Pinephone Pro/RK3399 SoC. All it takes to > trigger the use after free and kernel panic in my case is to plug in a USB dock > to make DWC3 DRD switch to host mode and then unplug it to make it switch back to > peripheral mode. > > This triggers a call to dwc3_gadget_exit and then to dwc3_gadget_init later on > > https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c#L4546 > > Then symlink to the net device becames broken in sysfs: > > https://megous.com/dl/tmp/3d8061f1749a7b2b.png > > And after this happens, there's a kernel panic when removing the rndis gadget > configuration from configfs: https://paste.mozilla.org/Z5DFP9BV > (and possibly other issues, but the kernel panic is the most noticable :)) > > Applaying this patch makes the issue go away. So there definitely seems to > be some device lifetime issue somewhere in there. Looks like this patch just masks an underlying issue. Eg. with DWC3 as UDC, rndis_bind/rndis_unbind is called multiple times (each time USB role switches from host to peripheral). This is because DWC3 re-creates the gadget device each time. During every call rndis_bind gets a pointer to a newly created gadget, but gether_set_gadget() is only called: if (!rndis_opts->bound) { ... } which only happens on the first call to rndis_bind. On any further calls to rndis_bind(), gether_set_gadget is not called, because rndis_opts->bound is already set and thus netdev private data keep pointing to some old, deleted gadget device. https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/function/f_rndis.c#L704 https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/function/u_ether.c#L890 It looks like this whole code was not really written with assumption that gadget devices can be deleted and re-added like this. Not sure where the bug really is... if f_* drivers should better handle gadget changes, or DWC3 should not re-create the gadget like it does on mode changes, or elsewhere. kind regards, o. > kind regards, > o. > > > Accessing the ethernet device with commands like: > > > > ip --details link show usb0 > > > > will result in a KASAN splat: > > > > ================================================================== > > BUG: KASAN: use-after-free in if_nlmsg_size+0x3e8/0x528 > > Read of size 4 at addr c5c84754 by task ip/357 > > > > CPU: 3 PID: 357 Comm: ip Not tainted 6.1.0-rc3-00013-gd14953726b24-dirty #324 > > Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > unwind_backtrace from show_stack+0x10/0x14 > > show_stack from dump_stack_lvl+0x58/0x70 > > dump_stack_lvl from print_report+0x134/0x4d4 > > print_report from kasan_report+0x78/0x10c > > kasan_report from if_nlmsg_size+0x3e8/0x528 > > if_nlmsg_size from rtnl_getlink+0x2b4/0x4d0 > > rtnl_getlink from rtnetlink_rcv_msg+0x1f4/0x674 > > rtnetlink_rcv_msg from netlink_rcv_skb+0xb4/0x1f8 > > netlink_rcv_skb from netlink_unicast+0x294/0x478 > > netlink_unicast from netlink_sendmsg+0x328/0x640 > > netlink_sendmsg from ____sys_sendmsg+0x2a4/0x3b4 > > ____sys_sendmsg from ___sys_sendmsg+0xc8/0x12c > > ___sys_sendmsg from sys_sendmsg+0xa0/0x120 > > sys_sendmsg from ret_fast_syscall+0x0/0x1c > > > > Solve this by not setting the parent of the ethernet device. > > > > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > --- > > drivers/usb/gadget/function/u_ether.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c > > index e06022873df16..8f12f3f8f6eeb 100644 > > --- a/drivers/usb/gadget/function/u_ether.c > > +++ b/drivers/usb/gadget/function/u_ether.c > > @@ -798,7 +798,6 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g, > > net->max_mtu = GETHER_MAX_MTU_SIZE; > > > > dev->gadget = g; > > - SET_NETDEV_DEV(net, &g->dev); > > SET_NETDEV_DEVTYPE(net, &gadget_type); > > > > status = register_netdev(net); > > @@ -873,8 +872,6 @@ int gether_register_netdev(struct net_device *net) > > struct usb_gadget *g; > > int status; > > > > - if (!net->dev.parent) > > - return -EINVAL; > > dev = netdev_priv(net); > > g = dev->gadget; > > > > @@ -905,7 +902,6 @@ void gether_set_gadget(struct net_device *net, struct usb_gadget *g) > > > > dev = netdev_priv(net); > > dev->gadget = g; > > - SET_NETDEV_DEV(net, &g->dev); > > } > > EXPORT_SYMBOL_GPL(gether_set_gadget); > > > > -- > > 2.30.2 > >
