Re: [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup

[Sakari Ailus]

Hi Laurent,

On Thu, May 02, 2024 at 06:56:26PM +0300, Laurent Pinchart wrote:
> Hi Julien,
> 
> On Thu, May 02, 2024 at 05:22:20PM +0200, Julien Massot wrote:
> > Many drivers has
> >   v4l2_async_nf_unregister(&notifier);
> >   v4l2_async_nf_cleanup(&notifier);
> > 
> > Introduce a helper function to call both functions in one line.
> 
> Does this really go in the right direction ? For other objects (video
> devices, media devices, ...), the unregistration should be done at
> .remove() time, and the cleanup at .release() time (the operation called
> when the last reference to the object is released). This is needed to
> ensure proper lifetime management of the objects, and avoid a
> use-after-free for objects that can be reached from userspace.
> 
> It could be argued that the notifier isn't exposed to userspace, but can
> we guarantee that no driver will have a need to access the notifier in a
> code path triggered by a userspace operation ? I think it would be safer
> to adopt the same split for the nofifier unregistration and cleanup. In
> my opinion using the same rule across different APIs also make it easier
> for driver authors and for reviewers to get it right.
> 
> As shown by your series, lots of drivers call v4l2_async_nf_cleanup()
> and .remove() time instead of .release(). That's because most drivers
> get lifetime management wrong and don't even implement .release().
> That's something Sakari is addressing with ongoing work. This patch
> series seems to go in the opposite direction.

This still avoids the driver authors feeling they need to implement wrapper
functions for v4l2_async_nf_{unregister,cleanup}. I'd be in favour merging
this.

I don't see this getting in the way of adding use counts as the code will
need to be changed in any case.

-- 
Regards,

Sakari Ailus