[v2] DmaBufAllocator: Avoid syncing with an invalid file descriptor
diff mbox series

Message ID 20250113202843.843466-1-chenghaoyang@chromium.org
State Accepted
Commit 4dad8ece72c0c1c6c9e0101f4c31937bd739d001
Headers show
Series
  • [v2] DmaBufAllocator: Avoid syncing with an invalid file descriptor
Related show

Commit Message

Harvey Yang Jan. 13, 2025, 8:28 p.m. UTC
As DmaSyncer disables the copy c'tor, the move c'tor will be used
instead. This leaves some DmaSyncers with invalid SharedFDs. They should
avoid syncing with invalid file descriptors in the d'tor.

Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable")
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
Tested-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/dma_buf_allocator.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Milan Zamazal Jan. 14, 2025, 9:10 a.m. UTC | #1
Harvey Yang <chenghaoyang@chromium.org> writes:

> As DmaSyncer disables the copy c'tor, the move c'tor will be used
> instead. This leaves some DmaSyncers with invalid SharedFDs. They should
> avoid syncing with invalid file descriptors in the d'tor.
>
> Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable")
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> Tested-by: Milan Zamazal <mzamazal@redhat.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  src/libcamera/dma_buf_allocator.cpp | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index a014c3b4263c..d8c62dd67a96 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -325,7 +325,12 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
>  
>  DmaSyncer::~DmaSyncer()
>  {
> -	sync(DMA_BUF_SYNC_END);
> +	/*
> +	 * DmaSyncer might be moved and left with an empty SharedFD.
> +	 * Avoid syncing with an invalid file descriptor in this case.
> +	 */
> +	if (fd_.isValid())
> +		sync(DMA_BUF_SYNC_END);
>  }
>  
>  void DmaSyncer::sync(uint64_t step)
Kieran Bingham Jan. 14, 2025, 10:15 p.m. UTC | #2
Quoting Milan Zamazal (2025-01-14 09:10:12)
> Harvey Yang <chenghaoyang@chromium.org> writes:
> 
> > As DmaSyncer disables the copy c'tor, the move c'tor will be used
> > instead. This leaves some DmaSyncers with invalid SharedFDs. They should
> > avoid syncing with invalid file descriptors in the d'tor.
> >
> > Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable")
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > Tested-by: Milan Zamazal <mzamazal@redhat.com>
> 
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Will push through CI and merge iff/when it completes.

--
Kieran


> 
> > ---
> >  src/libcamera/dma_buf_allocator.cpp | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> > index a014c3b4263c..d8c62dd67a96 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -325,7 +325,12 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
> >  
> >  DmaSyncer::~DmaSyncer()
> >  {
> > -     sync(DMA_BUF_SYNC_END);
> > +     /*
> > +      * DmaSyncer might be moved and left with an empty SharedFD.
> > +      * Avoid syncing with an invalid file descriptor in this case.
> > +      */
> > +     if (fd_.isValid())
> > +             sync(DMA_BUF_SYNC_END);
> >  }
> >  
> >  void DmaSyncer::sync(uint64_t step)
>

Patch
diff mbox series

diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
index a014c3b4263c..d8c62dd67a96 100644
--- a/src/libcamera/dma_buf_allocator.cpp
+++ b/src/libcamera/dma_buf_allocator.cpp
@@ -325,7 +325,12 @@  DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
 
 DmaSyncer::~DmaSyncer()
 {
-	sync(DMA_BUF_SYNC_END);
+	/*
+	 * DmaSyncer might be moved and left with an empty SharedFD.
+	 * Avoid syncing with an invalid file descriptor in this case.
+	 */
+	if (fd_.isValid())
+		sync(DMA_BUF_SYNC_END);
 }
 
 void DmaSyncer::sync(uint64_t step)