Re: [PATCH net-next v9] virtio/vsock: replace virtio_vsock_pkt with sk_buff

On Tue, Jan 10, 2023 at 03:45:32PM +0100, Paolo Abeni wrote:

On Tue, 2023-01-10 at 09:36 +0100, Paolo Abeni wrote:

quoted

On Sat, 2023-01-07 at 00:29 +0000, Bobby Eshleman wrote:

quoted

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 <redacted>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Acked-by: Paolo Abeni <pabeni@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.

quoted

Changes in v9:
- check length in rx header
- guard alloactor from small requests
- squashed fix for v8 bug reported by syzbot:
    syzbot+30b72abaa17c07fe39dd@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) ?

Yep, the check is in virtio_vsock_alloc_skb() (a good central point that
both vhost/virtio call into), returning NULL and allocating nothing if
the size is nonsense.

Thanks,
Bobby