re: [media] media-entity: protect object creation/removal using spin lock

re: [media] media-entity: protect object creation/removal using spin lock

[Dan Carpenter]

Hello Mauro Carvalho Chehab,

The patch f8fd4c61b5ae: "[media] media-entity: protect object
creation/removal using spin lock" from Dec 9, 2015, leads to the
following static checker warning:

	drivers/media/media-entity.c:781 media_remove_intf_link()
	error: dereferencing freed memory 'link'

drivers/media/media-entity.c
   777  void media_remove_intf_link(struct media_link *link)
   778  {
   779          spin_lock(&link->graph_obj.mdev->lock);
   780          __media_remove_intf_link(link);
   781          spin_unlock(&link->graph_obj.mdev->lock);

Do we need this unlock any more?  Haven't we freed the lock on the
previous line?

   782  }

regards,
dan carpenter

Re: [media] media-entity: protect object creation/removal using spin lock

[Mauro Carvalho Chehab]

Em Mon, 14 Dec 2015 22:50:53 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> Hello Mauro Carvalho Chehab,
> 
> The patch f8fd4c61b5ae: "[media] media-entity: protect object
> creation/removal using spin lock" from Dec 9, 2015, leads to the
> following static checker warning:
> 
> 	drivers/media/media-entity.c:781 media_remove_intf_link()
> 	error: dereferencing freed memory 'link'
> 
> drivers/media/media-entity.c
>    777  void media_remove_intf_link(struct media_link *link)
>    778  {
>    779          spin_lock(&link->graph_obj.mdev->lock);
>    780          __media_remove_intf_link(link);
>    781          spin_unlock(&link->graph_obj.mdev->lock);

Thanks for pointing it!

> 
> Do we need this unlock any more? 

Yes.

> Haven't we freed the lock on the previous line?

No. The lock is at the media_device struct, with is not freed here.

What we actually need is to cache mdev:

	struct media_device *mdev = link->graph_obj.mdev;

	spin_lock(&mdev->lock)
	__media_remove_intf_link(link);
	spin_unlock(&mdev->lock)


Probably the same thing is needed on other similar logic.

I guess gcc optimizer actually does the right thing, but we should
fix it to remove the static analyzer warnings.

Regards,
Mauro

> 
>    782  }
> 
> regards,
> dan carpenter

Re: [media] media-entity: protect object creation/removal using spin lock

[Dan Carpenter]

On Mon, Dec 14, 2015 at 06:00:52PM -0200, Mauro Carvalho Chehab wrote:
> I guess gcc optimizer actually does the right thing, but we should
> fix it to remove the static analyzer warnings.

It probably crashes if you enable poisoning freed memory?  (I haven't
tested).

regards,
dan carpenter

Re: [media] media-entity: protect object creation/removal using spin lock

[Mauro Carvalho Chehab]

Em Mon, 14 Dec 2015 23:04:34 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> On Mon, Dec 14, 2015 at 06:00:52PM -0200, Mauro Carvalho Chehab wrote:
> > I guess gcc optimizer actually does the right thing, but we should
> > fix it to remove the static analyzer warnings.
> 
> It probably crashes if you enable poisoning freed memory?  (I haven't
> tested).

I tested with KASAN: when I made such patch: it worked fine.
I suspect that gcc optimized the code to cache the value. Anyway, 
this should be fixed.

Thanks for pointing the issue!

Regards,
Mauro