Linaro-mm-sig January 2024

linaro-mm-sig@lists.linaro.org

23 participants
28 discussions

Re: [PATCH v3 4/4] Documentation: usb: Document FunctionFS DMABUF API

by Paul Cercueil

Hi Vegard, Le mardi 09 janvier 2024 à 14:08 +0100, Vegard Nossum a écrit : > On 08/01/2024 13:00, Paul Cercueil wrote: > > Add documentation for the three ioctls used to attach or detach > > externally-created DMABUFs, and to request transfers from/to > > previously > > attached DMABUFs. > > > > Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> > > > > --- > > v3: New patch > > --- > > Documentation/usb/functionfs.rst | 36 > > ++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > Hi, > > I'd like to point out that this file (usb/functionfs.rst) is > currently > included by Documentation/subsystem-apis.rst, the top-level file for > the > "Kernel subsystem documentation" set of books, which describe > internal > APIs: "These books get into the details of how specific kernel > subsystems work from the point of view of a kernel developer". > > However, functionfs.rst (and especially your new additions) are > documenting a userspace API, so it really belongs somewhere in > Documentation/userspace-api/ -- that's where /proc, /sys, /dev and > ioctl > descriptions for userspace programmers belong. Agreed. Even the original content prior to my additions describe a userspace API. > > I'm not NAKing the patch -- I just want to draw attention to this > discrepancy. Maybe we can separate the kernel-implementation details > (stuff about __init sections and stuff) from the new ioctl() info? > > Looking at <https://docs.kernel.org/usb/> I see that there are many > other adjacent documents that are also not really documenting kernel > implementation details, rough categorization as follows: > > USB support > ----------- > > - Linux ACM driver v0.16 ==> admin/user info > - Authorizing (or not) your USB devices to connect to the system ==> > admin/user info > - ChipIdea Highspeed Dual Role Controller Driver => admin/user info > - DWC3 driver ==> driver TODOs (can be moved into source code?) > - EHCI driver ==> technical info + driver details > - How FunctionFS works > - Linux USB gadget configured through configfs ==> userspace API + > implementation > - Linux USB HID gadget driver ==> implementation + userspace API > - Multifunction Composite Gadget ==> technical + user info > - Linux USB Printer Gadget Driver ==> userspace API > - Linux Gadget Serial Driver v2.0 ==> user/admin + userspace API > - Linux UVC Gadget Driver ==> user/admin + userspace API > - Gadget Testing ==> user/admin + userspace API > - Infinity Usb Unlimited Readme ==> user/admin > - Mass Storage Gadget (MSG) ==> user/admin > - USB 7-Segment Numeric Display ==> user/admin > - mtouchusb driver ==> user/admin > - OHCI ==> technical info > - USB Raw Gadget ==> userspace API > - USB/IP protocol ==> technical info > - usbmon ==> user/admin + userspace API > - USB serial ==> user/admin + technical info > - USB references > - Linux CDC ACM inf > - Linux inf > - USB devfs drop permissions source > - Credits > > By "admin/user info", I mean things that a user would have to do or > run > (e.g. modprobe + flags) to make use of a driver; "technical info" is > more like device specifications (transfer speeds, modes of operation, > etc.); "userspace API" is stuff like configfs and ioctls; "driver > details" is really implementation details and internal > considerations. > > The last ones I don't even really know how to categorize. > > I'm guessing nobody is really enthralled by the idea of splitting > Documentation/usb/ up like this? > > Documentation/admin-guide/usb/ > Documentation/driver-api/usb/ (this one actually exists already) > Documentation/userspace-api/usb/ > > For the stuff that is _actually_ internal to a specific driver (so > not > useful for end users, not useful for admins, not generic USB info, > and > not useful for userspace programmers), I would honestly propose to > just > move it directly into the driver's source code, or, if the text is > obsolete, just get rid of it completely. > > The distinction between user/admin and userspace API is pretty clear > (one is for end users, the other is for userspace _programmers_), but > it > can sometimes be hard to determine whether something falls in one or > the > other category. > > In any case -- it looks like almost all of the usb/ directory does > not > document "how specific kernel subsystems work from the point of view > of > a kernel developer" so maybe we should just move the include to > userspace-api/ for now as an obvious improvement (if still not 100% > correct): > > diff --git a/Documentation/subsystem-apis.rst > b/Documentation/subsystem-apis.rst > index 2d353fb8ea26..fe972f57bf4c 100644 > --- a/Documentation/subsystem-apis.rst > +++ b/Documentation/subsystem-apis.rst > @@ -81,7 +81,6 @@ Storage interfaces > security/index > crypto/index > bpf/index > - usb/index > PCI/index > misc-devices/index > peci/index > diff --git a/Documentation/userspace-api/index.rst > b/Documentation/userspace-api/index.rst > index 82f9dbd228f5..e60cd9174ada 100644 > --- a/Documentation/userspace-api/index.rst > +++ b/Documentation/userspace-api/index.rst > @@ -41,6 +41,7 @@ Subsystem-specific documentation: > tee > isapnp > dcdbas > + ../usb/index > > Kernel ABIs: These documents describe the the ABI between the Linux > kernel and userspace, and the relative stability of these > interfaces. > > > Thoughts? Makes sense to me. There's definitely some cleanup to be done in the USB documentation. > Vegard Cheers, -Paul

