Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	tfiga@chromium.org, m.szyprowski@samsung.com, mchehab@kernel.org,
	ming.qian@nxp.com, shijie.qin@nxp.com, eagle.zhou@nxp.com,
	bin.liu@mediatek.com, matthias.bgg@gmail.com,
	angelogioacchino.delregno@collabora.com,
	tiffany.lin@mediatek.com, andrew-ct.chen@mediatek.com,
	yunfei.dong@mediatek.com, stanimir.k.varbanov@gmail.com,
	quic_vgarodia@quicinc.com, agross@kernel.org,
	andersson@kernel.org, konrad.dybcio@linaro.org,
	ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
	daniel.almeida@collabora.com, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	linux-arm-msm@vger.kernel.org,
	linux-rockchip@lists.infradead.org, kernel@collabora.com
Subject: Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
Date: Wed, 31 May 2023 15:39:45 +0300	[thread overview]
Message-ID: <20230531123945.GF27043@pendragon.ideasonboard.com> (raw)
In-Reply-To: <608ae7d6-3f3b-137d-08d2-d41a240be2c4@xs4all.nl>

On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
> On 5/31/23 10:03, Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> >> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> >>> the number of vb2 buffers store in queue.
> >>>
> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>> ---
> >>>  drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> >>>  include/media/videobuf2-core.h                  | 11 +++++++++--
> >>>  2 files changed, 12 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >>> index ae9d72f4d181..f4da917ccf3f 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >>> @@ -34,6 +34,8 @@
> >>>  static int debug;
> >>>  module_param(debug, int, 0644);
> >>>  
> >>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> >>
> >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> >> the debug param either, it should be added for that as well.
> > 
> > Would this be the right time to consider resource accounting in V4L2 for
> > buffers ? Having a module parameter doesn't sound very useful, an
> > application could easily allocate more buffers by using buffer orphaning
> > (allocating buffers, exporting them as dmabuf objects, and freeing them,
> > which leaves the memory allocated). Repeating allocation cycles up to
> > max_vb_buffer_per_queue will allow allocating an unbounded number of
> > buffers, using all the available system memory. I'd rather not add a
> > module argument that only gives the impression of some kind of safety
> > without actually providing any value.
> 
> Does dmabuf itself provide some accounting mechanism? Just wondering.
> 
> More specific to V4L2: I'm not so sure about this module parameter either.
> It makes sense to have a check somewhere against ridiculous values (i.e.
> allocating MAXINT buffers), but that can be a define as well. But otherwise
> I am fine with allowing applications to allocate buffers until the memory
> is full.
> 
> The question is really: what is this parameter supposed to do? The only
> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
> 
> I prefer that as a define, to be honest.
> 
> I think it is perfectly fine for users to try to request more buffers than
> memory allows. It will just fail in that case, not a problem.
> 
> And if an application is doing silly things like buffer orphaning, then so
> what? Is that any different than allocating memory and not freeing it?
> Eventually it will run out of memory and crash, which is normal.

Linux provides APIs to account for and limit usage of resources,
including memory. A system administrator can prevent rogue processes
from starving system resources. The memory consumed by vb2 buffer isn't
taken into account, making V4L2 essentially unsafe for untrusted
processes.

Now, to be fair, there are many reasons why allowing access to v4L2
devices to untrusted applications is a bad idea, and memory consumption
is likely not even the worst one. Still, is this something we want to
fix, or do we want to consider V4L2 to be priviledged API only ? Right
now we can't do so, but with many Linux systems moving towards pipewire,
we could possibly have a system daemon isolating untrusted applications
from the rest of the system. We may thus not need to fix this in the
V4L2 API.

-- 
Regards,

Laurent Pinchart