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/driv...
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.