| Message ID | 20230321102855.346732-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
On 3/21/23 13:28, Benjamin Gaignard wrote: > + q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO); > + if (!q->bufs) > + return -ENOMEM; > + Use kcalloc()
On Tue, Mar 21, 2023 at 11:28:51AM +0100, Benjamin Gaignard wrote: > After changing bufs arrays to a dynamic allocated array > VB2_MAX_FRAME doesn't mean anything for videobuf2 core. > Remove it from the core definitions but keep it for drivers internal > needs. This made me think that some drivers could behave really badly if they get more than VB2_MAX_FRAME (or VIDEO_MAX_FRAME) buffers. I certainly see some having fixed-size arrays of exactly that size. Should we have a queue flag that enables more buffers, so it is only available for drivers which can handle them? Best regards, Tomasz > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 2 ++ > drivers/media/platform/amphion/vdec.c | 1 + > .../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c | 2 ++ > drivers/media/platform/qcom/venus/hfi.h | 2 ++ > drivers/media/platform/verisilicon/hantro_hw.h | 2 ++ > drivers/staging/media/ipu3/ipu3-v4l2.c | 2 ++ > include/media/videobuf2-core.h | 1 - > include/media/videobuf2-v4l2.h | 4 ---- > 8 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index f4da917ccf3f..3c6ced360770 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -31,6 +31,8 @@ > > #include <trace/events/vb2.h> > > +#define VB2_MAX_FRAME 32 > + > static int debug; > module_param(debug, int, 0644); > > diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c > index 87f9f8e90ab1..bef0c1b869be 100644 > --- a/drivers/media/platform/amphion/vdec.c > +++ b/drivers/media/platform/amphion/vdec.c > @@ -28,6 +28,7 @@ > > #define VDEC_MIN_BUFFER_CAP 8 > #define VDEC_MIN_BUFFER_OUT 8 > +#define VB2_MAX_FRAME 32 > > struct vdec_fs_info { > char name[8]; > diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > index f5958b6d834a..ba208caf3043 100644 > --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c > @@ -16,6 +16,8 @@ > #include "../vdec_drv_if.h" > #include "../vdec_vpu_if.h" > > +#define VB2_MAX_FRAME 32 > + > /* reset_frame_context defined in VP9 spec */ > #define VP9_RESET_FRAME_CONTEXT_NONE0 0 > #define VP9_RESET_FRAME_CONTEXT_NONE1 1 > diff --git a/drivers/media/platform/qcom/venus/hfi.h b/drivers/media/platform/qcom/venus/hfi.h > index f25d412d6553..bd5ca5a8b945 100644 > --- a/drivers/media/platform/qcom/venus/hfi.h > +++ b/drivers/media/platform/qcom/venus/hfi.h > @@ -10,6 +10,8 @@ > > #include "hfi_helper.h" > > +#define VB2_MAX_FRAME 32 > + > #define VIDC_SESSION_TYPE_VPE 0 > #define VIDC_SESSION_TYPE_ENC 1 > #define VIDC_SESSION_TYPE_DEC 2 > diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h > index e83f0c523a30..9e8faf7ba6fb 100644 > --- a/drivers/media/platform/verisilicon/hantro_hw.h > +++ b/drivers/media/platform/verisilicon/hantro_hw.h > @@ -15,6 +15,8 @@ > #include <media/v4l2-vp9.h> > #include <media/videobuf2-core.h> > > +#define VB2_MAX_FRAME 32 > + > #define DEC_8190_ALIGN_MASK 0x07U > > #define MB_DIM 16 > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c > index e530767e80a5..6627b5c2d4d6 100644 > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c > @@ -10,6 +10,8 @@ > #include "ipu3.h" > #include "ipu3-dmamap.h" > > +#define VB2_MAX_FRAME 32 > + > /******************** v4l2_subdev_ops ********************/ > > #define IPU3_RUNNING_MODE_VIDEO 0 > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index b8b34a993e04..4aa43c5c7c58 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -21,7 +21,6 @@ > #include <media/media-request.h> > #include <media/frame_vector.h> > > -#define VB2_MAX_FRAME (32) > #define VB2_MAX_PLANES (8) > > /** > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h > index 5a845887850b..88a7a565170e 100644 > --- a/include/media/videobuf2-v4l2.h > +++ b/include/media/videobuf2-v4l2.h > @@ -15,10 +15,6 @@ > #include <linux/videodev2.h> > #include <media/videobuf2-core.h> > > -#if VB2_MAX_FRAME != VIDEO_MAX_FRAME > -#error VB2_MAX_FRAME != VIDEO_MAX_FRAME > -#endif > - > #if VB2_MAX_PLANES != VIDEO_MAX_PLANES > #error VB2_MAX_PLANES != VIDEO_MAX_PLANES > #endif > -- > 2.34.1 >
On Fri, May 19, 2023 at 10:19:16AM +0000, Tomasz Figa wrote: > On Tue, Mar 21, 2023 at 11:28:50AM +0100, 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); > > + > > #define dprintk(q, level, fmt, arg...) \ > > do { \ > > if (debug >= level) \ > > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > > struct vb2_buffer *vb; > > int ret; > > > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > > - num_buffers = min_t(unsigned int, num_buffers, > > - VB2_MAX_FRAME - q->num_buffers); > > - > > We should keep the validation here, just using the new module parameter > instead of VB2_MAX_FRAME. Otherwise we let the userspace pass > UINT_MAX to REQBUFS and have the array below exhaust the system memory. > > > for (buffer = 0; buffer < num_buffers; ++buffer) { > > /* Allocate vb2 buffer structures */ > > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > /* > > * Make sure the requested values and current defaults are sane. > > */ > > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > > Similar concern here. > > > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > > /* > > * Set this now to ensure that drivers see the correct q->memory value > > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > bool no_previous_buffers = !q->num_buffers; > > int ret; > > > > - if (q->num_buffers == VB2_MAX_FRAME) { > > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > > - return -ENOBUFS; > > - } > > - > > Ditto. > > > if (no_previous_buffers) { > > if (q->waiting_in_dqbuf && *count) { > > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > return -EINVAL; > > } > > > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > > + num_buffers = *count; > > Ditto. > > > > > if (requested_planes && requested_sizes) { > > num_planes = requested_planes; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 397dbf6e61e1..b8b34a993e04 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -12,6 +12,7 @@ > > #ifndef _MEDIA_VIDEOBUF2_CORE_H > > #define _MEDIA_VIDEOBUF2_CORE_H > > > > +#include <linux/minmax.h> > > #include <linux/mm_types.h> > > #include <linux/mutex.h> > > #include <linux/poll.h> > > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > > struct vb2_threadio_data; > > struct vb2_buffer; > > > > +static size_t max_vb_buffer_per_queue = 1024; > > + > > /** > > * struct vb2_mem_ops - memory handling/memory allocator operations. > > * @alloc: allocate video memory and, optionally, allocator private data, > > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > > > if (vb->index >= q->max_num_bufs) { > > struct vb2_buffer **tmp; > > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > > Should cnt also be size_t to match max_vb_buffer_per_queue? > This could also overflow given q->max_num_bufs big enough, so maybe it > would just be better to rewrite to? > > size_t cnt = (q->max_num_bufs > max_vb_buffer_per_queue / 2) ? > max_vb_buffer_per_queue : q->max_num_bufs * 2; > > Or we could just switch to XArray and it would solve this for us. :) I like using existing libraries instead of reinventing the wheel :-) > > + > > + if (cnt >= q->max_num_bufs) > > + goto realloc_failed; > > > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > > if (!tmp) > > goto realloc_failed; > > > > - q->max_num_bufs *= 2; > > + q->max_num_bufs = cnt; > > q->bufs = tmp; > > } > >
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. Regards, Hans > + > #define dprintk(q, level, fmt, arg...) \ > do { \ > if (debug >= level) \ > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > - num_buffers = min_t(unsigned int, num_buffers, > - VB2_MAX_FRAME - q->num_buffers); > - > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > > - if (q->num_buffers == VB2_MAX_FRAME) { > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > - return -ENOBUFS; > - } > - > if (no_previous_buffers) { > if (q->waiting_in_dqbuf && *count) { > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -EINVAL; > } > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > + num_buffers = *count; > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 397dbf6e61e1..b8b34a993e04 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -12,6 +12,7 @@ > #ifndef _MEDIA_VIDEOBUF2_CORE_H > #define _MEDIA_VIDEOBUF2_CORE_H > > +#include <linux/minmax.h> > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/poll.h> > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > struct vb2_threadio_data; > struct vb2_buffer; > > +static size_t max_vb_buffer_per_queue = 1024; > + > /** > * struct vb2_mem_ops - memory handling/memory allocator operations. > * @alloc: allocate video memory and, optionally, allocator private data, > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > if (vb->index >= q->max_num_bufs) { > struct vb2_buffer **tmp; > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > + > + if (cnt >= q->max_num_bufs) > + goto realloc_failed; > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > if (!tmp) > goto realloc_failed; > > - q->max_num_bufs *= 2; > + q->max_num_bufs = cnt; > q->bufs = tmp; > } >
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. > > + > > #define dprintk(q, level, fmt, arg...) \ > > do { \ > > if (debug >= level) \ > > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > > struct vb2_buffer *vb; > > int ret; > > > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > > - num_buffers = min_t(unsigned int, num_buffers, > > - VB2_MAX_FRAME - q->num_buffers); > > - > > for (buffer = 0; buffer < num_buffers; ++buffer) { > > /* Allocate vb2 buffer structures */ > > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > /* > > * Make sure the requested values and current defaults are sane. > > */ > > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > > /* > > * Set this now to ensure that drivers see the correct q->memory value > > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > bool no_previous_buffers = !q->num_buffers; > > int ret; > > > > - if (q->num_buffers == VB2_MAX_FRAME) { > > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > > - return -ENOBUFS; > > - } > > - > > if (no_previous_buffers) { > > if (q->waiting_in_dqbuf && *count) { > > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > return -EINVAL; > > } > > > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > > + num_buffers = *count; > > > > if (requested_planes && requested_sizes) { > > num_planes = requested_planes; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 397dbf6e61e1..b8b34a993e04 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -12,6 +12,7 @@ > > #ifndef _MEDIA_VIDEOBUF2_CORE_H > > #define _MEDIA_VIDEOBUF2_CORE_H > > > > +#include <linux/minmax.h> > > #include <linux/mm_types.h> > > #include <linux/mutex.h> > > #include <linux/poll.h> > > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > > struct vb2_threadio_data; > > struct vb2_buffer; > > > > +static size_t max_vb_buffer_per_queue = 1024; > > + > > /** > > * struct vb2_mem_ops - memory handling/memory allocator operations. > > * @alloc: allocate video memory and, optionally, allocator private data, > > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > > > if (vb->index >= q->max_num_bufs) { > > struct vb2_buffer **tmp; > > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > > + > > + if (cnt >= q->max_num_bufs) > > + goto realloc_failed; > > > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > > if (!tmp) > > goto realloc_failed; > > > > - q->max_num_bufs *= 2; > > + q->max_num_bufs = cnt; > > q->bufs = tmp; > > } > >
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. Regards, Hans
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.
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@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 ? >
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@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.
On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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. Good point. It's even simpler, just keep opening new vim2m instances and requesting max buffers :). > > > > 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. I agree that proper accounting would be useful, although I wouldn't really make this patch series depend on it, since it's not introducing the loophole in the first place. We had some discussion about this in ChromeOS long ago and we thought it would be really useful for killing browser tabs with big videos, but otherwise using very little regular memory (e.g. via javascript). One challenge with accounting V4L2 allocations is how to count shared DMA-bufs. If one process allocates a V4L2 buffer, exports it to DMA-buf and then sends it to another process that keeps it alive, but frees the V4L2 buffer (and even closes the DMA-buf fd), should that memory be still accounted to it even though it doesn't hold a reference to it anymore? > > 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
Hi Tomasz, On Thu, Jun 08, 2023 at 07:24:29PM +0900, Tomasz Figa wrote: > On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart wrote: > > 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. > > Good point. It's even simpler, just keep opening new vim2m instances > and requesting max buffers :). > > > > > > > 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. > > I agree that proper accounting would be useful, although I wouldn't > really make this patch series depend on it, since it's not introducing > the loophole in the first place. No disagreement here, my concern was about introducing a workaround for the lack of proper memory accounting. I'd like to avoid the workaround, but it doesn't mean memory accounting has to be implement now. > We had some discussion about this in ChromeOS long ago and we thought > it would be really useful for killing browser tabs with big videos, > but otherwise using very little regular memory (e.g. via javascript). > > One challenge with accounting V4L2 allocations is how to count shared > DMA-bufs. If one process allocates a V4L2 buffer, exports it to > DMA-buf and then sends it to another process that keeps it alive, but > frees the V4L2 buffer (and even closes the DMA-buf fd), should that > memory be still accounted to it even though it doesn't hold a > reference to it anymore? I've thought about that too. It's an annoying problem, it should probably be discussed with memory management developers. > > 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.
VP9 dynamic resolution change use case requires to change resolution without doing stream off/on to keep references frames. In consequence the numbers of buffers increase until all 'old' reference frames are deprecated. To make it possible this series remove the 32 buffers limit per queue and introduce DELETE_BUF ioctl to delete buffers from a queue without doing stream off/on sequence. change in version 2: - Use a dynamic array and not a list to keep trace of allocated buffers. Not use IDR interface because it is marked as deprecated in kernel documentation. - Add a module parameter to limit the number of buffer per queue. - Add DELETE_BUF ioctl and m2m helpers. Benjamin Gaignard (8): media: videobuf2: Access vb2_queue bufs array through helper functions media: videobuf2: Make bufs array dynamic allocated media: videobuf2: Add a module param to limit vb2 queue buffer storage media: videobuf2: Stop define VB2_MAX_FRAME as global media: v4l2: Add DELETE_BUF ioctl media: v4l2: Add mem2mem helpers for DELETE_BUF ioctl media: vim2m: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl media: verisilicon: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl .../userspace-api/media/v4l/user-func.rst | 1 + .../media/v4l/vidioc-delete-buf.rst | 51 ++++++ .../media/common/videobuf2/videobuf2-core.c | 168 +++++++++++++----- .../media/common/videobuf2/videobuf2-v4l2.c | 23 ++- drivers/media/platform/amphion/vdec.c | 1 + drivers/media/platform/amphion/vpu_dbg.c | 4 +- .../platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +- .../vcodec/vdec/vdec_vp9_req_lat_if.c | 4 +- drivers/media/platform/qcom/venus/hfi.h | 2 + .../media/platform/verisilicon/hantro_hw.h | 2 + .../media/platform/verisilicon/hantro_v4l2.c | 1 + drivers/media/test-drivers/vim2m.c | 1 + drivers/media/test-drivers/visl/visl-dec.c | 16 +- drivers/media/v4l2-core/v4l2-dev.c | 1 + drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++ drivers/media/v4l2-core/v4l2-mem2mem.c | 20 +++ .../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 2 + include/media/v4l2-ioctl.h | 4 + include/media/v4l2-mem2mem.h | 12 ++ include/media/videobuf2-core.h | 84 ++++++++- include/media/videobuf2-v4l2.h | 15 +- include/uapi/linux/videodev2.h | 1 + 23 files changed, 353 insertions(+), 74 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst