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

[Janusz Krzysztofik]

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


> >  			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>