On Thu, May 02, 2024 at 04:01:45PM +0000, Sakari Ailus wrote:
> On Thu, May 02, 2024 at 06:56:26PM +0300, Laurent Pinchart wrote:
> > On Thu, May 02, 2024 at 05:22:20PM +0200, Julien Massot wrote:
> > > Many drivers has
> > > v4l2_async_nf_unregister(¬ifier);
> > > v4l2_async_nf_cleanup(¬ifier);
> > >
> > > 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.
Fixing the lifetime issues would essentially revert 2/2 and move the
v4l2_async_nf_cleanup() call to .remove(). I don't think providing a
helper that forces the cleanup at .remove() time is a good idea, it
gives a false sense of doing things right to drivers. This is the same
reason why devm_kzalloc() is so harmful, it gave the wrong message, and
created (or participated in) all those lifetime issues.
--
Regards,
Laurent Pinchart
