Hi Will,
On Wed, Oct 02, 2019 at 02:19:29PM +0100, Will Deacon wrote:
> On Wed, Oct 02, 2019 at 04:09:13PM +0300, Laurent Pinchart wrote:
> > Thank you for the patch.
>
> And thank you for the quick response.
>
> > On Wed, Oct 02, 2019 at 12:27:53PM +0100, Will Deacon wrote:
> > > I don't have a way to reproduce the original issue, so this change is
> > > based purely on inspection. Considering I'm not familiar with USB nor
> > > UVC, I may well have missed something!
> >
> > I may also be missing something, I haven't touched this code for a long
> > time :-)
>
> Actually, that is pretty helpful because it will make backporting easier
> if we get to that :)
>
> > uvc_scan_chain_entity(), at the end of the function, adds the entity to
> > the list of entities in the chain with
> >
> > list_add_tail(&entity->chain, &chain->entities);
>
> Yes.
>
> > uvc_scan_chain_forward() is then called (from uvc_scan_chain()), and
> > iterates over all entities connected to the entity being scanned.
> >
> > while (1) {
> > forward = uvc_entity_by_reference(chain->dev, entity->id,
> > forward);
>
> Yes.
>
> > At this point forward may be equal to entity, if entity references
> > itself.
>
> Correct -- that's indicative of a malformed entity which we want to reject,
> right?
Right. We can reject the whole chain in that case. There's one case
where we still want to succeed though, which is handled by
uvc_scan_fallback().
Looking at the code, uvc_scan_device() does
if (uvc_scan_chain(chain, term) < 0) {
kfree(chain);
continue;
}
It seems that's missing removal of all entities that would have been
successfully added to the chain. This prevents, I think,
uvc_scan_fallback() from working properly in some cases.
> > if (forward == NULL)
> > break;
> > if (forward == prev)
> > continue;
> > if (forward->chain.next || forward->chain.prev) {
> > uvc_trace(UVC_TRACE_DESCR, "Found reference to "
> > "entity %d already in chain.\n", forward->id);
> > return -EINVAL;
> > }
> >
> > But then this check should trigger, as forward == entity and entity is
> > in the chain's list of entities.
>
> Right, but this code is added by my patch, no? In mainline, the code only
> has the first two checks, which both end up comparing against NULL the first
> time around:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_driver.c#n1489
>
> Or are you referring to somewhere else?
Oops. This is embarassing... :-) You're of course right. The second hunk
seems fine too, even if I would have preferred centralising the check in
a single place. That should be possible, but it would involve
refactoring that isn't worth it at the moment.
--
Regards,
Laurent Pinchart
