Omap3-isp isp_remove() access subdev.entity after media_device_cleanup()

Omap3-isp isp_remove() access subdev.entity after media_device_cleanup()

[Shuah Khan]

Hi Sakari,

I am looking at omap3 isp_remove() closely and I think there are a few
issues there that could cause problems during unbind.

isp_remove() tries to do media_entity_cleanup() after it unregisters
media_device

isp_remove() calls isp_unregister_entities() followed by
isp_cleanup_modules() - cleanup routines call media_entity_cleanup()

media_entity_cleanup() accesses csi2a->subdev.entity which should be gone
by now after media_device_unregister(). This is just one example. I think
all of these cleanup routines isp_cleanup_modules() call access subdev.entity.

static void isp_cleanup_modules(struct isp_device *isp)
{
        omap3isp_h3a_aewb_cleanup(isp);
        omap3isp_h3a_af_cleanup(isp);
        omap3isp_hist_cleanup(isp);
        omap3isp_resizer_cleanup(isp);
        omap3isp_preview_cleanup(isp);
        omap3isp_ccdc_cleanup(isp);
        omap3isp_ccp2_cleanup(isp);
        omap3isp_csi2_cleanup(isp);
}

This is all done after media_device_cleanup() which does
ida_destroy(&mdev->entity_internal_idx); and mutex_destroy(&mdev->graph_mutex);

I think there are some paths during unbind - isp_remove() that are unsafe.
I am trying to build https://github.com/gumstix/linux/tree/master now and
if I can get it to boot - I can send you some logs.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com

Re: Omap3-isp isp_remove() access subdev.entity after media_device_cleanup()

[Sakari Ailus]

Hi Shuah,

On Fri, Dec 09, 2016 at 09:52:44AM -0700, Shuah Khan wrote:
> Hi Sakari,
> 
> I am looking at omap3 isp_remove() closely and I think there are a few
> issues there that could cause problems during unbind.
> 
> isp_remove() tries to do media_entity_cleanup() after it unregisters
> media_device
> 
> isp_remove() calls isp_unregister_entities() followed by
> isp_cleanup_modules() - cleanup routines call media_entity_cleanup()
> 
> media_entity_cleanup() accesses csi2a->subdev.entity which should be gone
> by now after media_device_unregister(). This is just one example. I think
> all of these cleanup routines isp_cleanup_modules() call access subdev.entity.
> 
> static void isp_cleanup_modules(struct isp_device *isp)
> {
>         omap3isp_h3a_aewb_cleanup(isp);
>         omap3isp_h3a_af_cleanup(isp);
>         omap3isp_hist_cleanup(isp);
>         omap3isp_resizer_cleanup(isp);
>         omap3isp_preview_cleanup(isp);
>         omap3isp_ccdc_cleanup(isp);
>         omap3isp_ccp2_cleanup(isp);
>         omap3isp_csi2_cleanup(isp);
> }
> 
> This is all done after media_device_cleanup() which does
> ida_destroy(&mdev->entity_internal_idx); and mutex_destroy(&mdev->graph_mutex);

Calling media_entity_cleanup() is not a source of the current problems in
any way. The function is defined in media-entity.h and it does nothing:

static inline void media_entity_cleanup(struct media_entity *entity) {};

We could later discuss when media_entity_cleanup() should be called though.
The existing drivers do call it in their remove() handler.

> I think there are some paths during unbind - isp_remove() that are unsafe.
> I am trying to build https://github.com/gumstix/linux/tree/master now and
> if I can get it to boot - I can send you some logs.

Please do.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: Omap3-isp isp_remove() access subdev.entity after media_device_cleanup()

[Mauro Carvalho Chehab]

