Re: [RFC v1 1/4] red-parse-qxl: Extract the dmabuf fd from the scanout

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
<vivek.kasireddy@xxxxxxxxx> ha scritto:
>
> Hi Frediano,
>
> Thank you for taking a look at this patch series.
>
> >
> > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > <vivek.kasireddy@xxxxxxxxx> ha scritto:
> > >
> > > If the scanout has a valid dmabuf fd, then it is extracted and a
> > > copy (of the fd) is stored in the drawable.
> > >
> > > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > > Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > ---
> > >  server/red-parse-qxl.cpp | 8 ++++++++
> > >  server/red-parse-qxl.h   | 1 +
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/server/red-parse-qxl.cpp b/server/red-parse-qxl.cpp
> > > index 68b9759d..8d9b82fb 100644
> > > --- a/server/red-parse-qxl.cpp
> > > +++ b/server/red-parse-qxl.cpp
> > > @@ -1038,6 +1038,7 @@ static bool red_get_native_drawable(QXLInstance
> > *qxl_instance, RedMemSlotInfo *s
> > >                                      RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
> > >  {
> > >      QXLDrawable *qxl;
> > > +    SpiceMsgDisplayGlScanoutUnix *scanout;
> > >      int i;
> > >
> > >      qxl = static_cast<QXLDrawable *>(memslot_get_virt(slots, addr,
> > sizeof(*qxl), group_id));
> > > @@ -1059,6 +1060,13 @@ static bool red_get_native_drawable(QXLInstance
> > *qxl_instance, RedMemSlotInfo *s
> > >          red_get_rect_ptr(&red->surfaces_rects[i], &qxl->surfaces_rects[i]);
> > >      }
> > >
> > > +    red->dmabuf_fd = 0;
> >
> > 0 is a perfectly valid file descriptor, usually invalid file
> > descriptors are initialized with -1.
> [Kasireddy, Vivek] Ok, I'll initialize it to -1.
>
> >
> > > +    scanout = red_qxl_get_gl_scanout(qxl_instance);
> > > +    if (scanout != nullptr) {
> > > +        red->dmabuf_fd = scanout->drm_dma_buf_fd;
> > > +    }
> > > +    red_qxl_put_gl_scanout(qxl_instance, scanout);
> > > +
> > >      red->type = qxl->type;
> > >      switch (red->type) {
> > >      case QXL_DRAW_ALPHA_BLEND:
> > > diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> > > index 8bf0e2e3..dee50743 100644
> > > --- a/server/red-parse-qxl.h
> > > +++ b/server/red-parse-qxl.h
> > > @@ -67,6 +67,7 @@ struct RedDrawable final: public
> > RedQXLResource<RedDrawable> {
> > >          SpiceWhiteness whiteness;
> > >          SpiceComposite composite;
> > >      } u;
> > > +    int32_t dmabuf_fd;
> > >  };
> > >
> > >  struct RedUpdateCmd final: public RedQXLResource<RedUpdateCmd> {
> >
> > This patch looks pretty error prone, it's easy to generate leaks or
> > use after free (of file descriptor).
> [Kasireddy, Vivek] At-least with Qemu, we usually hand over a dup of the original
> fd to Spice server with the assumption that the server will close it after it is
> done using it.
>
> > Also it adds the assumption that the drawing is always associated with
> > the DMA surface which is racy.
> [Kasireddy, Vivek] I see that access to the scanout and drm_dma_buf_fd is protected
> with a scanout_mutex. Are you suggesting that using red_qxl_get/put_gl_scanout is
> not going to be sufficient to prevent races?
>

No, now you created 3 copies and only one is protected by that mutex.
The race is in resource management.

> Thanks,
> Vivek
>

Frediano




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]