Subject: Re: [PATCH v4 12/13] xen/pvcalls: implement release command
To: Stefano Stabellini <sstabellini@kernel.org>, xen-devel@lists.xen.org
References: <alpine.DEB.2.10.1709121550270.9439@sstabellini-ThinkPad-X260>
 <1505516440-11111-1-git-send-email-sstabellini@kernel.org>
 <1505516440-11111-12-git-send-email-sstabellini@kernel.org>
Cc: linux-kernel@vger.kernel.org, jgross@suse.com,
        Stefano Stabellini <stefano@aporeto.com>
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <fe455763-4ced-0679-85ce-4f1d59c96f61@oracle.com>
Date: Fri, 22 Sep 2017 18:43:07 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <1505516440-11111-12-git-send-email-sstabellini@kernel.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3179
Lines: 118


>  
> +static void pvcalls_front_free_map(struct pvcalls_bedata *bedata,
> +				   struct sock_mapping *map)

I just noticed: pvcalls_front_free_map() is referenced by patches 2 and 8.

> +{
> +	int i;
> +
> +	unbind_from_irqhandler(map->active.irq, map);
> +
> +	spin_lock(&bedata->socket_lock);
> +	if (!list_empty(&map->list))
> +		list_del_init(&map->list);
> +	spin_unlock(&bedata->socket_lock);
> +
> +	for (i = 0; i < (1 << PVCALLS_RING_ORDER); i++)
> +		gnttab_end_foreign_access(map->active.ring->ref[i], 0, 0);
> +	gnttab_end_foreign_access(map->active.ref, 0, 0);
> +	free_page((unsigned long)map->active.ring);
> +}
> +
>  int pvcalls_front_socket(struct socket *sock)
>  {
>  	struct pvcalls_bedata *bedata;
> @@ -960,6 +978,92 @@ unsigned int pvcalls_front_poll(struct file *file, struct socket *sock,
>  	return ret;
>  }
>  
> +int pvcalls_front_release(struct socket *sock)
> +{
> +	struct pvcalls_bedata *bedata;
> +	struct sock_mapping *map;
> +	int req_id, notify, ret;
> +	struct xen_pvcalls_request *req;
> +
> +	pvcalls_enter;
> +	if (!pvcalls_front_dev) {
> +		pvcalls_exit;
> +		return -EIO;
> +	}
> +	if (sock->sk == NULL) {
> +		pvcalls_exit;
> +		return 0;
> +	}
> +
> +	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
> +
> +	map = (struct sock_mapping *) sock->sk->sk_send_head;
> +	if (map == NULL) {
> +		pvcalls_exit;
> +		return 0;
> +	}
> +
> +	spin_lock(&bedata->socket_lock);
> +	ret = get_request(bedata, &req_id);
> +	if (ret < 0) {
> +		spin_unlock(&bedata->socket_lock);
> +		pvcalls_exit;
> +		return ret;
> +	}
> +	sock->sk->sk_send_head = NULL;
> +
> +	req = RING_GET_REQUEST(&bedata->ring, req_id);
> +	req->req_id = req_id;
> +	req->cmd = PVCALLS_RELEASE;
> +	req->u.release.id = (uint64_t)map;
> +
> +	bedata->ring.req_prod_pvt++;
> +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
> +	spin_unlock(&bedata->socket_lock);
> +	if (notify)
> +		notify_remote_via_irq(bedata->irq);
> +
> +	wait_event(bedata->inflight_req,
> +		   READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
> +
> +	if (map->active_socket) {
> +		/*
> +		 * Set in_error and wake up inflight_conn_req to force
> +		 * recvmsg waiters to exit.
> +		 */
> +		map->active.ring->in_error = -EBADF;
> +		wake_up_interruptible(&map->active.inflight_conn_req);
> +
> +		/*
> +		 * Wait until there are no more waiters on the mutexes.
> +		 * We know that no new waiters can be added because sk_send_head
> +		 * is set to NULL -- we only need to wait for the existing
> +		 * waiters to return.
> +		 */
> +		while (!mutex_trylock(&map->active.in_mutex) ||
> +			   !mutex_trylock(&map->active.out_mutex))
> +			cpu_relax();


What if you manage to grab the locks before waiters get to run? for
example, in recvmsg:

	while (!(flags & MSG_DONTWAIT) && !pvcalls_front_read_todo(map)) {
		wait_event_interruptible(map->active.inflight_conn_req,
					 pvcalls_front_read_todo(map));
	}
	ret = __read_ring(map->active.ring, &map->active.data,
			  &msg->msg_iter, len, flags);

map will be freed (by pvcalls_front_free_map() below) before __read_ring
is passed the just-freed ring.


> +
> +		pvcalls_front_free_map(bedata, map);
> +		kfree(map);


-boris