linux-kernel - Re: [PATCH v7 03/28] media: videobuf2: calculate restricted memory size

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: <8c76b9da-bcc2-4316-b372-ea5f0c632d26@xs4all.nl>
Date: Sat, 20 Jul 2024 11:29:17 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Yunfei Dong <yunfei.dong@...iatek.com>,
 Jeffrey Kardatzke <jkardatzke@...gle.com>,
 Nícolas F . R . A . Prado <nfraprado@...labora.com>,
 Nathan Hebert <nhebert@...omium.org>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Benjamin Gaignard <benjamin.gaignard@...labora.com>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Tomasz Figa <tfiga@...omium.org>, Mauro Carvalho Chehab
 <mchehab@...nel.org>, Marek Szyprowski <m.szyprowski@...sung.com>
Cc: Chen-Yu Tsai <wenst@...omium.org>, Yong Wu <yong.wu@...iatek.com>,
 Hsin-Yi Wang <hsinyi@...omium.org>, Fritz Koenig <frkoenig@...omium.org>,
 Daniel Vetter <daniel@...ll.ch>, Steve Cho <stevecho@...omium.org>,
 Sumit Semwal <sumit.semwal@...aro.org>, Brian Starkey
 <Brian.Starkey@....com>, John Stultz <jstultz@...gle.com>,
 "T . J . Mercier" <tjmercier@...gle.com>,
 Christian König <christian.koenig@....com>,
 Matthias Brugger <matthias.bgg@...il.com>, linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v7 03/28] media: videobuf2: calculate restricted memory
 size

On 20/07/2024 09:15, Yunfei Dong wrote:
> Getting the physical address with sg_dma_address for restricted memory,
> only return the first physical address size since sg may not be physical
> continuous, then leading to the dmabuf size is small than buf size. Need
> to bypass continuous checking for restricted memory.
> 
> Signed-off-by: Yunfei Dong <yunfei.dong@...iatek.com>
> ---
>  .../common/videobuf2/videobuf2-dma-contig.c   | 34 +++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index 3d4fd4ef5310..f0e4652b615f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -66,6 +66,22 @@ static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
>  	return size;
>  }
>  
> +/**************************************************/
> +/*   restricted mem scatterlist table functions   */
> +/**************************************************/
> +
> +static unsigned long vb2_dc_get_res_mem_contiguous_size(struct sg_table *sgt)
> +{
> +	struct scatterlist *s;
> +	unsigned int i;
> +	unsigned long size = 0;
> +
> +	for_each_sgtable_dma_sg(sgt, s, i)
> +		size += sg_dma_len(s);
> +
> +	return size;
> +}

I think it is better to add a 'bool restricted' argument to vb2_dc_get_contiguous_size.
If true, then skip the 'expected' check there.

> +
>  /*********************************************/
>  /*         callbacks for all buffers         */
>  /*********************************************/
> @@ -648,10 +664,13 @@ static void *vb2_dc_get_userptr(struct vb2_buffer *vb, struct device *dev,
>  		goto fail_sgt_init;
>  	}
>  
> -	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (buf->vb->vb2_queue->restricted_mem)

I think it is better to do the same as with buf->non_coherent_mem,
so add a 'bool restricted_mem' to struct vb2_dc_buf and set it in
vb2_dc_alloc(). It makes this code easier to read.

> +		contig_size = vb2_dc_get_res_mem_contiguous_size(sgt);
> +	else
> +		contig_size = vb2_dc_get_contiguous_size(sgt);
>  	if (contig_size < size) {
> -		pr_err("contiguous mapping is too small %lu/%lu\n",
> -			contig_size, size);
> +		pr_err("contiguous mapping is too small %lu/%lu/%u\n",
> +		       contig_size, size, buf->vb->vb2_queue->restricted_mem);

Rather than add a "/%u", which is not easy to understand, perhaps do this:

		pr_err("%scontiguous mapping is too small %lu/%lu\n",
		       buf->vb->vb2_queue->restricted_mem ? "restricted " : "",
		       contig_size, size);

>  		ret = -EFAULT;
>  		goto fail_map_sg;
>  	}
> @@ -711,10 +730,13 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
>  	}
>  
>  	/* checking if dmabuf is big enough to store contiguous chunk */
> -	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (buf->vb->vb2_queue->restricted_mem)
> +		contig_size = vb2_dc_get_res_mem_contiguous_size(sgt);
> +	else
> +		contig_size = vb2_dc_get_contiguous_size(sgt);
>  	if (contig_size < buf->size) {
> -		pr_err("contiguous chunk is too small %lu/%lu\n",
> -		       contig_size, buf->size);
> +		pr_err("contiguous chunk is too small %lu/%lu/%u\n",
> +		       contig_size, buf->size, buf->vb->vb2_queue->restricted_mem);

Ditto.

>  		dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt,
>  						  buf->dma_dir);
>  		return -EFAULT;

Regards,

	Hans