netdev - Re: [PATCH net] virtio-net: reject small vring sizes

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AM0PR04MB4723E38859953B6C531D3E5CD4649@AM0PR04MB4723.eurprd04.prod.outlook.com>
Date:   Tue, 25 Apr 2023 13:02:38 +0000
From:   Alvaro Karsz <alvaro.karsz@...id-run.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
CC:     Jason Wang <jasowang@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] virtio-net: reject small vring sizes

> > In the virtnet case, we'll decide which features to block based on the ring size.
> > 2 < ring < MAX_FRAGS + 2  -> BLOCK GRO + MRG_RXBUF
> > ring < 2  -> BLOCK GRO + MRG_RXBUF + CTRL_VQ
> 
> why MRG_RXBUF? what does it matter?
> 

You're right, it should be blocked only when ring < 2.
Or we should let this pass, and let the device figure out that MRG_RXBUF is meaningless with 1 entry..

> > So we'll need a new virtio callback instead of flags.
> > Furthermore, other virtio drivers may decide which features to block based on parameters different than ring size (I don't have a good example at the moment).
> > So maybe we should leave it to the driver to handle (during probe), and offer a virtio core function to re-negotiate the features?
> >
> > In the solution I'm working on, I expose a new virtio core function that resets the device and renegotiates the received features.
> > + A new virtio_config_ops callback peek_vqs_len to peek at the VQ lengths before calling find_vqs. (The callback must be called after the features negotiation)
> >
> > So, the flow is something like:
> >
> > * Super early in virtnet probe, we peek at the VQ lengths and decide if we are
> >    using small vrings, if so, we reset and renegotiate the features.
> 
> Using which APIs? What does peek_vqs_len do and why does it matter that
> it is super early?
> 

We peek at the lengths using a new virtio_config.h function that calls a transport specific callback.
We renegotiate calling the new, exported virtio core function.

peek_vqs_len fills an array of u16 variables with the max length of every VQ.

The idea here is not to fail probe.
So we start probe, check if the ring is small, renegotiate the features and then continue with the new features.
This needs to be super early because otherwise, some virtio_has_feature calls before re-negotiating may be invalid, meaning a lot of reconfigurations.

> > * We continue normally and create the VQs.
> > * We check if the created rings are small.
> >    If they are and some blocked features were negotiated anyway (may occur if
> >    the re-negotiation fails, or if the transport has no implementation for
> >    peek_vqs_len), we fail probe.
> >    If the ring is small and the features are ok, we mark the virtnet device as
> >    vring_small and fixup some variables.
> >
> >
> > peek_vqs_len is needed because we must know the VQ length before calling init_vqs.
> >
> > During virtnet_find_vqs we check the following:
> > vi->has_cvq
> > vi->big_packets
> > vi->mergeable_rx_bufs
> >
> > But these will change if the ring is small..
> >
> > (Of course, another solution will be to re-negotiate features after init_vqs, but this will make a big mess, tons of things to clean and reconfigure)
> >
> >
> > The 2 < ring < MAX_FRAGS + 2 part is ready, I have tested a few cases and it is working.
> >
> > I'm considering splitting the effort into 2 series.
> > A 2 < ring < MAX_FRAGS + 2  series, and a follow up series with the ring < 2 case.
> >
> > I'm also thinking about sending the first series as an RFC soon, so it will be more broadly tested.
> >
> > What do you think?
> 
> Lots of work spilling over to transports.
> 
> And I especially don't like that it slows down boot on good path.

Yes, but I don't think that this is really significant.
It's just a call to the transport to get the length of the VQs.
If ring is not small, we continue as normal.
If ring is small, we renegotiate and continue, without failing probe.

> 
> I have the following idea:
> - add a blocked features value in virtio_device
> - before calling probe, core saves blocked features
> - if probe fails, checks blocked features.
>   if any were added, reset, negotiate all features
>   except blocked ones and do the validate/probe dance again
> 
> 
> This will mean mostly no changes to drivers: just check condition,
> block feature and fail probe.
> 

I like the idea, will try to implement it.

Thanks,