11 months, 1 week

[PATCH] dma-buf/dma-resv: fix spelling

by Randy Dunlap

Fix spelling mistakes as reported by codespell. Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: Christian König <christian.koenig(a)amd.com> Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-resv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff -- a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -405,7 +405,7 @@ static void dma_resv_iter_walk_unlocked( * * Beware that the iterator can be restarted. Code which accumulates statistics * or similar needs to check for this with dma_resv_iter_is_restarted(). For - * this reason prefer the locked dma_resv_iter_first() whenver possible. + * this reason prefer the locked dma_resv_iter_first() whenever possible. * * Returns the first fence from an unlocked dma_resv obj. */ @@ -428,7 +428,7 @@ EXPORT_SYMBOL(dma_resv_iter_first_unlock * * Beware that the iterator can be restarted. Code which accumulates statistics * or similar needs to check for this with dma_resv_iter_is_restarted(). For - * this reason prefer the locked dma_resv_iter_next() whenver possible. + * this reason prefer the locked dma_resv_iter_next() whenever possible. * * Returns the next fence from an unlocked dma_resv obj. */

11 months, 2 weeks

[PATCH] dma-buf/dma-fence: fix spelling

by Randy Dunlap

Fix spelling mistakes as reported by codespell. Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: Christian König <christian.koenig(a)amd.com> Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-fence.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff -- a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -102,7 +102,7 @@ static atomic64_t dma_fence_context_coun * * * Drivers are allowed to call dma_fence_wait() from their &mmu_notifier * respectively &mmu_interval_notifier callbacks. This means any code required - * for fence completeion cannot allocate memory with GFP_NOFS or GFP_NOIO. + * for fence completion cannot allocate memory with GFP_NOFS or GFP_NOIO. * Only GFP_ATOMIC is permissible, which might fail. * * Note that only GPU drivers have a reasonable excuse for both requiring @@ -522,7 +522,7 @@ dma_fence_wait_timeout(struct dma_fence EXPORT_SYMBOL(dma_fence_wait_timeout); /** - * dma_fence_release - default relese function for fences + * dma_fence_release - default release function for fences * @kref: &dma_fence.recfount * * This is the default release functions for &dma_fence. Drivers shouldn't call @@ -974,8 +974,8 @@ void dma_fence_set_deadline(struct dma_f EXPORT_SYMBOL(dma_fence_set_deadline); /** - * dma_fence_describe - Dump fence describtion into seq_file - * @fence: the 6fence to describe + * dma_fence_describe - Dump fence description into seq_file + * @fence: the fence to describe * @seq: the seq_file to put the textual description into * * Dump a textual description of the fence and it's state into the seq_file.

11 months, 2 weeks

[PATCH v3 0/7] dma-buf: heaps: Add secure heap

by Yong Wu

This patchset is for secure video playback and enables other potential uses in the future. The 'secure dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/ unmappable by the non-secure (i.e.kernel/userspace) world. That memory will be used by the secure world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two secure heaps and they will be used v4l2[1] and drm[2]. 1) secure_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) secure_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos, Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20231206081538.17056-1-yunfei.dong@m… [2] https://lore.kernel.org/linux-mediatek/20231023044549.21412-1-jason-jh.lin@… Change note: v3: Base on v6.7-rc1. 1) Separate the secure heap into a common file(secure_heap.c) and a mtk special file (secure_heap_mtk.c), and put all tee related code into our special file. 2) About dt-binding, a) Add "mediatek," prefix since this is Mediatek TEE firmware definition. b) Mute dt-binding check waring. 3) Remove the normal CMA heap which is a draft for qcom. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediat… 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@m… 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@media… Base on v6.6-rc1. Yong Wu (7): dt-bindings: reserved-memory: Add mediatek,dynamic-secure-region dma-buf: heaps: Initialize a secure heap dma-buf: heaps: secure_heap: Add private heap ops dma-buf: heaps: secure_heap: Add dma_ops dma-buf: heaps: secure_heap: Add MediaTek secure heap and heap_init dma-buf: heaps: secure_heap_mtk: Add tee memory service call dma_buf: heaps: secure_heap_mtk: Add a new CMA heap .../mediatek,dynamic-secure-region.yaml | 43 +++ drivers/dma-buf/heaps/Kconfig | 13 + drivers/dma-buf/heaps/Makefile | 2 + drivers/dma-buf/heaps/secure_heap.c | 234 +++++++++++++ drivers/dma-buf/heaps/secure_heap.h | 43 +++ drivers/dma-buf/heaps/secure_heap_mtk.c | 321 ++++++++++++++++++ 6 files changed, 656 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-region.yaml create mode 100644 drivers/dma-buf/heaps/secure_heap.c create mode 100644 drivers/dma-buf/heaps/secure_heap.h create mode 100644 drivers/dma-buf/heaps/secure_heap_mtk.c -- 2.18.0

11 months, 2 weeks

Re: [PATCH v3,03/21] v4l2: verify secure dmabufs are used in secure queue

by Jeffrey Kardatzke

On Mon, Dec 11, 2023 at 2:58 AM Hans Verkuil <hverkuil-cisco(a)xs4all.nl> wrote: > > On 06/12/2023 09:15, Yunfei Dong wrote: > > From: Jeffrey Kardatzke <jkardatzke(a)google.com> > > > > Verfies in the dmabuf implementations that if the secure memory flag is > > Verfies -> Verifies Thanks. Yunfei, change that please. > > > set for a queue that the dmabuf submitted to the queue is unmappable. > > > > Signed-off-by: Jeffrey Kardatzke <jkardatzke(a)google.com> > > Signed-off-by: Yunfei Dong <yunfei.dong(a)mediatek.com> > > --- > > drivers/media/common/videobuf2/videobuf2-dma-contig.c | 6 ++++++ > > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 6 ++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > > index 3d4fd4ef5310..ad58ef8dc231 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > > @@ -710,6 +710,12 @@ static int vb2_dc_map_dmabuf(void *mem_priv) > > return -EINVAL; > > } > > > > + /* verify the dmabuf is secure if we are in secure mode */ > > + if (buf->vb->vb2_queue->secure_mem && sg_page(sgt->sgl)) { > > This needs a bit more explanation. I guess that for secure memory > sg_page returns NULL? How about if we change it to: /* verify the dmabuf is secure if we are in secure mode, this is done by validating there is no page entry for the dmabuf */ > > > + pr_err("secure queue requires secure dma_buf"); > > + return -EINVAL; > > + } > > + > > /* checking if dmabuf is big enough to store contiguous chunk */ > > contig_size = vb2_dc_get_contiguous_size(sgt); > > if (contig_size < buf->size) { > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index 28f3fdfe23a2..55428c73c380 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -564,6 +564,12 @@ static int vb2_dma_sg_map_dmabuf(void *mem_priv) > > return -EINVAL; > > } > > > > + /* verify the dmabuf is secure if we are in secure mode */ > > + if (buf->vb->vb2_queue->secure_mem && !sg_dma_secure(sgt->sgl)) { > > I can't find the sg_dma_secure function. I suspect this patch series > depends on another series? That was an oversight, it should be the same as in videobuf2-dma-contig.c. Yunfei, can you change this to match what's in videobuf2-dma-contig.c after the comment is reworded? > > > + pr_err("secure queue requires secure dma_buf"); > > + return -EINVAL; > > + } > > + > > buf->dma_sgt = sgt; > > buf->vaddr = NULL; > > > > Regards, > > Hans

11 months, 3 weeks

Re: [PATCH v3,04/21] v4l: add documentation for secure memory flag

by Jeffrey Kardatzke

On Mon, Dec 11, 2023 at 3:05 AM Hans Verkuil <hverkuil-cisco(a)xs4all.nl> wrote: > > On 06/12/2023 09:15, Yunfei Dong wrote: > > From: Jeffrey Kardatzke <jkardatzke(a)google.com> > > > > Adds documentation for V4L2_MEMORY_FLAG_SECURE. > > > > Signed-off-by: Jeffrey Kardatzke <jkardatzke(a)google.com> > > Signed-off-by: Yunfei Dong <yunfei.dong(a)mediatek.com> > > --- > > Documentation/userspace-api/media/v4l/buffer.rst | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst > > index 52bbee81c080..a5a7d1c72d53 100644 > > --- a/Documentation/userspace-api/media/v4l/buffer.rst > > +++ b/Documentation/userspace-api/media/v4l/buffer.rst > > @@ -696,7 +696,7 @@ enum v4l2_memory > > > > .. _memory-flags: > > > > -Memory Consistency Flags > > +Memory Flags > > ------------------------ > > > > .. raw:: latex > > @@ -728,6 +728,12 @@ Memory Consistency Flags > > only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the > > queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS > > <V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS>` capability. > > + * .. _`V4L2-MEMORY-FLAG-SECURE`: > > + > > + - ``V4L2_MEMORY_FLAG_SECURE`` > > + - 0x00000002 > > + - DMA bufs passed into the queue will be validated to ensure they were > > + allocated from a secure dma-heap. > > Hmm, that needs a bit more work. How about: > > - The queued buffers are expected to be in secure memory. If not, an error will be > returned. This flag can only be used with ``V4L2_MEMORY_DMABUF``. Typically > secure buffers are allocated using a secure dma-heap. This flag can only be > specified if the ``V4L2_BUF_CAP_SUPPORTS_SECURE_MEM`` is set. > Thanks Hans. Yunfei, can you integrate this change into the patch please? > In addition, the title of this table is currently "Memory Consistency Flags": that > should be renamed to "Memory Flags". Hans, the patch is already renaming the table as you suggested. :) (unless there's some other spot I'm missing) > > Regards, > > Hans > > > > > .. raw:: latex > > >

11 months, 3 weeks

Re: [PATCH v3,02/21] v4l2: handle secure memory flags in queue setup

by Jeffrey Kardatzke

Yunfei, Can you please integrate these changes into the patch? Thanks, Jeff On Mon, Dec 11, 2023 at 2:45 AM Hans Verkuil <hverkuil-cisco(a)xs4all.nl> wrote: > > Hi Yunfei, Jeffrey, > > Some comments below: > > On 06/12/2023 09:15, Yunfei Dong wrote: > > From: Jeffrey Kardatzke <jkardatzke(a)google.com> > > > > Validates the secure memory flags when setting up a queue and ensures > > the queue has the proper capability. > > > > Signed-off-by: Jeffrey Kardatzke <jkardatzke(a)google.com> > > Signed-off-by: Yunfei Dong <yunfei.dong(a)mediatek.com> > > --- > > .../media/common/videobuf2/videobuf2-core.c | 23 +++++++++++++ > > .../media/common/videobuf2/videobuf2-v4l2.c | 34 +++++++++++++------ > > 2 files changed, 46 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index 8c1df829745b..09dc030484be 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -813,6 +813,15 @@ static bool verify_coherency_flags(struct vb2_queue *q, bool non_coherent_mem) > > return true; > > } > > > > +static bool verify_secure_mem_flags(struct vb2_queue *q, bool secure_mem) > > +{ > > + if (secure_mem != q->secure_mem) { > > + dprintk(q, 1, "secure memory model mismatch\n"); > > + return false; > > + } > > + return true; > > +} > > + > > int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > unsigned int flags, unsigned int *count) > > { > > @@ -820,6 +829,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > unsigned int q_num_bufs = vb2_get_num_buffers(q); > > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > > bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT; > > + bool secure_mem = flags & V4L2_MEMORY_FLAG_SECURE; > > unsigned int i; > > int ret = 0; > > > > @@ -836,6 +846,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > if (*count == 0 || q_num_bufs != 0 || > > (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) || > > !verify_coherency_flags(q, non_coherent_mem)) { > > + bool no_previous_buffers = !q->num_buffers; > > + > > /* > > * We already have buffers allocated, so first check if they > > * are not in use and can be freed. > > @@ -854,6 +866,12 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > __vb2_queue_free(q, q_num_bufs); > > mutex_unlock(&q->mmap_lock); > > > > + /* > > + * Do not allow switching secure buffer mode. > > + */ > > + if (!no_previous_buffers && !verify_secure_mem_flags(q, secure_mem)) > > + return -EINVAL; > > + > > Why is this needed? Here VIDIOC_REQBUFS is called either to just delete > all existing buffers (count == 0), or to delete all existing buffers and > allocate new buffers (count > 0). > > Since in both cases all existing buffers are deleted, you are free to choose > whatever new secure mode you want. > > > /* > > * In case of REQBUFS(0) return immediately without calling > > * driver's queue_setup() callback and allocating resources. > > @@ -882,6 +900,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > if (ret) > > return ret; > > set_queue_coherency(q, non_coherent_mem); > > + q->secure_mem = secure_mem; > > > > /* > > * Ask the driver how many buffers and planes per buffer it requires. > > @@ -986,6 +1005,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > > bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT; > > unsigned int q_num_bufs = vb2_get_num_buffers(q); > > + bool secure_mem = flags & V4L2_MEMORY_FLAG_SECURE; > > bool no_previous_buffers = !q_num_bufs; > > int ret = 0; > > > > @@ -1015,6 +1035,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > return ret; > > q->waiting_for_buffers = !q->is_output; > > set_queue_coherency(q, non_coherent_mem); > > + q->secure_mem = secure_mem; > > } else { > > if (q->memory != memory) { > > dprintk(q, 1, "memory model mismatch\n"); > > @@ -1022,6 +1043,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > } > > if (!verify_coherency_flags(q, non_coherent_mem)) > > return -EINVAL; > > + if (!verify_secure_mem_flags(q, secure_mem)) > > + return -EINVAL; > > } > > > > num_buffers = min(*count, q->max_num_buffers - q_num_bufs); > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > > index 54d572c3b515..0a530830276c 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > > @@ -686,22 +686,30 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps) > > *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS; > > if (q->supports_requests) > > *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS; > > + if (q->allow_secure_mem && q->io_modes & VB2_DMABUF) > > + *caps |= V4L2_BUF_CAP_SUPPORTS_SECURE_MEM; > > } > > > > -static void validate_memory_flags(struct vb2_queue *q, > > +static bool validate_memory_flags(struct vb2_queue *q, > > int memory, > > u32 *flags) > > { > > + if (*flags & V4L2_MEMORY_FLAG_SECURE && > > + (!q->allow_secure_mem || memory != V4L2_MEMORY_DMABUF)) { > > + return false; > > + } > > + > > This check belongs to videobuf2-core.c and the check should be done > in vb2_core_reqbufs and vb2_core_create_bufs. > > So just leave this function as a void. > > > if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP) { > > /* > > - * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only, > > - * but in order to avoid bugs we zero out all bits. > > + * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only. > > Just drop this as well since it adds no useful information anymore. > > > */ > > - *flags = 0; > > - } else { > > - /* Clear all unknown flags. */ > > - *flags &= V4L2_MEMORY_FLAG_NON_COHERENT; > > + *flags &= ~V4L2_MEMORY_FLAG_NON_COHERENT; > > } > > + > > + /* Clear all unknown flags. */ > > + *flags &= V4L2_MEMORY_FLAG_NON_COHERENT | V4L2_MEMORY_FLAG_SECURE; > > This is still needed here. > > > + > > + return true; > > } > > > > So the following changes from here... > > > int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > > @@ -710,7 +718,8 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > > u32 flags = req->flags; > > > > fill_buf_caps(q, &req->capabilities); > > - validate_memory_flags(q, req->memory, &flags); > > + if (!validate_memory_flags(q, req->memory, &flags)) > > + return -EINVAL; > > req->flags = flags; > > return ret ? ret : vb2_core_reqbufs(q, req->memory, > > req->flags, &req->count); > > @@ -752,7 +761,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > > unsigned i; > > > > fill_buf_caps(q, &create->capabilities); > > - validate_memory_flags(q, create->memory, &create->flags); > > + if (!validate_memory_flags(q, create->memory, &create->flags)) > > + return -EINVAL; > > create->index = vb2_get_num_buffers(q); > > create->max_num_buffers = q->max_num_buffers; > > create->capabilities |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS; > > @@ -1007,7 +1017,8 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, > > u32 flags = p->flags; > > > > fill_buf_caps(vdev->queue, &p->capabilities); > > - validate_memory_flags(vdev->queue, p->memory, &flags); > > + if (!validate_memory_flags(vdev->queue, p->memory, &flags)) > > + return -EINVAL; > > p->flags = flags; > > if (res) > > return res; > > @@ -1031,7 +1042,8 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv, > > > > p->index = vdev->queue->num_buffers; > > fill_buf_caps(vdev->queue, &p->capabilities); > > - validate_memory_flags(vdev->queue, p->memory, &p->flags); > > + if (!validate_memory_flags(vdev->queue, p->memory, &p->flags)) > > + return -EINVAL; > > /* > > * If count == 0, then just check if memory and type are valid. > > * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0. > > ...to the end should all be dropped since the vb2 core will do the checks. > > Regards, > > Hans

11 months, 3 weeks

Re: [syzbot] [dri?] WARNING in drm_prime_destroy_file_private (2)

by Christian König

Am 28.12.23 um 03:57 schrieb Qi Zheng: > > > On 2023/12/28 04:51, syzbot wrote: >> Hello, >> >> syzbot found the following issue on: >> >> HEAD commit: 5254c0cbc92d Merge tag 'block-6.7-2023-12-22' of >> git://git.. >> git tree: upstream >> console+strace: https://syzkaller.appspot.com/x/log.txt?x=10cc6995e80000 >> kernel config: >> https://syzkaller.appspot.com/x/.config?x=314e9ad033a7d3a7 >> dashboard link: >> https://syzkaller.appspot.com/bug?extid=59dcc2e7283a6f5f5ba1 >> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils >> for Debian) 2.40 >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13e35809e80000 >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=155d5fd6e80000 >> >> Downloadable assets: >> disk image: >> https://storage.googleapis.com/syzbot-assets/ebe09a5995ee/disk-5254c0cb.raw… >> vmlinux: >> https://storage.googleapis.com/syzbot-assets/02178d7f5f98/vmlinux-5254c0cb.… >> kernel image: >> https://storage.googleapis.com/syzbot-assets/12307f47d87c/bzImage-5254c0cb.… >> >> The issue was bisected to: >> >> commit ea4452de2ae987342fadbdd2c044034e6480daad >> Author: Qi Zheng <zhengqi.arch(a)bytedance.com> >> Date: Fri Nov 18 10:00:11 2022 +0000 >> >> mm: fix unexpected changes to {failslab|fail_page_alloc}.attr >> >> bisection log: >> https://syzkaller.appspot.com/x/bisect.txt?x=13027f76e80000 >> final oops: https://syzkaller.appspot.com/x/report.txt?x=10827f76e80000 >> console output: https://syzkaller.appspot.com/x/log.txt?x=17027f76e80000 >> >> IMPORTANT: if you fix the issue, please add the following tag to the >> commit: >> Reported-by: syzbot+59dcc2e7283a6f5f5ba1(a)syzkaller.appspotmail.com >> Fixes: ea4452de2ae9 ("mm: fix unexpected changes to >> {failslab|fail_page_alloc}.attr") >> >> R10: 0000000000000000 R11: 0000000000000246 R12: 00007efe98069194 >> R13: 00007efe97fd2210 R14: 0000000000000002 R15: 6972642f7665642f >> </TASK> >> ------------[ cut here ]------------ >> WARNING: CPU: 0 PID: 5107 at drivers/gpu/drm/drm_prime.c:227 >> drm_prime_destroy_file_private+0x43/0x60 drivers/gpu/drm/drm_prime.c:227 > > The warning is caused by !RB_EMPTY_ROOT(&prime_fpriv->dmabufs): > > drm_prime_destroy_file_private > --> WARN_ON(!RB_EMPTY_ROOT(&prime_fpriv->dmabufs)); > > It seems irrelevant to the logic of fault injection. So I don't see > why the commit ea4452de2ae9 can cause this warning. :( Making an educated guess I strongly think syzbot incorrectly bisected this. What basically happens is that a DRM test case crashes because a file private data structure is destroyed before all DMA-bufs referring to it are destroyed. Looks like a random race condition in a test case to me. Question is really what test is syzbot running and who is maintaining this test case? Regards, Christian. > >> Modules linked in: >> CPU: 0 PID: 5107 Comm: syz-executor227 Not tainted >> 6.7.0-rc6-syzkaller-00248-g5254c0cbc92d #0 >> Hardware name: Google Google Compute Engine/Google Compute Engine, >> BIOS Google 11/17/2023 >> RIP: 0010:drm_prime_destroy_file_private+0x43/0x60 >> drivers/gpu/drm/drm_prime.c:227 >> Code: 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c 02 00 75 21 48 8b 83 >> 90 00 00 00 48 85 c0 75 06 5b e9 13 f1 93 fc e8 0e f1 93 fc 90 <0f> >> 0b 90 5b e9 04 f1 93 fc e8 3f 9b ea fc eb d8 66 66 2e 0f 1f 84 >> RSP: 0018:ffffc90003bdf9e0 EFLAGS: 00010293 >> RAX: 0000000000000000 RBX: ffff888019f28378 RCX: ffffc90003bdf9b0 >> RDX: ffff888018ff9dc0 RSI: ffffffff84f380c2 RDI: ffff888019f28408 >> RBP: ffff888019f28000 R08: 0000000000000001 R09: 0000000000000001 >> R10: ffffffff8f193a57 R11: 0000000000000000 R12: ffff88814829a000 >> R13: ffff888019f282a8 R14: ffff88814829a068 R15: ffff88814829a0a0 >> FS: 0000000000000000(0000) GS:ffff8880b9800000(0000) >> knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007efe98050410 CR3: 000000006d1ff000 CR4: 0000000000350ef0 >> Call Trace: >> <TASK> >> drm_file_free.part.0+0x738/0xb90 drivers/gpu/drm/drm_file.c:290 >> drm_file_free drivers/gpu/drm/drm_file.c:247 [inline] >> drm_close_helper.isra.0+0x180/0x1f0 drivers/gpu/drm/drm_file.c:307 >> drm_release+0x22a/0x4f0 drivers/gpu/drm/drm_file.c:494 >> __fput+0x270/0xb70 fs/file_table.c:394 >> task_work_run+0x14d/0x240 kernel/task_work.c:180 >> exit_task_work include/linux/task_work.h:38 [inline] >> do_exit+0xa8a/0x2ad0 kernel/exit.c:869 >> do_group_exit+0xd4/0x2a0 kernel/exit.c:1018 >> get_signal+0x23b5/0x2790 kernel/signal.c:2904 >> arch_do_signal_or_restart+0x90/0x7f0 arch/x86/kernel/signal.c:309 >> exit_to_user_mode_loop kernel/entry/common.c:168 [inline] >> exit_to_user_mode_prepare+0x121/0x240 kernel/entry/common.c:204 >> __syscall_exit_to_user_mode_work kernel/entry/common.c:285 [inline] >> syscall_exit_to_user_mode+0x1e/0x60 kernel/entry/common.c:296 >> do_syscall_64+0x4d/0x110 arch/x86/entry/common.c:89 >> entry_SYSCALL_64_after_hwframe+0x63/0x6b >> RIP: 0033:0x7efe98014769 >> Code: Unable to access opcode bytes at 0x7efe9801473f. >> RSP: 002b:00007efe97fd2208 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca >> RAX: fffffffffffffe00 RBX: 00007efe9809c408 RCX: 00007efe98014769 >> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007efe9809c408 >> RBP: 00007efe9809c400 R08: 0000000000003131 R09: 0000000000003131 >> R10: 0000000000000000 R11: 0000000000000246 R12: 00007efe98069194 >> R13: 00007efe97fd2210 R14: 0000000000000002 R15: 6972642f7665642f >> </TASK> >> >> >> --- >> This report is generated by a bot. It may contain errors. >> See https://goo.gl/tpsmEJ for more information about syzbot. >> syzbot engineers can be reached at syzkaller(a)googlegroups.com. >> >> syzbot will keep track of this issue. See: >> https://goo.gl/tpsmEJ#status for how to communicate with syzbot. >> For information about bisection process see: >> https://goo.gl/tpsmEJ#bisection >> >> If the report is already addressed, let syzbot know by replying with: >> #syz fix: exact-commit-title >> >> If you want syzbot to run the reproducer, reply with: >> #syz test: git://repo/address.git branch-or-commit-hash >> If you attach or paste a git patch, syzbot will apply it before testing. >> >> If you want to overwrite report's subsystems, reply with: >> #syz set subsystems: new-subsystem >> (See the list of subsystem names on the web dashboard) >> >> If the report is a duplicate of another one, reply with: >> #syz dup: exact-subject-of-another-report >> >> If you want to undo deduplication, reply with: >> #syz undup

11 months, 3 weeks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig January 2024