Re: [PULL v2 2/5] hw/xen: Expose handle_bufioreq in xen_register_ioreq

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>]

On Mon, Oct 07, 2024 at 04:42:49PM +0100, Peter Maydell wrote:
> On Thu, 3 Oct 2024 at 19:57, Edgar E. Iglesias <edgar.iglesias@xxxxxxxxx> 
> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
> >
> > Expose handle_bufioreq in xen_register_ioreq().
> > This is to allow machines to enable or disable buffered ioreqs.
> >
> > No functional change since all callers still set it to
> > HVM_IOREQSRV_BUFIOREQ_ATOMIC.
> >
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> 
> Hi; Coverity has noticed a problem (CID 1563383) with this change:
> 
> > diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> > index 3a9d6f981b..7d2b72853b 100644
> > --- a/hw/xen/xen-hvm-common.c
> > +++ b/hw/xen/xen-hvm-common.c
> > @@ -667,6 +667,8 @@ static int xen_map_ioreq_server(XenIOState *state)
> >      xen_pfn_t ioreq_pfn;
> >      xen_pfn_t bufioreq_pfn;
> >      evtchn_port_t bufioreq_evtchn;
> 
> In this function bufioreq_evtchn is declared uninitialized...
> 
> > +    unsigned long num_frames = 1;
> > +    unsigned long frame = 1;
> >      int rc;
> >
> >      /*
> > @@ -675,59 +677,78 @@ static int xen_map_ioreq_server(XenIOState *state)
> >       */
> >      QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq != 0);
> >      QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1);
> > +
> > +    if (state->has_bufioreq) {
> > +        frame = 0;
> > +        num_frames = 2;
> > +    }
> >      state->fres = xenforeignmemory_map_resource(xen_fmem, xen_domid,
> >                                           XENMEM_resource_ioreq_server,
> > -                                         state->ioservid, 0, 2,
> > +                                         state->ioservid,
> > +                                         frame, num_frames,
> >                                           &addr,
> >                                           PROT_READ | PROT_WRITE, 0);
> >      if (state->fres != NULL) {
> >          trace_xen_map_resource_ioreq(state->ioservid, addr);
> > -        state->buffered_io_page = addr;
> > -        state->shared_page = addr + XC_PAGE_SIZE;
> > +        state->shared_page = addr;
> > +        if (state->has_bufioreq) {
> > +            state->buffered_io_page = addr;
> > +            state->shared_page = addr + XC_PAGE_SIZE;
> > +        }
> >      } else if (errno != EOPNOTSUPP) {
> >          error_report("failed to map ioreq server resources: error %d 
> > handle=%p",
> >                       errno, xen_xc);
> >          return -1;
> >      }
> >
> > -    rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
> > -                                   (state->shared_page == NULL) ?
> > -                                   &ioreq_pfn : NULL,
> > -                                   (state->buffered_io_page == NULL) ?
> > -                                   &bufioreq_pfn : NULL,
> > -                                   &bufioreq_evtchn);
> 
> ...which was OK prior to this change, because (ignoring the
> early-exit case) we would always pass through this function
> call, which initializes bufioreq_evtchn...
> 
> > -    if (rc < 0) {
> > -        error_report("failed to get ioreq server info: error %d handle=%p",
> > -                     errno, xen_xc);
> > -        return rc;
> > -    }
> > +    /*
> > +     * If we fail to map the shared page with 
> > xenforeignmemory_map_resource()
> > +     * or if we're using buffered ioreqs, we need 
> > xen_get_ioreq_server_info()
> > +     * to provide the the addresses to map the shared page and/or to get 
> > the
> > +     * event-channel port for buffered ioreqs.
> > +     */
> > +    if (state->shared_page == NULL || state->has_bufioreq) {
> > +        rc = xen_get_ioreq_server_info(xen_domid, state->ioservid,
> > +                                       (state->shared_page == NULL) ?
> > +                                       &ioreq_pfn : NULL,
> > +                                       (state->has_bufioreq &&
> > +                                        state->buffered_io_page == NULL) ?
> > +                                       &bufioreq_pfn : NULL,
> > +                                       &bufioreq_evtchn);
> 
> ...but now the initialization has moved inside an if() so it only
> happens under certain conditions...
> 
> > +        if (rc < 0) {
> > +            error_report("failed to get ioreq server info: error %d 
> > handle=%p",
> > +                         errno, xen_xc);
> > +            return rc;
> > +        }
> >
> > -    if (state->shared_page == NULL) {
> > -        trace_xen_map_ioreq_server_shared_page(ioreq_pfn);
> > +        if (state->shared_page == NULL) {
> > +            trace_xen_map_ioreq_server_shared_page(ioreq_pfn);
> >
> > -        state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
> > -                                                  PROT_READ | PROT_WRITE,
> > -                                                  1, &ioreq_pfn, NULL);
> > +            state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
> > +                                                      PROT_READ | 
> > PROT_WRITE,
> > +                                                      1, &ioreq_pfn, NULL);
> > +        }
> >          if (state->shared_page == NULL) {
> >              error_report("map shared IO page returned error %d handle=%p",
> >                           errno, xen_xc);
> >          }
> > -    }
> >
> > -    if (state->buffered_io_page == NULL) {
> > -        trace_xen_map_ioreq_server_buffered_io_page(bufioreq_pfn);
> > +        if (state->has_bufioreq && state->buffered_io_page == NULL) {
> > +            trace_xen_map_ioreq_server_buffered_io_page(bufioreq_pfn);
> >
> > -        state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid,
> > -                                                       PROT_READ | 
> > PROT_WRITE,
> > -                                                       1, &bufioreq_pfn,
> > -                                                       NULL);
> > -        if (state->buffered_io_page == NULL) {
> > -            error_report("map buffered IO page returned error %d", errno);
> > -            return -1;
> > +            state->buffered_io_page = xenforeignmemory_map(xen_fmem, 
> > xen_domid,
> > +                                                        PROT_READ | 
> > PROT_WRITE,
> > +                                                        1, &bufioreq_pfn,
> > +                                                        NULL);
> > +            if (state->buffered_io_page == NULL) {
> > +                error_report("map buffered IO page returned error %d", 
> > errno);
> > +                return -1;
> > +            }
> >          }
> >      }
> >
> > -    if (state->shared_page == NULL || state->buffered_io_page == NULL) {
> > +    if (state->shared_page == NULL ||
> > +        (state->has_bufioreq && state->buffered_io_page == NULL)) {
> >          return -1;
> >      }
> 
> ...and the tail end of the function has not been modified, so
> (not visible in this diff context) when we do:
> 
>     trace_xen_map_ioreq_server_buffered_io_evtchn(bufioreq_evtchn);
> 
>     state->bufioreq_remote_port = bufioreq_evtchn;
> 
>     return 0;
> 
> we may be using it uninitialized (in the trace statement
> and when assigning it to state->bufioreq_remote_port).
> 
> Could you have a look at this and send a fix, please?
>

Thanks Peter,

I sent a fix for this yesterday.

Best regards,
Edgar