linux-kernel - Re: [PATCH v2 06/18] xen/pvcalls: handle commands from the frontend

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: <73d522d3-bec2-09ef-e8ab-b2014ff87d5d@oracle.com>
Date:   Fri, 26 May 2017 11:32:11 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel@...ts.xen.org
Cc:     linux-kernel@...r.kernel.org, jgross@...e.com,
        Stefano Stabellini <stefano@...reto.com>
Subject: Re: [PATCH v2 06/18] xen/pvcalls: handle commands from the frontend

On 05/19/2017 07:22 PM, Stefano Stabellini wrote:
> +
>  static void pvcalls_back_work(struct work_struct *work)
>  {
> +	struct pvcalls_back_priv *priv = container_of(work,
> +		struct pvcalls_back_priv, register_work);
> +	int notify, notify_all = 0, more = 1;
> +	struct xen_pvcalls_request req;
> +	struct xenbus_device *dev = priv->dev;
> +
> +	atomic_set(&priv->work, 1);
> +
> +	while (more || !atomic_dec_and_test(&priv->work)) {
> +		while (RING_HAS_UNCONSUMED_REQUESTS(&priv->ring)) {
> +			RING_COPY_REQUEST(&priv->ring,
> +					  priv->ring.req_cons++,
> +					  &req);
> +
> +			if (!pvcalls_back_handle_cmd(dev, &req)) {
> +				RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(
> +					&priv->ring, notify);
> +				notify_all += notify;
> +			}
> +		}
> +
> +		if (notify_all)
> +			notify_remote_via_irq(priv->irq);
> +
> +		RING_FINAL_CHECK_FOR_REQUESTS(&priv->ring, more);
> +	}
>  }
>  
>  static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
>  {
> +	struct xenbus_device *dev = dev_id;
> +	struct pvcalls_back_priv *priv = NULL;
> +
> +	if (dev == NULL)
> +		return IRQ_HANDLED;
> +
> +	priv = dev_get_drvdata(&dev->dev);
> +	if (priv == NULL)
> +		return IRQ_HANDLED;
> +
> +	atomic_inc(&priv->work);

I will paste you response here from v1 --- I thought I understood it and
now I don't anymore.

>>
>> Is this really needed? We have a new entry on the ring, so the outer
loop in
>> pvcalls_back_work() will pick this up (by setting 'more').
>
> This is to avoid race conditions. A notification could be delivered
> after RING_FINAL_CHECK_FOR_REQUESTS is called, returning more == 0, but
> before pvcalls_back_work completes. In that case, without priv->work,
> pvcalls_back_work wouldn't be rescheduled because it is still running
> and the work would be left undone.


How is this different from the case when new work comes after the outer
loop is done but we still haven't returned from pvcalls_back_work()?

-boris

> +	queue_work(priv->wq, &priv->register_work);
> +
>  	return IRQ_HANDLED;
>  }
>