Hi Benjamin,

On Thu, Jun 01, 2023 at 10:03:39AM +0200, Benjamin Gaignard wrote:

Le 31/05/2023 à 14:39, Laurent Pinchart a écrit :

quoted

On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:

quoted

On 5/31/23 10:03, Laurent Pinchart wrote:

quoted

On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:

quoted

On 21/03/2023 11:28, Benjamin Gaignard wrote:

quoted

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.

I'm working in v3 where I'm using Xarray API.

Just to be sure to understand you well:
I can just remove VB2_MAX_FRAME limit without adding a new one ?

As long as the code is protected against overflows (e.g. if it uses
num_buffers + 1 in some calculations and allows num_buffers to be
UINT_MAX, you'll have an issue), it should be fine for the vb2 and V4L2
core. Limiting the number of buffers doesn't really protect against
much.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel