Re: [PATCH net v1] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Feng Liu <feliu@nvidia.com>,
	 Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	 Simon Horman <horms@kernel.org>
Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	Eric Dumazet <edumazet@google.com>, Jiri Pirko <jiri@nvidia.com>,
	Bodong Wang <bodong@nvidia.com>, Jakub Kicinski <kuba@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net v1] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting
Date: Wed, 16 Aug 2023 10:53:38 -0400	[thread overview]
Message-ID: <64dce2f2b99f5_23f1f82949f@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <f9f3c150-2b5e-7bd0-1c1a-062bd1f16fcd@nvidia.com>

> > 
> > Since legacy virtio will no longer be modified, I don't think there is
> > much value is exposing this new union as UAPI. I do appreciate the
> > benefit to the implementation.
> > 
> > [1] https://patches.linaro.org/project/netdev/patch/20210208185558.995292-3-willemdebruijn.kernel@gmail.com/
> Hi, William and Simon
> 
> 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.

> 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.

> 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?

This does seem mostly a helper to simplify kernel logic to me, which
is better kept in non-UAPI headers.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization