netdev - Re: [PATCH v3 1/5] net: add header len parameter to tun_get_socket(), tap_get

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d291fdafe4bb2ee5c1f272b990784894f03894fd.camel@infradead.org>
Date:   Fri, 25 Jun 2021 19:55:30 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     netdev@...r.kernel.org, Jason Wang <jasowang@...hat.com>,
        Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH v3 1/5] net: add header len parameter to
 tun_get_socket(), tap_get_socket()

On Fri, 2021-06-25 at 14:13 -0400, Willem de Bruijn wrote:
> On Thu, Jun 24, 2021 at 8:30 AM David Woodhouse <dwmw2@...radead.org>
> wrote:
> > 
> > From: David Woodhouse <dwmw@...zon.co.uk>
> > 
> > The vhost-net driver was making wild assumptions about the header
> > length
> 
> If respinning, please more concretely describe which configuration is
> currently broken.

Fairly much all of them. Here's a test run on the 5.12.8 kernel:

$ sudo ./test_vhost_net 
TEST: (hdr 0, xdp 0, pi 0, features 0) RESULT: -1
TEST: (hdr 10, xdp 0, pi 0, features 0) RESULT: 0
TEST: (hdr 12, xdp 0, pi 0, features 0) RESULT: -1
TEST: (hdr 20, xdp 0, pi 0, features 0) RESULT: -1
TEST: (hdr 0, xdp 1, pi 0, features 0) RESULT: -1
TEST: (hdr 10, xdp 1, pi 0, features 0) RESULT: -1
TEST: (hdr 12, xdp 1, pi 0, features 0) RESULT: -1
TEST: (hdr 20, xdp 1, pi 0, features 0) RESULT: -1
TEST: (hdr 0, xdp 0, pi 1, features 0) RESULT: -1
TEST: (hdr 10, xdp 0, pi 1, features 0) RESULT: -1
TEST: (hdr 12, xdp 0, pi 1, features 0) RESULT: -1
TEST: (hdr 20, xdp 0, pi 1, features 0) RESULT: -1
TEST: (hdr 0, xdp 1, pi 1, features 0) RESULT: -1
TEST: (hdr 10, xdp 1, pi 1, features 0) RESULT: -1
TEST: (hdr 12, xdp 1, pi 1, features 0) RESULT: -1
TEST: (hdr 20, xdp 1, pi 1, features 0) RESULT: -1
TEST: (hdr 0, xdp 0, pi 0, features 100000000) RESULT: -1
TEST: (hdr 10, xdp 0, pi 0, features 100000000) RESULT: -1
TEST: (hdr 12, xdp 0, pi 0, features 100000000) RESULT: 0
TEST: (hdr 20, xdp 0, pi 0, features 100000000) RESULT: -1
TEST: (hdr 0, xdp 1, pi 0, features 100000000) RESULT: -1
TEST: (hdr 10, xdp 1, pi 0, features 100000000) RESULT: -1
TEST: (hdr 12, xdp 1, pi 0, features 100000000) RESULT: -1
TEST: (hdr 20, xdp 1, pi 0, features 100000000) RESULT: -1
TEST: (hdr 0, xdp 0, pi 1, features 100000000) RESULT: -1
TEST: (hdr 10, xdp 0, pi 1, features 100000000) RESULT: -1
TEST: (hdr 12, xdp 0, pi 1, features 100000000) RESULT: -1
TEST: (hdr 20, xdp 0, pi 1, features 100000000) RESULT: -1
TEST: (hdr 0, xdp 1, pi 1, features 100000000) RESULT: -1
TEST: (hdr 10, xdp 1, pi 1, features 100000000) RESULT: -1
TEST: (hdr 12, xdp 1, pi 1, features 100000000) RESULT: -1
TEST: (hdr 20, xdp 1, pi 1, features 100000000) RESULT: -1
TEST: (hdr 0, xdp 0, pi 0, features 8000000) RESULT: 0
TEST: (hdr 0, xdp 1, pi 0, features 8000000) RESULT: -1
TEST: (hdr 0, xdp 0, pi 1, features 8000000) RESULT: -1
TEST: (hdr 0, xdp 1, pi 1, features 8000000) RESULT: -1
TEST: (hdr 0, xdp 0, pi 0, features 108000000) RESULT: 0
TEST: (hdr 0, xdp 1, pi 0, features 108000000) RESULT: -1
TEST: (hdr 0, xdp 0, pi 1, features 108000000) RESULT: -1
TEST: (hdr 0, xdp 1, pi 1, features 108000000) RESULT: -1

> IFF_NO_PI + IFF_VNET_HDR, if I understand correctly. 

That's fairly much the only one that *did* work. As long as you use
TUNSETSNDBUF which has the undocumented side-effect of turning off the
XDP path.

> > On the receive side, where the tun device generates the virtio_net_hdr
> > but VIRITO_NET_F_MSG_RXBUF was negotiated and vhost-net needs to fill
> 
> Nit: VIRTIO_NET_F_MSG_RXBUF

Thanks.

> > in the 'num_buffers' field on top of the existing virtio_net_hdr, fix
> > that to use 'sock_hlen - 2' as the location, which means that it goes
> 
> Please use sizeof(hdr.num_buffers) instead of a raw constant 2, to
> self document the code.

Makes sense.

> Should this be an independent one-line fix?

I don't think so; it's very much intertwined with the way it makes
assumptions about someone else's data.

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