Re: [ovs-dev] [PATCH v2 8/8] netdev-linux: Fix uninitialized gso_type case.

[Eelco Chaudron]

On 28 May 2024, at 21:30, Ilya Maximets wrote:

> On 5/28/24 13:39, Eelco Chaudron wrote:
>> This patch fixes an uninitialized gso_type case in
>> netdev_linux_prepend_vnet_hdr() by returning an error.
>>
>> Fixes: 3337e6d91c5b ("userspace: Enable L4 checksum offloading by default.")
>> Signed-off-by: Eelco Chaudron 
>> ---
>>  lib/netdev-linux.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index eb0c5c624..7cffc0e13 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -7167,6 +7167,11 @@ netdev_linux_prepend_vnet_hdr(struct dp_packet *b, 
>> int mtu)
>>  vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>  } else if (dp_packet_hwol_tx_ipv6(b)) {
>>  vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>> +} else {
>> +VLOG_ERR_RL(, "Unknown gso_type for TSO hw offload packet. "
>> +"Flags: %"PRIu64,
>> +(uint64_t)*dp_packet_ol_flags_ptr(b));
>
> I'm not sure if this should be an error or warning, up to you.  But I'd 
> suggest
> removing the 'hw offload' part from the message since 'TSO' contains 'offload'
> and it's not necessarily hardware here.  Also, flags are better printed in 
> hex,
> i.e. %#"PRIx64.  And there is a missing space between the cast and a variable.

Thanks Ilya, I would keep it an error for now, as it probably needs some code 
fixing if this happens.

I’ll update the error message and next time I will take a better look than just 
cut/paste some code ;)

//Eelco

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 8/8] netdev-linux: Fix uninitialized gso_type case.

[Ilya Maximets]

On 5/28/24 13:39, Eelco Chaudron wrote:
> This patch fixes an uninitialized gso_type case in
> netdev_linux_prepend_vnet_hdr() by returning an error.
> 
> Fixes: 3337e6d91c5b ("userspace: Enable L4 checksum offloading by default.")
> Signed-off-by: Eelco Chaudron 
> ---
>  lib/netdev-linux.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index eb0c5c624..7cffc0e13 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -7167,6 +7167,11 @@ netdev_linux_prepend_vnet_hdr(struct dp_packet *b, int 
> mtu)
>  vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  } else if (dp_packet_hwol_tx_ipv6(b)) {
>  vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +} else {
> +VLOG_ERR_RL(, "Unknown gso_type for TSO hw offload packet. "
> +"Flags: %"PRIu64,
> +(uint64_t)*dp_packet_ol_flags_ptr(b));

I'm not sure if this should be an error or warning, up to you.  But I'd suggest
removing the 'hw offload' part from the message since 'TSO' contains 'offload'
and it's not necessarily hardware here.  Also, flags are better printed in hex,
i.e. %#"PRIx64.  And there is a missing space between the cast and a variable.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 8/8] netdev-linux: Fix uninitialized gso_type case.

[Eelco Chaudron]

This patch fixes an uninitialized gso_type case in
netdev_linux_prepend_vnet_hdr() by returning an error.

Fixes: 3337e6d91c5b ("userspace: Enable L4 checksum offloading by default.")
Signed-off-by: Eelco Chaudron 
---
 lib/netdev-linux.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index eb0c5c624..7cffc0e13 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -7167,6 +7167,11 @@ netdev_linux_prepend_vnet_hdr(struct dp_packet *b, int 
mtu)
 vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 } else if (dp_packet_hwol_tx_ipv6(b)) {
 vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+} else {
+VLOG_ERR_RL(, "Unknown gso_type for TSO hw offload packet. "
+"Flags: %"PRIu64,
+(uint64_t)*dp_packet_ol_flags_ptr(b));
+return EINVAL;
 }
 } else {
 vnet->hdr_len = 0;
-- 
2.44.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev