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

[Kasireddy, Vivek]

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 at intel.com> 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 at redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> > Cc: Dongwon Kim <dongwon.kim at intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> > ---
> >  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?

Thanks,
Vivek

> 
> Frediano