public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: passt-dev@passt.top
Subject: Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
Date: Fri, 09 Sep 2022 18:06:59 +0200	[thread overview]
Message-ID: <20220909180659.7b6fe407@elisabeth> (raw)
In-Reply-To: <YxsX8IFvOePGi/xE@yekko>

[-- Attachment #1: Type: text/plain, Size: 3967 bytes --]

On Fri, 9 Sep 2022 20:39:44 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> > On Fri,  9 Sep 2022 14:27:13 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > udp_tap_handler() currently skips outbound packets if they have a payload
> > > length of zero.  This is not correct, since in a datagram protocol zero
> > > length packets still have meaning.  
> > 
> > Right, nice catch. As far as I can tell it's an issue I added with
> > commit bb708111833e ("treewide: Packet abstraction with mandatory
> > boundary checks").
> >   
> > > Adjust this to correctly forward the zero-length packets by using a msghdr
> > > with msg_iovlen == 0.
> > > 
> > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> > > 
> > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > ---
> > >  udp.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/udp.c b/udp.c
> > > index c4ebecc..caa852a 100644
> > > --- a/udp.c
> > > +++ b/udp.c
> > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > >  		uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > >  		if (!uh_send)
> > >  			return p->count;
> > > +
> > > +		mm[i].msg_hdr.msg_name = sa;
> > > +		mm[i].msg_hdr.msg_namelen = sl;
> > > +		count++;
> > > +
> > >  		if (!len)
> > >  			continue;
> > >  
> > >  		m[i].iov_base = (char *)(uh_send + 1);
> > >  		m[i].iov_len = len;  
> > 
> > I haven't tested this yet, but:
> > 
> > - shouldn't iov_len be set to 0 (moving also this line before)? Note
> >   that I'm not initialising m
> > 
> > - shouldn't iov_base point to NULL to avoid noise from valgrind?  
> 
> No, because with this change m[i] is entirely unreferenced by mm[].
> 
> > Also:
> >   
> > >  
> > > -		mm[i].msg_hdr.msg_name = sa;
> > > -		mm[i].msg_hdr.msg_namelen = sl;
> > > -
> > >  		mm[i].msg_hdr.msg_iov = m + i;
> > >  		mm[i].msg_hdr.msg_iovlen = 1;  
> > 
> > ...I guess we should still go through those even if the size is zero,
> > because we're appending a message. If we don't, I would expect some
> > subsequent messages in the batch to be dropped (as many as zero sized
> > packets we have).  
> 
> Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
> so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.

> I was looking at removing that initialization, but I haven't gotten
> that working yet.

Oops, I see now.

So, I suppose that if you want to drop that initialisation, you might
need to zero msg_hdr.controllen as well.

And msg_hdr.control too: other than keeping valgrind happy, not leaking
random stuff to the kernel might make this marginally more secure.

That should be better than the huge memset() at the beginning, because
we're already writing to msg_iovlen anyway.

If you already tried that, though, I don't have any other quick idea.

By the way, I had a mechanism in place, just for TCP though, to avoid
reassigning those pointers and also length descriptors.

I got rid of it in commit 38fbfdbcb95d ("tcp: Get rid of iov with
cached MSS, drop sendmmsg(), add deferred flush") because it didn't
really help with throughput. I don't see any significant "userspace"
overhead on guest-to-host TCP paths with perf(1).

...maybe for UDP that's different, I haven't focused that much on UDP
performance.

> > That is, I suppose we could just drop the continue statement on if
> > (!len) above -- but, again, I haven't tested it.  
> 
> My first version actually did that, so it also works, but I think
> setting msg_iovlen to 0 is a bit neater.

Right. Maybe it was just me being thick, or perhaps that could use a
comment:

		/* Zero-length packet: don't use any buffer, msg_iovlen is 0 */
		if (!len)
			continue;

-- 
Stefano


  reply	other threads:[~2022-09-09 16:06 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-09  4:27 [PATCH 0/2] Don't drop outbound zero-length UDP packets over tap David Gibson
2022-09-09  4:27 ` [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets David Gibson
2022-09-09  9:26   ` Stefano Brivio
2022-09-09 10:39     ` David Gibson
2022-09-09 16:06       ` Stefano Brivio [this message]
2022-09-13  6:39         ` David Gibson
2022-09-13  9:08           ` Stefano Brivio
2022-09-13  9:30             ` David Gibson
2022-09-09  4:27 ` [PATCH 2/2] test: Simpler termination handling for UDP tests David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220909180659.7b6fe407@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).