| 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
| ||
|
Message-ID: <20230313181155.GC22646@pendragon.ideasonboard.com>
Date: Mon, 13 Mar 2023 20:11:55 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc: 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, hverkuil-cisco@...all.nl,
jerbel@...nel.org, 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: [RFC 2/4] media: videobuf2: Replace bufs array by a list
Hi Benjamin,
Thank you for the patch.
On Mon, Mar 13, 2023 at 02:59:14PM +0100, Benjamin Gaignard wrote:
> Replacing bufs array by a list allows to remove the 32 buffers
> limit per queue.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 14 ++------------
> include/media/videobuf2-core.h | 19 +++++++++++++------
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b51152ace763..96597d339a07 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -412,10 +412,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 can indeed drop this check now, but shouldn't we introduce some kind
of resource accounting and limitation ? Otherwise any unpriviledged
userspace will be able to starve system memory. This could be
implemented on top, as the problem largely exists today already, but I'd
like to at least record this in a TODO comment.
I also wonder if we should still limit the number of allocated buffers.
The limit could be large, for instance 1024 buffers, and it would be an
in-kernel limit that could be increased later if needed. I'm concerned
that dropping the limit completely will allow userspace to request
UINT_MAX buffers, which may cause integer overflows somewhere. Limiting
the number of buffers would avoid extensive review of all the code that
deals with counting buffers.
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> @@ -797,9 +793,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
> @@ -915,11 +909,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");
> @@ -944,7 +933,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;
> @@ -2444,6 +2433,7 @@ int vb2_core_queue_init(struct vb2_queue *q)
>
> INIT_LIST_HEAD(&q->queued_list);
> INIT_LIST_HEAD(&q->done_list);
> + INIT_LIST_HEAD(&q->allocated_bufs);
> spin_lock_init(&q->done_lock);
> mutex_init(&q->mmap_lock);
> init_waitqueue_head(&q->done_wq);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index d18c57e7aef0..47f1f35eb9cb 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -276,6 +276,8 @@ struct vb2_buffer {
> * done_entry: entry on the list that stores all buffers ready
> * to be dequeued to userspace
> * vb2_plane: per-plane information; do not change
> + * allocated_entry: entry on the list that stores all buffers allocated
> + * for the queue.
> */
> enum vb2_buffer_state state;
> unsigned int synced:1;
> @@ -287,6 +289,7 @@ struct vb2_buffer {
> struct vb2_plane planes[VB2_MAX_PLANES];
> struct list_head queued_entry;
> struct list_head done_entry;
> + struct list_head allocated_entry;
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> /*
> * Counters for how often these buffer-related ops are
> @@ -556,7 +559,7 @@ struct vb2_buf_ops {
> * @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
> * @memory: current memory type used
> * @dma_dir: DMA mapping direction.
> - * @bufs: videobuf2 buffer structures
> + * @allocated_bufs: list of buffer allocated for the queue.
> * @num_buffers: number of allocated/used buffers
> * @queued_list: list of buffers currently queued from userspace
> * @queued_count: number of buffers queued and ready for streaming.
> @@ -619,7 +622,7 @@ struct vb2_queue {
> struct mutex mmap_lock;
> unsigned int memory;
> enum dma_data_direction dma_dir;
> - struct vb2_buffer *bufs[VB2_MAX_FRAME];
> + struct list_head allocated_bufs;
> unsigned int num_buffers;
>
> struct list_head queued_list;
> @@ -1239,8 +1242,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> unsigned int index)
> {
> - if (index < q->num_buffers)
> - return q->bufs[index];
> + struct vb2_buffer *vb;
> +
> + list_for_each_entry(vb, &q->allocated_bufs, allocated_entry)
> + if (vb->index == index)
> + return vb;
> +
> return NULL;
> }
>
> @@ -1251,7 +1258,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> */
> static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - q->bufs[vb->index] = vb;
> + list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
> }
>
> /**
> @@ -1261,7 +1268,7 @@ static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> */
> static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - q->bufs[vb->index] = NULL;
> + list_del(&vb->allocated_entry);
> }
>
> /*
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists