Message-ID: <20230601083438.GE22609@pendragon.ideasonboard.com>
Date:   Thu, 1 Jun 2023 11:34:38 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc:     Hans Verkuil <hverkuil-cisco@...all.nl>, tfiga@...omium.org,
        m.szyprowski@...sung.com, mchehab@...nel.org, ming.qian@....com,
        shijie.qin@....com, eagle.zhou@....com, bin.liu@...iatek.com,
        matthias.bgg@...il.com, angelogioacchino.delregno@...labora.com,
        tiffany.lin@...iatek.com, andrew-ct.chen@...iatek.com,
        yunfei.dong@...iatek.com, stanimir.k.varbanov@...il.com,
        quic_vgarodia@...cinc.com, agross@...nel.org, andersson@...nel.org,
        konrad.dybcio@...aro.org, ezequiel@...guardiasur.com.ar,
        p.zabel@...gutronix.de, daniel.almeida@...labora.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2
 queue buffer storage

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 :
> > 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@...labora.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

Powered by blists - more mailing lists