[igt-dev] [PATCH i-g-t] lib: Fix device lists not cleaned up sufficiently before rescan

[Zbigniew Kempczyński]

On Thu, Apr 02, 2020 at 08:12:15PM +0200, Janusz Krzysztofik wrote:
> Hi Zbyszek,
> 
> Thanks for review.
> 
> On Thu, 2020-04-02 at 19:23 +0200, Zbigniew Kempczyński wrote:
> > On Fri, Mar 20, 2020 at 04:36:23PM +0100, Janusz Krzysztofik wrote:
> > > Some effort is already taken to clean up previous content of device
> > > lists before forced device rescan is performed but it is not
> > > sufficient.  An attempt to use forced device rescan in a test results
> > > in that test crashing or spinning until being killed by OOM killer.
> > > Fix it.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > > ---
> > >  lib/igt_device_scan.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > index b3f79c81e..db4da30d3 100644
> > > --- a/lib/igt_device_scan.c
> > > +++ b/lib/igt_device_scan.c
> > > @@ -353,14 +353,11 @@ static void set_vendor_device(struct igt_device *dev)
> > >  	dev->device = strndup(pci_id + 5, 4);
> > >  }
> > >  
> > > -/* Allocate arrays for keeping scanned devices */
> > > +/* Initialize lists for keeping scanned devices */
> > >  static bool prepare_scan(void)
> > >  {
> > > -	if (igt_devs.all.prev == NULL || igt_devs.all.next == NULL)
> > > -		IGT_INIT_LIST_HEAD(&igt_devs.all);
> > > -
> > > -	if (igt_devs.filtered.prev == NULL || igt_devs.filtered.next == NULL)
> > > -		IGT_INIT_LIST_HEAD(&igt_devs.filtered);
> > > +	IGT_INIT_LIST_HEAD(&igt_devs.all);
> > > +	IGT_INIT_LIST_HEAD(&igt_devs.filtered);
> > 
> > I think above change according to the change below is not required. 
> 
> Indeed, but I can see no use case for conditional initialisation of the
> lists.
> 
> > 
> > >  
> > >  	return true;
> > >  }
> > > @@ -595,8 +592,15 @@ void igt_devices_scan(bool force)
> > >  {
> > >  	if (force && igt_devs.devs_scanned) {
> > >  		struct igt_device *dev, *tmp;
> > > +
> > > +		igt_list_for_each_entry_safe(dev, tmp, &igt_devs.filtered,
> > > +					     link) {
> > > +			igt_list_del(&dev->link);
> > > +			free(dev);
> > > +		}
> > >  		igt_list_for_each_entry_safe(dev, tmp, &igt_devs.all, link) {
> > >  			igt_device_free(dev);
> > > +			igt_list_del(&dev->link);
> 
> Looking at it again, I think we should better move deletion of the
> device from the list up one line so we are still more safe from use
> after free.  Please confirm your R-b still applies in that case.
> 
> Thanks,
> Janusz

Yes, you're right, better is to free the device one line after :)

Yes, you still got my R-B.

--
Zbigniew

> 
> 
> > >  			free(dev);
> > >  		}
> > >  
> > > -- 
> > > 2.21.1
> > > 
> > 
> > Yes, that part is what we need to fix the leak. 
> > 
> > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com> 
>