Virtualization - Jan 2023 - [PATCH net-next v9] virtio/vsock: replace virtio_vsock_pkt with sk_buff

On Sat, 2023-01-07 at 00:29 +0000, Bobby Eshleman wrote:
> This commit changes virtio/vsock to use sk_buff instead of
> virtio_vsock_pkt. Beyond better conforming to other net code, using
> sk_buff allows vsock to use sk_buff-dependent features in the future
> (such as sockmap) and improves throughput.
> 
> This patch introduces the following performance changes:
> 
> Tool/Config: uperf w/ 64 threads, SOCK_STREAM
> Test Runs: 5, mean of results
> Before: commit 95ec6bce2a0b ("Merge branch
'net-ipa-more-endpoints'")
> 
> Test: 64KB, g2h
> Before: 21.63 Gb/s
> After: 25.59 Gb/s (+18%)
> 
> Test: 16B, g2h
> Before: 11.86 Mb/s
> After: 17.41 Mb/s (+46%)
> 
> Test: 64KB, h2g
> Before: 2.15 Gb/s
> After: 3.6 Gb/s (+67%)
> 
> Test: 16B, h2g
> Before: 14.38 Mb/s
> After: 18.43 Mb/s (+28%)
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman at bytedance.com>
> Reviewed-by: Stefano Garzarella <sgarzare at redhat.com>
> Acked-by: Paolo Abeni <pabeni at redhat.com>
> ---
> 
> Tested using vsock_test g2h and h2g.  I'm not sure if it is standard
> practice here to carry Acks and Reviews forward to future versions, but
> I'm doing that here to hopefully make life easier for maintainers.
> Please let me know if it is not standard practice.
As Jakub noted, there is no clear rule for tag passing across different
patch revisions.

Here, given the complexity of the patch and the not trivial list of
changes, I would have preferred you would have dropped my tag.
> Changes in v9:
> - check length in rx header
> - guard alloactor from small requests
> - squashed fix for v8 bug reported by syzbot:
>     syzbot+30b72abaa17c07fe39dd at syzkaller.appspotmail.com
It's not clear to me what/where is the fix exactly, could you please
clarify?

Thanks!

Paolo

On Tue, 2023-01-10 at 09:36 +0100, Paolo Abeni wrote:
> On Sat, 2023-01-07 at 00:29 +0000, Bobby Eshleman wrote:
> > This commit changes virtio/vsock to use sk_buff instead of
> > virtio_vsock_pkt. Beyond better conforming to other net code, using
> > sk_buff allows vsock to use sk_buff-dependent features in the future
> > (such as sockmap) and improves throughput.
> > 
> > This patch introduces the following performance changes:
> > 
> > Tool/Config: uperf w/ 64 threads, SOCK_STREAM
> > Test Runs: 5, mean of results
> > Before: commit 95ec6bce2a0b ("Merge branch
'net-ipa-more-endpoints'")
> > 
> > Test: 64KB, g2h
> > Before: 21.63 Gb/s
> > After: 25.59 Gb/s (+18%)
> > 
> > Test: 16B, g2h
> > Before: 11.86 Mb/s
> > After: 17.41 Mb/s (+46%)
> > 
> > Test: 64KB, h2g
> > Before: 2.15 Gb/s
> > After: 3.6 Gb/s (+67%)
> > 
> > Test: 16B, h2g
> > Before: 14.38 Mb/s
> > After: 18.43 Mb/s (+28%)
> > 
> > Signed-off-by: Bobby Eshleman <bobby.eshleman at bytedance.com>
> > Reviewed-by: Stefano Garzarella <sgarzare at redhat.com>
> > Acked-by: Paolo Abeni <pabeni at redhat.com>
> > ---
> > 
> > Tested using vsock_test g2h and h2g.  I'm not sure if it is
standard
> > practice here to carry Acks and Reviews forward to future versions,
but
> > I'm doing that here to hopefully make life easier for maintainers.
> > Please let me know if it is not standard practice.
> 
> As Jakub noted, there is no clear rule for tag passing across different
> patch revisions.
> 
> Here, given the complexity of the patch and the not trivial list of
> changes, I would have preferred you would have dropped my tag.
> 
> > Changes in v9:
> > - check length in rx header
> > - guard alloactor from small requests
> > - squashed fix for v8 bug reported by syzbot:
> >     syzbot+30b72abaa17c07fe39dd at syzkaller.appspotmail.com
> 
> It's not clear to me what/where is the fix exactly, could you please
> clarify?
Reading the syzkaller report, it looks like iov_length() in
vhost_vsock_alloc_pkt() can not be trusted to carry a reasonable value.

As such, don't you additionally need to ensure/check that iov_length()
is greater or equal to sizeof(virtio_vsock_hdr) ?

Thanks.

Paolo