linux-kernel - Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200124012803.GB158382@google.com>
Date:   Fri, 24 Jan 2020 10:28:03 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Pawel Osciak <posciak@...omium.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 06/15] videobuf2: handle
 V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS

On (20/01/23 12:41), Hans Verkuil wrote:
[..]
> >>>  
> >>>  	fill_buf_caps(q, &create->capabilities);
> >>> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> >>>  	for (i = 0; i < requested_planes; i++)
> >>>  		if (requested_sizes[i] == 0)
> >>>  			return -EINVAL;
> >>> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> >>> +
> >>> +	if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> >>> +		consistent = false;
> >>> +
> >>> +	return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent,
> >>>  		&create->count, requested_planes, requested_sizes);
> >>
> >> As mentioned before: we need a V4L2_BUF_CAP capability.
> > 
> > I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory
> > property though? User space may request MMAP consistent memory or MMAP
> > inconsistent memory.
> 
> So instead of adding a flag we add a V4L2_MEMORY_MMAP_NON_CONSISTENT memory
> type and add a V4L2_BUF_CAP_SUPPORTS_MMAP_NON_CONSISTENT to signal support
> for this?
> 
> I like that better than a flag. It also automatically enforces that all
> buffers must be of that type.

Yes, we had this idea as well. The conclusion was it makes the patch
set bigger and harder to verify and review. Passing memory consistency
attribute via ->flags was the shortest path at the end. Namely due to all
those numerous places that test q->memory:

 455                 if (q->memory == VB2_MEMORY_MMAP)
 456                         __vb2_buf_mem_free(vb);
 457                 else if (q->memory == VB2_MEMORY_DMABUF)
 458                         __vb2_buf_dmabuf_put(vb);
 459                 else
 460                         __vb2_buf_userptr_put(vb);

[..]

 737                 mutex_lock(&q->mmap_lock);
 738                 if (debug && q->memory == VB2_MEMORY_MMAP &&
 739                     __buffers_in_use(q))
 740                         dprintk(1, "memory in use, orphaning buffers\n");

[..]

etc.

As a workaround we looked at the idea that V4L2_MEMORY_MMAP_NON_CONSISTENT
flag might make sense only on the very high level - when user space requests
V4L2_MEMORY_MMAP_NON_CONSISTENT then we simply set DMA attribute and then
"downgrade" requested V4L2_MEMORY_MMAP_NON_CONSISTENT memory type to
V4L2_MEMORY_MMAP.


Something like this.

---

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4489744fbbd9..60afbfcca995 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -803,6 +803,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 }
 EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
 
+static void __set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
+{
+	if (consistent_mem)
+		q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
+	else
+		q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
+}
+
 int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 		unsigned int *count, unsigned requested_planes,
 		const unsigned requested_sizes[])
@@ -810,6 +818,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 	unsigned int num_planes = 0, num_buffers, allocated_buffers;
 	unsigned plane_sizes[VB2_MAX_PLANES] = { };
 	int ret;
+	bool consistent_mem = (memory == V4L2_MEMORY_MMAP_NON_CONSISTENT);
+
+	if (consistent_mem)
+		memory = V4L2_MEMORY_MMAP;
 
 	if (q->num_buffers == VB2_MAX_FRAME) {
 		dprintk(1, "maximum number of buffers already allocated\n");
@@ -822,6 +834,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 			return -EBUSY;
 		}
 		memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
+		__set_queue_consistency(q, consistent_mem);
 		q->memory = memory;
 		q->waiting_for_buffers = !q->is_output;
 	} else if (q->memory != memory) {