RE: [PATCH] net: fix IPv4 cksum simple function

["Morten Brørup" <mb@smartsharesystems.com>]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 5 November 2024 09.59
> 
> The new function breaks compilation with -Wcast-align.
> 
> In file included from /home/runner/work/ovs/ovs/dpdk-
> dir/include/rte_ip.h:9:
> /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:10:
> 	error: cast from 'const uint8_t *' (aka 'const unsigned char *')
> 	to 'const unaligned_uint16_t *' (aka 'const unsigned short *')
> 	increases required alignment from 1 to 2 [-Werror,-Wcast-align]
>         v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix this by aligning rte_ipv4_hdr to two bytes, and point at the start
> of the structure rather than the first field (which happens to be 1
> byte
> large).
> 
> Fixes: f9e1d67f237a ("net: add IPv4 cksum function for simple cases")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  doc/guides/rel_notes/release_24_11.rst | 2 ++
>  lib/net/rte_ip4.h                      | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_24_11.rst
> b/doc/guides/rel_notes/release_24_11.rst
> index 53a5ffebe5..d0152c558c 100644
> --- a/doc/guides/rel_notes/release_24_11.rst
> +++ b/doc/guides/rel_notes/release_24_11.rst
> @@ -291,6 +291,8 @@ API Changes
>      releases: it handles key=value and only-key cases.
>    * Both ``rte_kvargs_process`` and ``rte_kvargs_process_opt`` reject
> a NULL ``kvlist`` parameter.
> 
> +* net: The IPv4 header structure ``rte_ipv4_hdr`` has been marked as
> two bytes aligned.
> +
>  * net: The ICMP message types ``RTE_IP_ICMP_ECHO_REPLY`` and
> ``RTE_IP_ICMP_ECHO_REQUEST``
>    are marked as deprecated, and are replaced
>    by ``RTE_ICMP_TYPE_ECHO_REPLY`` and ``RTE_ICMP_TYPE_ECHO_REQUEST``.
> diff --git a/lib/net/rte_ip4.h b/lib/net/rte_ip4.h
> index 4dd0058cc5..f9b8333332 100644
> --- a/lib/net/rte_ip4.h
> +++ b/lib/net/rte_ip4.h
> @@ -39,7 +39,7 @@ extern "C" {
>  /**
>   * IPv4 Header
>   */
> -struct rte_ipv4_hdr {
> +struct __rte_aligned(2) rte_ipv4_hdr {

<unrelated>
I wonder why we have a convention of putting __rte_packed after the struct, and not between the "struct" tag and the name of the struct.
It would make the code much more readable having it here, like __rte_aligned().
</unrelated>

>  	__extension__
>  	union {
>  		uint8_t version_ihl;    /**< version and header length */
> @@ -188,7 +188,7 @@ rte_ipv4_cksum_simple(const struct rte_ipv4_hdr
> *ipv4_hdr)
>  	 * Compute the sum of successive 16-bit words of the IPv4 header,
>  	 * skipping the checksum field of the header.
>  	 */
> -	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> +	v16_h = (const uint16_t *)ipv4_hdr;
>  	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
>  		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> 
> --
> 2.46.2

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

For consistency, could one of you - David or Robin - please also 2-byte align the IPv6 header structure?