linux-kernel - Re: [PATCH net v1] virtio_net: Introduce skb_vnet_common

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] [day] [month] [year] [list]

Message-ID: <e8a414f0-ac55-9ac1-b115-94daa5914a9c@nvidia.com>
Date:   Wed, 16 Aug 2023 14:31:15 -0400
From:   Feng Liu <feliu@...dia.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Simon Horman <horms@...nel.org>
Cc:     virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jason Wang <jasowang@...hat.com>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Bodong Wang <bodong@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v1] virtio_net: Introduce skb_vnet_common_hdr to avoid
 typecasting



On 2023-08-16 a.m.10:53, Willem de Bruijn wrote:
> External email: Use caution opening links or attachments
> 
> 
>>
>> Thanks for the detailed explanation.
>>
>> I kept virtio_net_hdr_mrg_rxbuf and virtio_net_hdr_v1_hash structures in
>> virtio_net.h, which can be forward compatible with existing user
>> applications which use these structures.
> 
> They're UAPI, so we cannot modify or remove them anyway.
> 
> Which is exactly why we want to be careful with adding anything new.
> 
ok

>> virtio_net_hdr_v1_hash cannot use virtio_net_hdr as the first member,
>> because in virtio_net_hdr_v1, csum_start and csum_offset are stored in
>> union as a structure, and virtio_net_hdr cannot be used instead.
> 
> Oh right. That wasn't always the case, or the reason for this.
> Not super relevant but, commit ed9ecb0415b9 has the history
> 
>      virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.
> 
>      In particular, the virtio header always has the u16 num_buffers field.
>      We define a new 'struct virtio_net_hdr_v1' for this (rather than
>      simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
>      if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
> 
>      Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will
>      have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same
>      byte layout as struct virtio_net_hdr_v1.
> 
> The union was added to overload csum use on tx with RSC use on rx, in
> commit 22b436c9b568. I don't quite follow why there now are three
> structs, rather than two. The first two seem to both implement csum
> partial. Anyway, not super important here.
>ok

>> In addition, I put this new structure virtio_net_common_hdr in uapi,
>> hoping it could be used in future user space application to avoid
>> potential risks caused by type coercion (such as the problems mentioned
>> in the patch description ). So I think it should be in this header file.
>> What do you think?
> 
> Adding anything to UAPI has a high bar. Do you have a concrete use
> case for this?

In the scene of with and without VIRTIO_NET_F_HASH_REPORT feature, this 
patch has been tested on my setup, and the function is ok.

> 
> This does seem mostly a helper to simplify kernel logic to me, which
> is better kept in non-UAPI headers.
OK, will change it.