Re: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 17, 2013 at 10:15:46AM -0000, David Laight wrote:
> > From: Sarah Sharp
> > On Mon, Dec 16, 2013 at 03:20:28PM -0000, David Laight wrote:
> > > > I know all these difference clearly,  inc_deq() is indeed a common
> > > > function for different rings, but lasr_trb() & last_trb_on_last_seg()
> > > > inside it use different condition to determine the last trb in an
> > > > event ring and an non-event ring; and sorry, i still not find why last
> > > > trb in an event ring skipped by H/W according to the current logic.
> > > >
> > > > Thanks!
> > > >
> > >
> > > Read the specs and the code.
> > 
> > State your objections clearly.
> > 
> > Based on my analysis, this patch will produce correct behavior with the
> > event ring:
> > 
> > http://marc.info/?l=linux-usb&m=138697807827548&w=2
> > 
> > Am I missing something?
> 
> > -/* Is this TRB a link TRB or was the last TRB the last TRB in this event ring
> > - * segment?  I.e. would the updated event TRB pointer step off the end of the
> > - * event seg?
> > +/* Is this TRB a link TRB or was the last TRB in this event ring segment?
> > + * I.e. would the updated event TRB pointer step off the end of the event
> > + * seg?
> > */
> > static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> > struct xhci_segment *seg, union xhci_trb *trb)
> > {
> > 	if (ring == xhci->event_ring)
> > -         return trb == &seg->trbs[TRBS_PER_SEGMENT];
> > +         return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
> > 	else
> > 		return TRB_TYPE_LINK_LE32(trb->link.control);
> > }
> 
> 
> It would be extremely confusing for the above function to behave
> differently for event and other rings.
> Actually last_trb() could just be:
> 	return trb == seg->last_trb;
> provided an appropriate 'last_trb' field was set.
> 
> If we assume that there are no empty ring segments (ie adjacent LINK TRBs),
> and that it is never actually called with a pointer to a link trb.

That second assumption is not true, unfortunately.  inc_deq() is not the
only code that manipulates the ring->dequeue function.  If we issue a
Set TR dequeue command to move the hardware dequeue pointer to a link
TRB (which we can do if the TD to be canceled ended just before the link
TRB), update_ring_for_set_deq_completion() will leave ring->dequeue
pointing to a link TRB.  That means inc_deq() could be called with a
link TRB.  So the code below would have to be changed to avoid updating
num_trbs_free in that case.

> Then the inc_deq() code could just be:
> 
> 	ring->deq_updates++;
> 	ring->num_trbs_free++;
> 
> 	if (last_trb(xhci, ring, ring->deq_seg, ++ring->dequeue)) {
> 		ring->deq_seg = ring->deq_seg->next;
> 		ring->dequeue = ring->deq_seg->trbs;
> 		if (ring->deq_seg == ring->first_seg) /* not sure of name */
> 			ring->cycle_state ^= 1;
> 	}

The cycle state is only changed when the enqueue pointer makes it past
the segment with the toggle bit.  It shouldn't be modified for the
dequeue pointer.

And yes, inc_deq() could do with some simplification.  David, are you
interested in creating a patch to simplify this code?  I haven't gotten
v2 revisions from a couple of patches you previously sent, so I wasn't
sure if you're still interested in creating xHCI patches.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux