On 03/03/2023 12:23, Sakari Ailus wrote: > Hi Hans, > > Many thanks for the review. > > On Fri, Mar 03, 2023 at 10:07:38AM +0100, Hans Verkuil wrote: >> Hi Sakari, >> >> On 01/02/2023 22:45, 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. >>> >>> Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use, >>> as device_release() releases the resources before calling the driver's >>> remove function. While further work will be required also on these drivers >>> to safely stop he hardware at unbind time, I don't see a reason not to merge >>> these patches now. >>> >>> Some patches are temporarily reverted in order to make reworks easier, then >>> applied later on. >>> >>> 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. >>> >>> Questions and comments are welcome. >> >> Most of this series looks good. >> >> You can add my: >> >> Acked-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> >> >> for patches 1-17, 19, 20 and 22. > > Thank you! > >> >> As I mentioned in my review of patch 21, I don't think 18 and 21 belong to >> this series. > > Yes, some patches I wrote as part of this should be merged earlier. I think > I'll just pick them to my master branch once we have rc1. > >> >> I am also not keen on patch 25 (and thus 23 and 24 that it needs). Perhaps >> take that out for now for more discussion later? >> >> I would like to see some more drivers to be converted: specifically uvc >> and the test drivers (vidtv, vim2m, vimc, visl, vivid). uvc because it is >> widely used, the test drivers because they function as reference code. > > Sounds reasonable. Uvc especially should benefit from this. The conversion > isn't even difficult at all, but still requires testing. > >> >> Finally, I don't think this series addresses another life-cycle problem: >> unbinding subdevices when they are still being used, either by userspace >> or a bridge driver. > > That is correct. I wanted to address this for the media device first and > tackle other problems once we have these patches in. > >> >> I have patches for that here: >> >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=subdev-kref >> >> I think these are pretty much independent from this patch series, but >> I'll wait with posting them until this is merged. > > Interesting. I thought in practice addressing the problem for sub-devices > would require addressing media device lifetime management first. In > practice most drivers have one big allocation for everything and that can > be released only once all users are gone. > >> >> Background: we have an fpga that implements subdevices and also >> (depending on the configuration) complete v4l2 platform drivers. >> >> When the fpga is unloaded when going to standby, subdevices and/or >> platform drivers just disappear, and without correct life-time management >> you get crashes. Basically exactly the same problem as you have with the >> media device. > > Yes. > > Have you posted the subdev-kref patches to the list yet? > No, I don't believe I did. I've been sitting on them waiting for this series, basically. But we (Cisco) have been using these patches for some time now. But that's on a really old kernel :-( I also need to double-check vimc as well: I have a memory that I had to make changes there, but I will have to retest it. Regards, Hans
