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/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.
Agreed, thanks.
Will
