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?
Thanks,
Vivek
>
> Frediano