Re: WARNING in usb_composite_setup_continue

[Peter Chen <peter.chen@nxp.com>]

On 20-11-12 10:59:05, Alan Stern wrote:
> On Thu, Nov 12, 2020 at 09:01:11AM +0000, Peter Chen wrote:
> > On 20-11-11 14:47:10, Alan Stern wrote:
> > > There's still a possible race, although it's a different one: The 
> > > gadget's delayed status reply can race with the host timing out and 
> > > sending a new SETUP packet:
> > > 
> > > 	Host sends SETUP packet A
> > > 
> > > 	Function receives A and decides
> > > 	to send a delayed status reply
> > > 
> > > 					Function thread starts to
> > > 					process packet A
> > > 
> > > 	Host times out waiting for A status
> > > 
> > > 	Host sends new SETUP packet B
> > > 
> > > 	Composite core receives packet B
> > > 	and resets cdev->delayed_status
> > 
> > resets cdev->delayed_status? Where to do that?
> 
> Sorry, for some reason I though I read a line that set delayed_status to 
> 0 in composite_setup().  I must have been fantasizing...
> 
> >  Even the host re-try the
> > control transfer, it should send the same control request, eg,
> > SET_CONFIGURATION, and increase cdev->delayed_status. There is a description
> > for possible host sends next control request before receiving status
> > packet at USB 2.0 Spec, CH 5.5.5 Control Transfer Data Sequences
> > 
> > 	  If a Setup transaction is received by an endpoint before a previously
> > 	  initiated control transfer is completed,
> > 	  the device must abort the current transfer/operation and
> > 	  handle the new control Setup transaction. A Setup
> > 	  transaction should not normally be sent before the completion
> > 	  of a previous control transfer. However, if a
> > 	  transfer is aborted, for example, due to errors on the bus,
> > 	  the host can send the next Setup transaction
> > 	  prematurely from the endpoint’s perspective.
> 
> Yes.  The composite core does not abort the current transfer/operation 
> when a new Setup transaction occurs.  But it should -- and it should set 
> delayed_status back to 0.  (Maybe this is what I was fantasizing.)
> 
> > > 					Function thread finishes and calls
> > > 					usb_composite_setup_continue()
> > > 
> > > 					The composite core sends a status
> > > 					reply for packet A, not packet B
> > > 
> > > 	Host receives status for A but thinks
> > > 	it is the status for B!
> > > 
> > > 					Function thread processes packet B
> > > 
> > > 					Function thread finishes and calls
> > > 					usb_composite_setup_continue()
> > > 
> > > 					The composite core sees
> > > 					cdev->delayed_status == 0 and WARNs.
> > > 
> > > At the moment I don't see how to prevent this sort of race from 
> > > happening.  We may need to change the API, giving the composite core a 
> > > way to match up calls to usb_composite_setup_continue() with the 
> > > corresponding call to composite_setup().  But even that wouldn't fix 
> > > the entire problem.
> > > 
> > 
> > Hi Alan,
> > 
> > I more think a possible reset or disconnect occurrence between them, and
> > composite_disconnect is called.
> 
> That sounds reasonable.
> 
> This is why I think we will need to change the API.  There has to be a 
> way to tell usb_composite_setup_continue() which SETUP it is being 
> called for.  A new SETUP or a disconnect should invalidate the old 
> SETUP, and then usb_composite_setup_continue() would ignore any calls 
> that were for the old SETUP packet.
> 

I think usb_composite_setup_continue logic has no issue, we only need to
consider if changing WARN to DBG is necessary, FuzzUSB seems to be
trigger panic for WARN.

See below message on its trace log
> Kernel panic - not syncing: panic_on_warn set ...


For new SETUP, current composite layer makes sure the pending request
will not get the status since the stage request is only queued when
cdev->delayed_status is 1. But the UDC driver should make sure no new
request if old request has not finished, consider the corner case that
the new SETUP comes after the pending request calls usb_composite_setup_continue
and queues the status stage, then, the two zero-length packets from device
will be the bus, but host only wants get one. Meanwhile, there is only one
request for composite device (see: usb_composite_dev.req), it means the
composite layer can't handle multiple ep0 request.

For disconnect event, it is an unexpected event between SETUP(DATA) stage
and status stage, and usb_composite_setup_continue just does nothing
since the bus has already not at configured state.

-- 

Thanks,
Peter Chen