linux-kernel - Re: [PATCH v4 11/13] xen/pvcalls: implement poll command

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.10.1710061515060.3073@sstabellini-ThinkPad-X260>
Date:   Fri, 6 Oct 2017 16:33:48 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Andrea Parri <parri.andrea@...il.com>
cc:     Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel@...ts.xen.org, linux-kernel@...r.kernel.org,
        jgross@...e.com, boris.ostrovsky@...cle.com,
        Stefano Stabellini <stefano@...reto.com>
Subject: Re: [PATCH v4 11/13] xen/pvcalls: implement poll command

On Tue, 19 Sep 2017, Andrea Parri wrote:
> Hi Stefano,
> 
> On Fri, Sep 15, 2017 at 04:00:38PM -0700, Stefano Stabellini wrote:
> > For active sockets, check the indexes and use the inflight_conn_req
> > waitqueue to wait.
> > 
> > For passive sockets if an accept is outstanding
> > (PVCALLS_FLAG_ACCEPT_INFLIGHT), check if it has been answered by looking
> > at bedata->rsp[req_id]. If so, return POLLIN.  Otherwise use the
> > inflight_accept_req waitqueue.
> > 
> > If no accepts are inflight, send PVCALLS_POLL to the backend. If we have
> > outstanding POLL requests awaiting for a response use the inflight_req
> > waitqueue: inflight_req is awaken when a new response is received; on
> > wakeup we check whether the POLL response is arrived by looking at the
> > PVCALLS_FLAG_POLL_RET flag. We set the flag from
> > pvcalls_front_event_handler, if the response was for a POLL command.
> > 
> > In pvcalls_front_event_handler, get the struct sock_mapping from the
> > poll id (we previously converted struct sock_mapping* to uint64_t and
> > used it as id).
> > 
> > Signed-off-by: Stefano Stabellini <stefano@...reto.com>
> > CC: boris.ostrovsky@...cle.com
> > CC: jgross@...e.com
> > ---
> >  drivers/xen/pvcalls-front.c | 144 +++++++++++++++++++++++++++++++++++++++++---
> >  drivers/xen/pvcalls-front.h |   3 +
> >  2 files changed, 138 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > index 01a5a69..8a90213 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -85,6 +85,8 @@ struct sock_mapping {
> >  		 * Only one poll operation can be inflight for a given socket.
> >  		 */
> >  #define PVCALLS_FLAG_ACCEPT_INFLIGHT 0
> > +#define PVCALLS_FLAG_POLL_INFLIGHT   1
> > +#define PVCALLS_FLAG_POLL_RET        2
> >  			uint8_t flags;
> >  			uint32_t inflight_req_id;
> >  			struct sock_mapping *accept_map;
> > @@ -155,15 +157,32 @@ static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> >  		rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
> >  
> >  		req_id = rsp->req_id;
> > -		dst = (uint8_t *)&bedata->rsp[req_id] + sizeof(rsp->req_id);
> > -		src = (uint8_t *)rsp + sizeof(rsp->req_id);
> > -		memcpy(dst, src, sizeof(*rsp) - sizeof(rsp->req_id));
> > -		/*
> > -		 * First copy the rest of the data, then req_id. It is
> > -		 * paired with the barrier when accessing bedata->rsp.
> > -		 */
> > -		smp_wmb();
> > -		WRITE_ONCE(bedata->rsp[req_id].req_id, rsp->req_id);
> > +		if (rsp->cmd == PVCALLS_POLL) {
> > +			struct sock_mapping *map = (struct sock_mapping *)
> > +						   rsp->u.poll.id;
> > +
> > +			set_bit(PVCALLS_FLAG_POLL_RET,
> > +				(void *)&map->passive.flags);
> > +			/*
> > +			 * Set RET, then clear INFLIGHT. It pairs with
> > +			 * the checks at the beginning of
> > +			 * pvcalls_front_poll_passive.
> > +			 */
> > +			smp_wmb();
> 
> pvcalls_front_poll_passive() seems to first check RET, then INFLIGHT
> (no "crossing of mem. locations"): can you elaborate here?

Thank you for the review! It looks like you spotted to errors. I think
heree should be

  clear INFLIGHT
  smp_wmb()
  set RET

to pair properly.


> > +			clear_bit(PVCALLS_FLAG_POLL_INFLIGHT,
> > +				  (void *)&map->passive.flags);
> > +		} else {
> > +			dst = (uint8_t *)&bedata->rsp[req_id] +
> > +			      sizeof(rsp->req_id);
> > +			src = (uint8_t *)rsp + sizeof(rsp->req_id);
> > +			memcpy(dst, src, sizeof(*rsp) - sizeof(rsp->req_id));
> > +			/*
> > +			 * First copy the rest of the data, then req_id. It is
> > +			 * paired with the barrier when accessing bedata->rsp.
> > +			 */
> > +			smp_wmb();
> 
> Would you point me to the "pairing barrier"? (not sure I understand
> the logic here...)

On this side (pvcalls_front_event_handler):
    write content
    smp_wmb
    write id

On the other side (all command functions, for example pvcalls_front_socket)
    read id (passed to wait_event)
    smp_rmb() <--- this is missing and should pair with the above smp_wmb
    read content
    smp_mb() (*)
    clear id
 
(*) this is unnecessary and has no pairing, it was supposed to protect
against the content being overwritten before the read is complete,
because after clearing the id the slot could be reused. get_request
returns an id, but the content is not overwritten immediately. The id is
passed to the backend, and only upon receiving an answer the content
gets overwritten. I think it is unnecessary and I could remove it or
turn it into a compiler barrier.


> > +			WRITE_ONCE(bedata->rsp[req_id].req_id, rsp->req_id);
> 
> Could this be rewritten as
> 
>    WRITE_ONCE(bedata->rsp[req_id].req_id, req_id);
 
Yes, I'll change it

 
> > +		}
> >  
> >  		done = 1;
> >  		bedata->ring.rsp_cons++;