Em Mon, 12 Dec 2016 10:03:16 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Shuah,
> 
> On Fri, Dec 09, 2016 at 09:52:44AM -0700, Shuah Khan wrote:
> > Hi Sakari,
> > 
> > I am looking at omap3 isp_remove() closely and I think there are a few
> > issues there that could cause problems during unbind.
> > 
> > isp_remove() tries to do media_entity_cleanup() after it unregisters
> > media_device
> > 
> > isp_remove() calls isp_unregister_entities() followed by
> > isp_cleanup_modules() - cleanup routines call media_entity_cleanup()
> > 
> > media_entity_cleanup() accesses csi2a->subdev.entity which should be gone
> > by now after media_device_unregister(). This is just one example. I think
> > all of these cleanup routines isp_cleanup_modules() call access subdev.entity.
> > 
> > static void isp_cleanup_modules(struct isp_device *isp)
> > {
> >         omap3isp_h3a_aewb_cleanup(isp);
> >         omap3isp_h3a_af_cleanup(isp);
> >         omap3isp_hist_cleanup(isp);
> >         omap3isp_resizer_cleanup(isp);
> >         omap3isp_preview_cleanup(isp);
> >         omap3isp_ccdc_cleanup(isp);
> >         omap3isp_ccp2_cleanup(isp);
> >         omap3isp_csi2_cleanup(isp);
> > }
> > 
> > This is all done after media_device_cleanup() which does
> > ida_destroy(&mdev->entity_internal_idx); and mutex_destroy(&mdev->graph_mutex);  
> 
> Calling media_entity_cleanup() is not a source of the current problems in
> any way. The function is defined in media-entity.h and it does nothing:
> 
> static inline void media_entity_cleanup(struct media_entity *entity) {};

> 
> We could later discuss when media_entity_cleanup() should be called though.
> The existing drivers do call it in their remove() handler.

I kept it per Laurent's request, because he believed that we might
need it on some future, and keeping it would make easier to add usage
for it again, provided that it is called at the right place.

Well, as it is not been called at the right place anyway and, whatever we do
to fix the issues with data lifetime media_entity_cleanup() logic will
be affected, I suggest to just get rid of it.

Regards,
Mauro

Re: Omap3-isp isp_remove() access subdev.entity after media_device_cleanup()

[Shuah Khan]

On 12/12/2016 01:03 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Fri, Dec 09, 2016 at 09:52:44AM -0700, Shuah Khan wrote:
>> Hi Sakari,
>>
>> I am looking at omap3 isp_remove() closely and I think there are a few
>> issues there that could cause problems during unbind.
>>
>> isp_remove() tries to do media_entity_cleanup() after it unregisters
>> media_device
>>
>> isp_remove() calls isp_unregister_entities() followed by
>> isp_cleanup_modules() - cleanup routines call media_entity_cleanup()
>>
>> media_entity_cleanup() accesses csi2a->subdev.entity which should be gone
>> by now after media_device_unregister(). This is just one example. I think
>> all of these cleanup routines isp_cleanup_modules() call access subdev.entity.
>>
>> static void isp_cleanup_modules(struct isp_device *isp)
>> {
>>         omap3isp_h3a_aewb_cleanup(isp);
>>         omap3isp_h3a_af_cleanup(isp);
>>         omap3isp_hist_cleanup(isp);
>>         omap3isp_resizer_cleanup(isp);
>>         omap3isp_preview_cleanup(isp);
>>         omap3isp_ccdc_cleanup(isp);
>>         omap3isp_ccp2_cleanup(isp);
>>         omap3isp_csi2_cleanup(isp);
>> }
>>
>> This is all done after media_device_cleanup() which does
>> ida_destroy(&mdev->entity_internal_idx); and mutex_destroy(&mdev->graph_mutex);
> 
> Calling media_entity_cleanup() is not a source of the current problems in
> any way. The function is defined in media-entity.h and it does nothing:
> 
> static inline void media_entity_cleanup(struct media_entity *entity) {};

Perhaps. Please see what I said about accesses csi2a->subdev.entity which is
gone by now to call media_entity_cleanup(). Even though media_entity_cleanup()
doesn't do anything, just this access could result in problems during unbind.

> 
> We could later discuss when media_entity_cleanup() should be called though.
> The existing drivers do call it in their remove() handler.
> 
>> I think there are some paths during unbind - isp_remove() that are unsafe.
>> I am trying to build https://github.com/gumstix/linux/tree/master now and
>> if I can get it to boot - I can send you some logs.
> 
> Please do.
>