Hi Laurent,
Sorry for the delay, I got tied up with other patches.
On Wed, Oct 02, 2019 at 09:56:04PM +0300, Laurent Pinchart wrote:
On Wed, Oct 02, 2019 at 02:19:29PM +0100, Will Deacon wrote:
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.
I started trying to hack something up here, but I'm actually not sure there's anything to do!
I agree that 'uvc_scan_chain()' can fail after adding entities to the chain, however, 'uvc_scan_fallback()' allocates a new chain and calls only 'uvc_scan_chain_entity()' to add entities to it. This doesn't fail on pre-existing 'list_head' structures, so the dangling pointers shouldn't pose a problem there. My patch only adds the checks to 'uvc_scan_chain_forward()' and 'uvc_scan_chain_backward()', neither of which are invoked on the fallback path.
The fallback also seems like a best-effort thing, since it isn't even invoked if we managed to initialise *any* chains successfully.
Does that make sense, or did you have another failure case in mind?
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.
Agreed, thanks.
Will