| 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
| ||
|
Message-ID: <cc14248c-cd6c-d604-003c-7384363dab8e@redhat.com>
Date: Tue, 21 Feb 2023 10:38:10 +0800
From: Jason Wang <jasowang@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>,
"Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com,
virtualization@...ts.linux-foundation.org,
Vitaly Mireyno <vmireyno@...vell.com>
Subject: Re: [patch net-next] net: virtio_net: implement exact header length
guest feature
在 2023/2/20 21:56, Jiri Pirko 写道:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@...hat.com wrote:
>> On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
>>> Fri, Feb 17, 2023 at 02:47:36PM CET, mst@...hat.com wrote:
>>>> On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
>>>>> Fri, Feb 17, 2023 at 01:22:01PM CET, mst@...hat.com wrote:
>>>>>> On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>>>>>>> From: Jiri Pirko <jiri@...dia.com>
>>>>>>>
>>>>>>> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
>>>>>>>
>>>>>>> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>>>>>>> set implicates that the driver provides the exact size of the header.
>>>>>>>
>>>>>>> The driver already complies to fill the correct value. Introduce the
>>>>>>> feature and advertise it.
>>>>>>>
>>>>>>> Signed-off-by: Jiri Pirko <jiri@...dia.com>
>>>>>> Could you add a bit of motivation just for the record?
>>>>>> Does this improve performance for some card? By how much?
>>>>>> Expected to help some future card?
>>>>> I can get that info, but isn't that rather something to be appended to
>>>>> the virtio-spec patch? I mean, the feature is there, this is just
>>>>> implementing it in one driver.
>>>> It is more like using it in the driver. It's not like we have to use
>>>> everything - it could be useful for e.g. dpdk but not linux.
>>>> Implementing it in the Linux driver has support costs - for example what
>>>> if there's a bug and sometimes the length is incorrect?
>>>> We'll be breaking things.
>>> I understand. To my understanding this feature just fixes the original
>>> ambiguity in the virtio spec.
>>>
>>> Quoting the original virtio spec:
>>> "hdr_len is a hint to the device as to how much of the header needs to
>>> be kept to copy into each packet"
>>>
>>> "a hint" might not be clear for the reader what does it mean, if it is
>>> "maybe like that" of "exactly like that". This feature just makes it
>>> crystal clear.
>>>
>>> If you look at the tap implementation, it uses hdr_len to alloc
>>> skb linear part. No hint, it counts with the provided value.
>>> So if the driver is currently not precise, it breaks tap.
>> Well that's only for gso though right?
> Yep.
>
>
>> And making it bigger than necessary works fine ...
> Well yeah. But tap does not do that, does it? it uses hdr_len directly.
tap_get_user() limit the head length:
static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
struct iov_iter *from, int noblock)
{
int good_linear = SKB_MAX_HEAD(TAP_RESERVE);
...
>
>
>>> I will add this to the patch description and send v2.
>>>
>> I feel this does not answer the question yet, or maybe I am being dense.
>> My point was not about making hdr_len precise. My point was that we are
>> making a change here for no apparent reason. I am guessing you are not
>> doing it for fun - so why? Is there a device with this feature bit
>> you are aware of?
> Afaik real hw which does emulation of virtio_net would benefit from
> that, our hw including.
Note that driver can choose to no negotiate this feature, so malicious
drivers can still try to use illegal value.
Thanks
>
>
>>
>>
>>>> The patch was submitted by Marvell but they never bothered with
>>>> using it in Linux. I guess they are using it for something else?
>>>> CC Vitaly who put this in.
>>>>
>>>>>> thanks!
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>> drivers/net/virtio_net.c | 6 ++++--
>>>>>>> include/uapi/linux/virtio_net.h | 1 +
>>>>>>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>> index fb5e68ed3ec2..e85b03988733 100644
>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>>>>>>> VIRTIO_NET_F_GUEST_UFO,
>>>>>>> VIRTIO_NET_F_GUEST_CSUM,
>>>>>>> VIRTIO_NET_F_GUEST_USO4,
>>>>>>> - VIRTIO_NET_F_GUEST_USO6
>>>>>>> + VIRTIO_NET_F_GUEST_USO6,
>>>>>>> + VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>> };
>>>>>>>
>>>>>>> #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>>>>>>> @@ -4213,7 +4214,8 @@ static struct virtio_device_id id_table[] = {
>>>>>>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>>>>> VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>>>>> VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>>>>>>> - VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
>>>>>>> + VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
>>>>>>> + VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>>
>>>>>>> static unsigned int features[] = {
>>>>>>> VIRTNET_FEATURES,
>>>>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>>>>> index b4062bed186a..12c1c9699935 100644
>>>>>>> --- a/include/uapi/linux/virtio_net.h
>>>>>>> +++ b/include/uapi/linux/virtio_net.h
>>>>>>> @@ -61,6 +61,7 @@
>>>>>>> #define VIRTIO_NET_F_GUEST_USO6 55 /* Guest can handle USOv6 in. */
>>>>>>> #define VIRTIO_NET_F_HOST_USO 56 /* Host can handle USO in. */
>>>>>>> #define VIRTIO_NET_F_HASH_REPORT 57 /* Supports hash report */
>>>>>>> +#define VIRTIO_NET_F_GUEST_HDRLEN 59 /* Guest provides the exact hdr_len value. */
>>>>>>> #define VIRTIO_NET_F_RSS 60 /* Supports RSS RX steering */
>>>>>>> #define VIRTIO_NET_F_RSC_EXT 61 /* extended coalescing info */
>>>>>>> #define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
>>>>>>> --
>>>>>>> 2.39.0
Powered by blists - more mailing lists