On Wed, 13 Jul 2011, Sarah Sharp wrote:
> > > + /* Insert the endpoint into the list, largest max packet size first. */
> > > + if (list_empty(&interval_bw->endpoints))
> > > + list_add(&virt_ep->bw_endpoint_list, &interval_bw->endpoints);
> > > + else {
> > > + struct xhci_virt_ep *smaller_ep;
> > > +
> > > + list_for_each(ep_list_entry, &interval_bw->endpoints) {
> > > + smaller_ep = list_entry(ep_list_entry,
> > > + struct xhci_virt_ep, bw_endpoint_list);
> > > + if (ep_bw->max_packet_size >=
> > > + smaller_ep->bw_info.max_packet_size) {
> > > + break;
> > > + }
> > > + }
> > > + /* If we have looped through the list, ep_list_entry will point
> > > + * to the head of the list, and list_add_tail will put the new
> > > + * entry at the end of the list (before the head entry).
> > > + * Otherwise, we'll add this endpoint in the list before the
> > > + * endpoint with the smaller max packet size.
> > > + */
> > > + list_add_tail(&virt_ep->bw_endpoint_list,
> > > + &smaller_ep->bw_endpoint_list);
> > > + }
> >
> > Despite the lengthy comment, the logic is wrong. I'll leave it as an
> > exercise for you to figure out why. Here are two hints:
> >
> > The comment talks about ep_list_entry, but the following line
> > of code uses smaller_ep instead.
>
> Ah, you're right. If we've wrapped around the list, smaller_ep will
> point to the last endpoint in the list, and list_add_tail will add the
> new endpoint in the second-to-last position. Nice catch!
I tend to notice things like that special-case "if" statement, which
ought to be handled with no difficulty by the general-case code that
follows. Some people have a tendency to add extra, unnecessary
special-casing.
> > If the loop used list_for_each_entry() then the initial "if"
> > statement wouldn't be needed.
>
> Really? I always feel a bit clumsy with the list structures, so I may
> be wrong, but it seems like we would potentially be dereferencing memory
> we don't own.
No. But to understand this, you have to be aware of the implementation
of list_for_each_entry().
> If the list is empty, interval_bw->endpoints->next will point to the
> struct list_head in struct xhci_interval_bw. But we would pass
> containerof the type struct xhci_virt_ep, so we'd have a pointer to an
> xhci_virt_ep that would be somewhere far away from the struct
> xhci_interval_bw, since the xhci_virt_ep structure is much bigger. Then
That's right.
> the conditional part of the for loop in list_for_each_entry would
> calculate &smaller_ep->bw_endpoint_list. Won't the kernel complain when
> the bogus address is dereferenced? Or is it just pointer math, so the
> bogus smaller_ep address won't actually be dereferenced?
Bingo! The end of the loop is detected by comparing pointer values,
not by dereferencing anything. Thus the loop would end normally,
before executing the body even once.
On the other hand, even though smaller_ep is itself a bogus address,
&smaller_ep->bw_endpoint_list is a perfectly valid address -- it refers
to the list_head in struct xhci_interval_bw. Thus, the final
list_add_tail() statement will work okay.
Alan Stern
--
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
