linux-kernel - Re: [PATCH 5/7] xen/9pfs: send requests to the backend

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]

Date:   Wed, 8 Mar 2017 11:33:16 -0800 (PST)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
cc:     Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        Stefano Stabellini <stefano@...reto.com>, jgross@...e.com,
        Eric Van Hensbergen <ericvh@...il.com>,
        Ron Minnich <rminnich@...dia.gov>,
        Latchesar Ionkov <lucho@...kov.net>,
        v9fs-developer@...ts.sourceforge.net
Subject: Re: [PATCH 5/7] xen/9pfs: send requests to the backend

On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> +}
> >>> +
> >>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >>> +{
> >>> +	RING_IDX cons, prod;
> >>> +
> >>> +	cons = ring->intf->out_cons;
> >>> +	prod = ring->intf->out_prod;
> >>> +	mb();
> >>> +
> >>> +	if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> >>> +		return 1;
> >>> +	else
> >>> +		return 0;
> >>>  }
> >>>  
> >>>  static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >>>  {
> >>> +	struct xen_9pfs_front_priv *priv = NULL;
> >>> +	RING_IDX cons, prod, masked_cons, masked_prod;
> >>> +	unsigned long flags;
> >>> +	uint32_t size = p9_req->tc->size;
> >>> +	struct xen_9pfs_dataring *ring;
> >>> +	int num;
> >>> +
> >>> +	list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>> +		if (priv->client == client)
> >>> +			break;
> >>> +	}
> >>> +	if (priv == NULL || priv->client != client)
> >>> +		return -EINVAL;
> >>> +
> >>> +	num = p9_req->tc->tag % priv->num_rings;
> >>> +	ring = &priv->rings[num];
> >>> +
> >>> +again:
> >>> +	while (wait_event_interruptible(ring->wq,
> >>> +				p9_xen_write_todo(ring, size) > 0) != 0);
> >>> +
> >>> +	spin_lock_irqsave(&ring->lock, flags);
> >>> +	cons = ring->intf->out_cons;
> >>> +	prod = ring->intf->out_prod;
> >>> +	mb();
> >>> +
> >>> +	if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
> >>
> >> This looks like p9_xen_write_todo().
> > p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> > a return value that works well with wait_event_interruptible.
> >
> > I would prefer not to call p9_xen_write_todo here, because it's simpler
> > if we don't read prod and cons twice.
> 
> I was referring to the whole code fragment after spin_lock_irqsave(),
> not just the last line. Isn't it exactly !p9_xen_write_todo()?

Yes, it is true they are almost the same. The difference, and the reason
for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
the wait_event_interruptible loop, as such it needs to read prod and
cons every time. On the other end, here we want to read them once. Does
it make sense?