linux-kernel - Re: [PATCH] media: videobuf2: sync caches for dmabuf memory

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 for Android: free password hash cracker in your pocket

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

Message-ID: <036bf0d7f657cae444d20ea6d279b47e3bf0164e.camel@ndufresne.ca>
Date: Tue, 18 Jun 2024 12:24:03 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Tomasz Figa <tfiga@...omium.org>, TaoJiang <tao.jiang_2@....com>
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl, shawnguo@...nel.org, 
 robh+dt@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de, 
 festevam@...il.com, linux-imx@....com, xiahong.bao@....com,
 eagle.zhou@....com,  ming.qian@....nxp.com, imx@...ts.linux.dev,
 linux-media@...r.kernel.org,  linux-kernel@...r.kernel.org,
 m.szyprowski@...sung.com, sumit.semwal@...aro.org, 
 christian.koenig@....com, dri-devel@...ts.freedesktop.org, 
 linaro-mm-sig@...ts.linaro.org, Ming Qian <ming.qian@....com>
Subject: Re: [PATCH] media: videobuf2: sync caches for dmabuf memory

Le mardi 18 juin 2024 à 16:47 +0900, Tomasz Figa a écrit :
> Hi TaoJiang,
> 
> On Tue, Jun 18, 2024 at 4:30 PM TaoJiang <tao.jiang_2@....com> wrote:
> > 
> > From: Ming Qian <ming.qian@....com>
> > 
> > When the memory type is VB2_MEMORY_DMABUF, the v4l2 device can't know
> > whether the dma buffer is coherent or synchronized.
> > 
> > The videobuf2-core will skip cache syncs as it think the DMA exporter
> > should take care of cache syncs
> > 
> > But in fact it's likely that the client doesn't
> > synchronize the dma buf before qbuf() or after dqbuf(). and it's
> > difficult to find this type of error directly.
> > 
> > I think it's helpful that videobuf2-core can call
> > dma_buf_end_cpu_access() and dma_buf_begin_cpu_access() to handle the
> > cache syncs.
> > 
> > Signed-off-by: Ming Qian <ming.qian@....com>
> > Signed-off-by: TaoJiang <tao.jiang_2@....com>
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> 
> Sorry, that patch is incorrect. I believe you're misunderstanding the
> way DMA-buf buffers should be managed in the userspace. It's the
> userspace responsibility to call the DMA_BUF_IOCTL_SYNC ioctl [1] to
> signal start and end of CPU access to the kernel and imply necessary
> cache synchronization.
> 
> [1] https://docs.kernel.org/driver-api/dma-buf.html#dma-buffer-ioctls
> 
> So, really sorry, but it's a NAK.



This patch *could* make sense if it was inside UVC Driver as an example, as this
driver can import dmabuf, to CPU memcpy, and does omits the required sync calls
(unless that got added recently, I can easily have missed it).

But generally speaking, bracketing all driver with CPU access synchronization
does not make sense indeed, so I second the rejection.

Nicolas

> 
> Best regards,
> Tomasz
> 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 358f1fe42975..4734ff9cf3ce 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -340,6 +340,17 @@ static void __vb2_buf_mem_prepare(struct vb2_buffer *vb)
> >         vb->synced = 1;
> >         for (plane = 0; plane < vb->num_planes; ++plane)
> >                 call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
> > +
> > +       if (vb->memory != VB2_MEMORY_DMABUF)
> > +               return;
> > +       for (plane = 0; plane < vb->num_planes; ++plane) {
> > +               struct dma_buf *dbuf = vb->planes[plane].dbuf;
> > +
> > +               if (!dbuf)
> > +                       continue;
> > +
> > +               dma_buf_end_cpu_access(dbuf, vb->vb2_queue->dma_dir);
> > +       }
> >  }
> > 
> >  /*
> > @@ -356,6 +367,17 @@ static void __vb2_buf_mem_finish(struct vb2_buffer *vb)
> >         vb->synced = 0;
> >         for (plane = 0; plane < vb->num_planes; ++plane)
> >                 call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> > +
> > +       if (vb->memory != VB2_MEMORY_DMABUF)
> > +               return;
> > +       for (plane = 0; plane < vb->num_planes; ++plane) {
> > +               struct dma_buf *dbuf = vb->planes[plane].dbuf;
> > +
> > +               if (!dbuf)
> > +                       continue;
> > +
> > +               dma_buf_begin_cpu_access(dbuf, vb->vb2_queue->dma_dir);
> > +       }
> >  }
> > 
> >  /*
> > --
> > 2.43.0-rc1
> >