Re: [PATCH] usb: gadget: avoid using gadget after freed

[Felipe Balbi <balbi@kernel.org>]

Hi,

Lianwei Wang <lianwei.wang@gmail.com> writes:

> On Tue, Jun 18, 2019 at 11:21 PM Felipe Balbi <balbi@kernel.org> wrote:
>>
>>
>> Hi,
>>
>> Lianwei Wang <lianwei.wang@gmail.com> writes:
>> >> Lianwei Wang <lianwei.wang@gmail.com> writes:
>> >> > On Mon, Jun 17, 2019 at 5:40 AM Felipe Balbi <balbi@kernel.org> wrote:
>> >> >>
>> >> >> Lianwei Wang <lianwei.wang@gmail.com> writes:
>> >> >>
>> >> >> > The udc and gadget device will be deleted when udc device is
>> >> >> > disconnected and the related function will be unbind with it.
>> >> >> >
>> >> >> > But if the configfs is not deleted, then the function object
>> >> >> > will be kept and the bound status is kept true.
>> >> >> >
>> >> >> > Then after udc device is connected again and a new udc and
>> >> >> > gadget objects will be created and passed to bind interface.
>> >> >> > But because the bound is still true, the new gadget is not
>> >> >> > updated to netdev and a previous freed gadget will be used
>> >> >> > in netdev after bind.
>> >> >> >
>> >> >> > To fix this using after freed issue, always set the gadget
>> >> >> > object to netdev in bind interface.
>> >> >> >
>> >> >> > Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
>> >> >>
>> >> >> I can't actually understand what's the problem here. The gadget is not
>> >> >> deleted when we disconnect the cable.
>> >> >>
>> >> >> --
>> >> >> balbi
>> >> >
>> >> > The issue was observed with a dual-role capable USB controller (e.g. Intel
>> >> > XHCI controller), which has the ability to switch role between host and device
>> >> > mode. The gadget is deleted when we switch role to device mode from host
>> >> > mode. See below log:
>> >> > # echo p > /sys/devices/pci0000:00/0000:00:15.1/intel-cht-otg.0/mux_state #(4.4)
>> >>
>> >> oh, so you're using a modified tree :-) Then we can't really help.
>> >>
>> >> > [   41.170891] intel-cht-otg intel-cht-otg.0: p: set PERIPHERAL mode
>> >> > [   41.171895] dwc3 dwc3.0.auto: DWC3 OTG Notify USB_EVENT_VBUS
>> >> > [   41.187420] dwc3 dwc3.0.auto: dwc3_resume_common
>> >> > [   41.191192] usb 1-1: USB disconnect, device number 3
>> >> > [   41.191284] usb 1-1.1: USB disconnect, device number 4
>> >> > [   41.218958] usb 1-1.5: USB disconnect, device number 5
>> >> > [   41.238117] android_work: sent uevent USB_STATE=CONFIGURED
>> >> > [   41.240572] android_work: sent uevent USB_STATE=DISCONNECTED
>> >>
>> >> What is this android_work. That doesn't exist upstream.
>> >>
>> >> > [   41.263285] platform dabr_udc.0: unregister gadget driver 'configfs-gadget'
>> >> > [   41.263413] configfs-gadget gadget: unbind function 'Function FS
>> >> > Gadget'/ffff8801db049e38
>> >> > [   41.263969] configfs-gadget gadget: unbind function
>> >> > 'cdc_network'/ffff8801d8897400
>> >> > [   41.325943] dabridge 1-1.5:1.0: Port 3 VBUS OFF
>> >> > [   41.720957] dabr_udc deleted
>> >> > [   41.721097] dabridge 1-5 deleted
>> >> >
>> >> > The UDC and gadget will be deleted after switch role to device mode.
>> >> > And they will be
>> >> > created as new object when switching back to host mode. At this time
>> >> > the bind in function
>> >> > driver (e.g. f_ncm) will not set the new gadget.
>> >> >
>> >> > For kernel 4.19+, the role switch command will be:
>> >> >   echo "device" > /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role
>> >> >
>> >> > The latest Intel role switch kernel driver can be found here:
>> >> >   https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/usb/roles/intel-xhci-usb-role-switch.c
>> >>
>> >> Right, please test against v5.2-rc5 and show me the problem on that
>> >> kernel. I can't apply patches for problems that may not even exist in
>> >> upstream, sorry.
>> >>
>> >> --
>> >> balbi
>> >
>> > The issue exist in main line kernel, but I can not test it with
>> > v5.2-rc5 kernel. I tested it with 4.19 kernel,
>>
>> which of the v4.19?
>>
>> > which, for the usb gadget part, has almost the same code as v5.2. It
>> > is 100% reproducible with dual role
>> > USB controller or by removing UDC hardware. Take f_ncm for example,
>> > the use case as follows:
>>
>> Keep in mind that the way android handles dual-role is completely
>> different from what we have upstream.
>>
> Right, Our xchi controller support dual role and normally it works in host mode
> and the other device, e.g. phone can connect to our system as a gadget device.
> It works in the same way as PC.
>
>> > 1. USB controller is in host mode, f_ncm and UDC is configured in configfs.
>> >    - The ncm is instanced and alloced when "functions/ncm.usb0" is
>> > created and it will be freed
>> >       when those files are delted from configfs.
>> >
>> > 2. enable the gadget and bind it to this ncm function.
>> >     - For the first time running, ncm_opts->bound is 0 and
>> > gether_set_gadget is called to set the
>> >       gadget. The bound is set to 1 then.
>> >
>> > 3. If the UDC is disconnected from bus, then the UDC and its gadget is
>>
>> what do you mean by "disconnected from the bus"? Removing the cable
>> (aka, disconnect) will only cause the ->disconnect() callback to be
>> called. It will not result in the UDC being freed. Is this, perhaps,
>> something specific to android?
>>
> Our UDC is removable so we can remove the UDC from our system and the
> udc and gadget will be deleted after that.
>
> And our xhci support dual role mode and we can switch role between host and
> device mode. In host mode, the UDC will be connected to xhci host controller
> and the other device can connect to UDC. In device mode, it will work
> as a device
> and can connect to other host computer.
>
>> > deleted. But because the
>> >     ncm.usb0 is still there, ncm object is not freed and
>> > ncm_opts->bound is still set.
>> >     There are two ways to disconnect the UDC hardware. One is for dual
>> > role host controller by switch
>> >     host controller role to device mode. Another way is by removing
>> > the UDC hardware from bus, both
>> >     will generate an usb device disconnect event to UDC driver to
>> > delete udc and gadget.
>>
>> not true, unless I misunderstand what you mean. Disconnect will generate
>> a disconnect interrupt in most UDCs (except for dummy) and the
>> ->disconnect() callback will be called. Nothing will be freed.
>>
> As I mentioned above, the disconnect means the UDC is disconnected.
>
>> > 4. Now the bound of ncm is still set and gadget is deleted due to udc
>> > disconnected. And if we connect
>> >     the udc device again, then it will create new udc and gadget and
>> > bind to ncm again. But because
>> >     bound is already set, the new gadget is not set to gether
>> > (gether_register_netdev not called).
>> >
>> > Not sure if this is clear to you. Please review the scenario and the patch.
>>
>> This sounds a little like it's android-specific. Is your platform using
>> dwc3? Can you capture tracepoints of the failure? ftrace_dump_on_oops
>> will help getting the actual tracepoints in this case.
>>
>> cheers
>>
> It is not android specific. For the dual role controller, in device
> mode the dwc3 will
> work as gadget device and can connect to PC as device. In host mode, the dwc3
> does not work, instead the removable UDC will be connected to our system as
> gadget device.
>
>> --
>> balbi
>
> And the key point is that if the UDC can be disconnected as deleted?
> The answer is
> yes for below two cases:
>   - The UDC is removable
>   - It is a dual role controller and can switch between host mode and
> device mode. In host
> mode it will work as a xhci host controller. And in device mode it
> works as dwc3 gadget
> device.
>
> In both way we see that the udc is disconnected and deleted in our system.
>
> config USB_ROLES_INTEL_XHCI
>         tristate "Intel XHCI USB Role Switch"
>         depends on ACPI && X86
>         help
>           Driver for the internal USB role switch for switching the USB data
>           lines between the xHCI host controller and the dwc3 gadget controller
>           found on various Intel SoCs.
>
>           To compile the driver as a module, choose M here: the module will
>           be called intel-xhci-usb-role-switch.
>
> The issue should be 100% reproducible with that support Intel XHCI USB
> role switch, or
> the udc can be removabled.

what's specific to your case is *how* you implemented dual-role. None of
that is upstream and I have no means of verifying that what you claim is
true.

Sorry

-- 
balbi