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

[Feng Liu <feliu@nvidia.com>]

On 2023-08-15 p.m.2:13, Willem de Bruijn wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Aug 15, 2023 at 12:29 PM Simon Horman <horms@kernel.org> wrote:
>>
>> On Tue, Aug 15, 2023 at 11:09:02AM -0400, Feng Liu wrote:

>> To clarify: In general new Networking features go via the net-next tree,
>> while bug fixes go via the net tree. I was suggesting this
>> is more appropriate for net-next, and that should be reflected in the
>> subject.
>>
>>          Subject: [PATCH net-next] ...
>>
>> Sorry for not being clearer the first time around.
> 
> Right, this should go to net-next.
> 
Will do, thanks

>>
>>>

>>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>>> index 12c1c9699935..db40f93ae8b3 100644
>>>>> --- a/include/uapi/linux/virtio_net.h
>>>>> +++ b/include/uapi/linux/virtio_net.h
>>>>> @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf {
>>>>>         struct virtio_net_hdr hdr;
>>>>>         __virtio16 num_buffers; /* Number of merged rx buffers */
>>>>>    };
>>>>> +
>>>>> +struct virtio_net_common_hdr {
>>>>> +     union {
>>>>> +             struct virtio_net_hdr_mrg_rxbuf mrg_hdr;
>>>>> +             struct virtio_net_hdr_v1_hash hash_v1_hdr;
>>>>> +     };
>>>>> +};
>>>>
>>>> Does this belong in the UAPI?
>>>> I would have assumed it's a Kernel implementation detail.
>>>>
>>> The existing codes, virtio_net.h is in uapi/linux/, I added the new
>>> structure and followed existing code. My modification is related to Kernel
>>> implementation detail now.
>>
>> The header you have modified forms part of the userspace API (UAPI).
>> Perhaps there is something about virtio_net that makes this correct, but it
>> seems to me that kernel-internal details don't belong there.
> 
> FWIW, I ran into similar issues before in a draft that added timestamp
> support [1]
> 
> If we're going to change this structure, we should do it in a way that
> is forward proof to future extensions to the virtio spec and with that
> the fields in this struct. Especially in UAPI.
> 
> Is virtio_net_hdr_v1_hash the latest virtio-spec compliant header? And
> do we expect for v1.3 to just add some fields to this?
> 
> The struct comment of virtio_net_hdr_v1 states "This is
> bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf, only
> flattened.". I don't quite understand what the flattening bought, vs
> having struct virtio_net_hdr as first member. Another difference may
> be the endianness between legacy (0.9) and v1.0+.
> 
> 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.

After checking kernel code, the virtio_net_hdr_v1_hash structure does 
only add new members to virtio_net_hdr_mrg_rxbuf, so the spec should 
only add new members, otherwise there will be compatibility problems in 
struct virtio_net_hdr_v1_hash structure.

struct virtio_net_hdr_v1_hash {
	struct virtio_net_hdr_v1 hdr; /*same size as virtio_net_hdr*/
[...]
	__le32 hash_value; /*new member*/
	__le16 hash_report; /*new member*/
	__le16 padding;	/*new member*/
};

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.

struct virtio_net_hdr_v1 {
[...]
	union {
		struct {
			__virtio16 csum_start;
			__virtio16 csum_offset;
		};
		[...]
	};
	__virtio16 num_buffers;	/* Number of merged rx buffers */
};


struct virtio_net_hdr {
[...]
	__virtio16 csum_start;	
	__virtio16 csum_offset;	
};



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?