linux-kernel - Re: [PATCH v2] um: read multiple msg from virtio slave request fd

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: <53bdf85f-8c29-7843-7ccd-88432e66b282@uni-rostock.de>
Date:   Wed, 27 Jul 2022 22:28:25 +0200
From:   Benjamin Beichler <Benjamin.Beichler@...-rostock.de>
To:     Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Johannes Berg <johannes@...solutions.net>
CC:     Johannes Berg <johannes.berg@...el.com>,
        <linux-um@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] um: read multiple msg from virtio slave request fd

Are there any issues with that patch?
I would be happy to receive any comments or an acceptance :-D

Sorry for my former HTML-Email.

kind regards

Benjamin


Am 07.06.2022 um 13:27 schrieb Benjamin Beichler:
> If VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS is activated, the user mode
> linux virtio irq handler only read one msg from the corresponding socket.
> This creates issues, when the device emulation creates multiple call
> requests (e.g. for multiple virtqueues), as the socket buffer tend to fill
> up and the call requests are delayed.
>
> This creates a deadlock situation, when the device simulation blocks,
> because of sending a msg and the kernel side blocks because of
> synchronously waiting for an acknowledge of kick request.
>
> Actually inband notifications are meant to be used in combination with the
> time travel protocol, but it is not required, therefore this corner case
> needs to be handled.
>
> Anyways, in general it seems to be more natural to consume always all
> messages from a socket, instead of only a single one.
>
> Fixes: 2cd097ba8c05 ("um: virtio: Implement VHOST_USER_PROTOCOL_F_SLAVE_REQ")
> Signed-off-by: Benjamin Beichler <benjamin.beichler@...-rostock.de>
> ---
>   arch/um/drivers/virtio_uml.c | 71 +++++++++++++++++++-----------------
>   1 file changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> index 82ff3785bf69..3716c5f6f9aa 100644
> --- a/arch/um/drivers/virtio_uml.c
> +++ b/arch/um/drivers/virtio_uml.c
> @@ -374,45 +374,48 @@ static irqreturn_t vu_req_read_message(struct virtio_uml_device *vu_dev,
>   		u8 extra_payload[512];
>   	} msg;
>   	int rc;
> +	irqreturn_t irq_rc = IRQ_NONE;
>   
> -	rc = vhost_user_recv_req(vu_dev, &msg.msg,
> -				 sizeof(msg.msg.payload) +
> -				 sizeof(msg.extra_payload));
> -
> -	vu_dev->recv_rc = rc;
> -	if (rc)
> -		return IRQ_NONE;
> -
> -	switch (msg.msg.header.request) {
> -	case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG:
> -		vu_dev->config_changed_irq = true;
> -		response = 0;
> -		break;
> -	case VHOST_USER_SLAVE_VRING_CALL:
> -		virtio_device_for_each_vq((&vu_dev->vdev), vq) {
> -			if (vq->index == msg.msg.payload.vring_state.index) {
> -				response = 0;
> -				vu_dev->vq_irq_vq_map |= BIT_ULL(vq->index);
> -				break;
> +	while (1) {
> +		rc = vhost_user_recv_req(vu_dev, &msg.msg,
> +					 sizeof(msg.msg.payload) +
> +					 sizeof(msg.extra_payload));
> +		if (rc)
> +			break;
> +
> +		switch (msg.msg.header.request) {
> +		case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG:
> +			vu_dev->config_changed_irq = true;
> +			response = 0;
> +			break;
> +		case VHOST_USER_SLAVE_VRING_CALL:
> +			virtio_device_for_each_vq((&vu_dev->vdev), vq) {
> +				if (vq->index == msg.msg.payload.vring_state.index) {
> +					response = 0;
> +					vu_dev->vq_irq_vq_map |= BIT_ULL(vq->index);
> +					break;
> +				}
>   			}
> +			break;
> +		case VHOST_USER_SLAVE_IOTLB_MSG:
> +			/* not supported - VIRTIO_F_ACCESS_PLATFORM */
> +		case VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG:
> +			/* not supported - VHOST_USER_PROTOCOL_F_HOST_NOTIFIER */
> +		default:
> +			vu_err(vu_dev, "unexpected slave request %d\n",
> +			       msg.msg.header.request);
>   		}
> -		break;
> -	case VHOST_USER_SLAVE_IOTLB_MSG:
> -		/* not supported - VIRTIO_F_ACCESS_PLATFORM */
> -	case VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG:
> -		/* not supported - VHOST_USER_PROTOCOL_F_HOST_NOTIFIER */
> -	default:
> -		vu_err(vu_dev, "unexpected slave request %d\n",
> -		       msg.msg.header.request);
> -	}
> -
> -	if (ev && !vu_dev->suspended)
> -		time_travel_add_irq_event(ev);
>   
> -	if (msg.msg.header.flags & VHOST_USER_FLAG_NEED_REPLY)
> -		vhost_user_reply(vu_dev, &msg.msg, response);
> +		if (ev && !vu_dev->suspended)
> +			time_travel_add_irq_event(ev);
>   
> -	return IRQ_HANDLED;
> +		if (msg.msg.header.flags & VHOST_USER_FLAG_NEED_REPLY)
> +			vhost_user_reply(vu_dev, &msg.msg, response);
> +		irq_rc = IRQ_HANDLED;
> +	};
> +	/* mask EAGAIN as we try non-blocking read until socket is empty */
> +	vu_dev->recv_rc = (rc == -EAGAIN) ? 0 : rc;
> +	return irq_rc;
>   }
>   
>   static irqreturn_t vu_req_interrupt(int irq, void *data)


-- 
M.Sc. Benjamin Beichler

Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik

University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE

Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany

phone: +49 (0) 381 498 - 7278
email: Benjamin.Beichler@...-rostock.de
www: http://www.imd.uni-rostock.de/

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5364 bytes)