Hi Hans, On Mon, Jun 17, 2024 at 01:55:53PM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > Hi folks, > > > > This is a refresh of my 2016 RFC patchset to start addressing object > > lifetime issues in Media controller. It further allows continuing work to > > address lifetime management of media entities. > > > > The underlying problem is described in detail in v4 of the previous RFC: > > <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@xxxxxxxxxxxxxxxxxxxxxxxxxx/>. > > In brief, there is currently no connection between releasing media device > > (and related) memory and IOCTL calls, meaning that there is a time window > > during which released kernel memory can be accessed, and that access can be > > triggered from the user space. The only reason why this is not a grave > > security issue is that it is not triggerable by the user alone but requires > > unbinding a device. That is still not an excuse for not fixing it. > > > > This set differs from the earlier RFC to address the issue in the > > following respects: > > > > - Make changes for ipu3-cio2 driver, too. > > > > - Continue to provide best effort attempt to keep the window between device > > removal and user space being able to access released memory as small as > > possible. This means the problem won't become worse for drivers for which > > Media device lifetime management has not been implemented. > > > > The latter is achieved by adding a new object, Media devnode compat > > reference, which is allocated, refcounted and eventually released by the > > Media controller framework itself, and where the information on registration > > and open filehandles is maintained. This is only done if the driver does not > > manage the lifetime of the media device itself, i.e. its release operation > > is NULL. > > > > Due to this, Media device file handles will also be introduced by this > > patchset. I thought the first user of this would be Media device events but > > it seems we already need them here. > > > > Some patches are temporarily reverted in order to make reworks easier, > > then applied later on. Others are not re-applied: this is a change of > > direction, not development over those patches. It would be possible to > > squash the reverts into others on the expense of readability, so the > > reverts are maintained for that reason. > > > > I've tested this on ipu3-cio2 with and without the refcounting patch (media: > > ipu3-cio2: Release the cio2 device context by media device callback), > > including failures in a few parts of the driver initialisation process in > > the MC framework. I've also tested the vimc driver. > > You need to convert at least one m2m test driver (vicodec and ideally also > vim2m). M2M device have their own media controller setup, so it is good to > have at least one converted. My earlier objection to convert vim2m to managed media device lifetime was that vim2m was obviously intended to be compiled without MC but it seems since 016baa59bf9f6 CONFIG_MEDIA_CONTROLLER is selected for the driver. Thus the related #ifdefs can be removed, too. I'll add two patches for this, one that can be merged independently for removing the #ifdefs. -- Kind regards, Sakari Ailus
