Re: [PATCH v8 7/8] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 15 Nov 2024 12:13:23 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 14/11/2024 15:23, Stefano Brivio wrote:
> > On Thu, 14 Nov 2024 11:29:36 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 17/10/2024 02:10, Stefano Brivio wrote:  
> >>>> +/**
> >>>> + * tcp_vu_prepare() - Prepare the packet header
> >>>> + * @c:		Execution context
> >>>> + * @conn:	Connection pointer
> >>>> + * @first:	Pointer to the array of IO vectors
> >>>> + * @dlen:	Packet data length
> >>>> + * @check:	Checksum, if already known
> >>>> + */
> >>>> +static void tcp_vu_prepare(const struct ctx *c,
> >>>> +			   struct tcp_tap_conn *conn, struct iovec *first,
> >>>> +			   size_t dlen, const uint16_t **check)
> >>>> +{
> >>>> +	const struct flowside *toside = TAPFLOW(conn);
> >>>> +	char *base = first->iov_base;
> >>>> +	struct ethhdr *eh;
> >>>> +
> >>>> +	/* we guess the first iovec provided by the guest can embed
> >>>> +	 * all the headers needed by L2 frame
> >>>> +	 */  
> >>> What happens if it doesn't (buggy guest)? Do we have a way to make sure
> >>> it's the case? I guess it's more straightforward to do this in
> >>> tcp_vu_data_from_sock() where we check and set iov_len (even though the
> >>> implication of VIRTIO_NET_F_MRG_RXBUF isn't totally clear to me).  
> >>
> >> According to spec, minimum size of a buffer is 1526 bytes
> >> (https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2340003)
> >>
> >> So if the guest is buggy, we will write in the guest memory out of the (buggy) provided
> >> buffer, and we can crash the guest. But it's what happens to a buggy guest.
> >>
> >> We can't fix the guest, IMO passt should crash in this case (add an ASSERT()?).  
> > 
> > I think we should rather call tap_sock_reset() (see tap_passt_input())
> > so that the guest has a chance to reconnect (you implemented this in
> > QEMU...).
> 
> If I add a call to tap_sock_reset() in tcp_vu_data_from_sock(), I need to remove all the 
> "const" from "struct ctx *" in the entire call chain. It's a lot. I'm not sure it is worth it.

...or you drop c->fd_tap = -1 from tap_sock_reset(). At a glance it
looks fine to me, we never check it afterwards, but please double
check. :)

If that's not feasible, yeah, I guess an ASSERT() will do for the
moment.

-- 
Stefano