linux-kernel - Re: [PATCH 0/2] Introduce v4l2_async_nf_unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240502160830.GB11443@pendragon.ideasonboard.com>
Date: Thu, 2 May 2024 19:08:30 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Julien Massot <julien.massot@...labora.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Jacopo Mondi <jacopo+renesas@...ndi.org>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	Sylvain Petinot <sylvain.petinot@...s.st.com>,
	Yong Zhi <yong.zhi@...el.com>, Bingbu Cao <bingbu.cao@...el.com>,
	Dan Scally <djrscally@...il.com>,
	Tianshu Qiu <tian.shu.qiu@...el.com>,
	Eugen Hristev <eugen.hristev@...labora.com>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Maxime Ripard <mripard@...nel.org>,
	Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, Robert Foss <rfoss@...nel.org>,
	Todor Tomov <todor.too@...il.com>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konrad.dybcio@...aro.org>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Dafna Hirschfeld <dafna@...tmail.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Alim Akhtar <alim.akhtar@...sung.com>,
	Hugues Fruchet <hugues.fruchet@...s.st.com>,
	Alain Volmat <alain.volmat@...s.st.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Yong Deng <yong.deng@...ewell.com>,
	Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
	Benoit Parrot <bparrot@...com>, Jai Luthra <j-luthra@...com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Michal Simek <michal.simek@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Sowjanya Komatineni <skomatineni@...dia.com>,
	Luca Ceresoli <luca.ceresoli@...tlin.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev,
	linux-arm-msm@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-sunxi@...ts.linux.dev, linux-staging@...ts.linux.dev,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup

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(&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.

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