Skip to content

icmp6: replace endian-dependent bitfields with flag enums #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
christophefontaine merged 1 commit into from
Nov 4, 2025
Merged

icmp6: replace endian-dependent bitfields with flag enums #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions modules/infra/datapath/gr_icmp6.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#pragma once

#include <gr_bitops.h>
#include <gr_macro.h>

#include <rte_byteorder.h>
Expand Down Expand Up @@ -74,14 +75,15 @@ struct icmp6_router_solicit {
uint32_t __reserved;
} __rte_packed;

typedef enum : uint8_t {
ICMP6_RA_F_MANAGED_ADDR = GR_BIT8(0),
ICMP6_RA_F_OTHER_CONFIG = GR_BIT8(1),
} icmp6_ra_flags_t;

// ICMP6_TYPE_ROUTER_ADVERT
struct icmp6_router_advert {
uint8_t cur_hoplim;
#if BYTE_ORDER == BIG_ENDIAN
uint8_t managed_addr : 1, other_config : 1, __unused_flags : 6;
#else
uint8_t __unused_flags : 6, managed_addr : 1, other_config : 1;
Copy link
Collaborator

@maxime-leroy maxime-leroy Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean we should not allow any bit-fields in grout ?

Copy link
Collaborator

@christophefontaine christophefontaine Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trading a known complexity in 1 header with a more complex code to write, read and debug in all other files.
I'm not sure if I'm a fan of that.
@rjarry Do we have a test that this works on big endian arch ? Otherwise, I'd refrain merging that.

Copy link
Collaborator

@maxime-leroy maxime-leroy Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://bugs.dpdk.org/show_bug.cgi?id=1679

Copy link
Collaborator Author

@rjarry rjarry Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can have bitfields, but for stuff that goes in the network, it is "unsafe" to rely on compiler implementation details.

Copy link
Collaborator Author

@rjarry rjarry Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C standard only mentions byte ordering. Not bit ordering. Compilers can store them in any order they want.

Copy link
Collaborator

@christophefontaine christophefontaine Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the approach in the BZ comment, using __attribute__((scalar_storage_order("big-endian"))).
Wouldn't it be better ? Even on the debugging side, this feels cleaner.

Copy link
Collaborator

@maxime-leroy maxime-leroy Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scalar_storage_order doesn't exist on clang : https://bugs.llvm.org/show_bug.cgi?id=35293

Copy link
Collaborator Author

@rjarry rjarry Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Maxime noted, this isn't supported by clang. And not standard at all.

What's wrong with using an enum with flags? Debuggers will display them without issues.

Copy link
Collaborator

@maxime-leroy maxime-leroy Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same problem with vi. You should use emacs ;-) . Why ? Because !

#endif
icmp6_ra_flags_t flags;
rte_be16_t lifetime;
rte_be32_t reachable_time;
rte_be32_t retrans_timer;
Expand All @@ -93,13 +95,15 @@ struct icmp6_neigh_solicit {
struct rte_ipv6_addr target;
} __rte_packed;

typedef enum : uint8_t {
ICMP6_NA_F_ROUTER = GR_BIT8(0),
ICMP6_NA_F_SOLICITED = GR_BIT8(1),
ICMP6_NA_F_OVERRIDE = GR_BIT8(2),
} icmp6_na_flags_t;

// ICMP6_TYPE_NEIGH_ADVERT
struct icmp6_neigh_advert {
#if BYTE_ORDER == BIG_ENDIAN
uint8_t router : 1, solicited : 1, override : 1, __unused_flags : 5;
#else
uint8_t __unused_flags : 5, override : 1, solicited : 1, router : 1;
#endif
icmp6_na_flags_t flags;
uint8_t __reserved;
uint16_t __reserved2;
struct rte_ipv6_addr target;
Expand Down
8 changes: 7 additions & 1 deletion modules/infra/datapath/trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,13 @@ int trace_icmp6_format(char *buf, size_t len, const struct icmp6 *icmp6, size_t
const struct icmp6_neigh_advert *na = PAYLOAD(icmp6);
payload_len -= sizeof(*na);
inet_ntop(AF_INET6, &na->target, dst, sizeof(dst));
SAFE_BUF(snprintf, len, "neigh advert %s is at (solicited=%u)", dst, na->solicited);
SAFE_BUF(
snprintf,
len,
"neigh advert %s is at (solicited=%u)",
dst,
(na->flags & ICMP6_NA_F_SOLICITED) != 0
);
opt = PAYLOAD(na);
break;
}
Expand Down
3 changes: 1 addition & 2 deletions modules/ip6/control/router_advert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ static void build_ra_packet(struct rte_mbuf *m, const struct rte_ipv6_addr *src)
icmp6->code = 0;
ra = (struct icmp6_router_advert *)rte_pktmbuf_append(m, sizeof(*ra));
ra->cur_hoplim = IP6_DEFAULT_HOP_LIMIT; // Default TTL for this network
ra->managed_addr = 0; // DHCPv6 is available
ra->other_config = 0; // DNS available, ...
ra->flags = 0;
ra->lifetime = rte_cpu_to_be_16(ra_conf[iface_id].lifetime);
ra->reachable_time = RTE_BE16(0);
ra->retrans_timer = RTE_BE16(0);
Expand Down
6 changes: 2 additions & 4 deletions modules/ip6/datapath/ndp_na_input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ static uint16_t ndp_na_input_process(
ASSERT_NDP(!rte_ipv6_addr_is_mcast(&na->target));
// - If the IP Destination Address is a multicast address the
// Solicited flag is zero.
ASSERT_NDP(!rte_ipv6_addr_is_mcast(&d->dst) || na->solicited == 0);
ASSERT_NDP(!rte_ipv6_addr_is_mcast(&d->dst) || !(na->flags & ICMP6_NA_F_SOLICITED));

// https://www.rfc-editor.org/rfc/rfc4861.html#section-7.2.5
//
Expand Down Expand Up @@ -162,9 +162,7 @@ static void init_default_na_mbuf(struct fake_ndp_na_mbuf *ndp_mbuf) {
ndp_mbuf->icmp6_hdr.code = 0;

// NA specific packet headers
ndp_mbuf->na_hdr.router = 0;
ndp_mbuf->na_hdr.solicited = 1;
ndp_mbuf->na_hdr.override = 1;
ndp_mbuf->na_hdr.flags = ICMP6_NA_F_SOLICITED | ICMP6_NA_F_OVERRIDE;
ndp_mbuf->na_hdr.target = (struct rte_ipv6_addr)
RTE_IPV6(0xfe80, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x00aa);

Expand Down
6 changes: 3 additions & 3 deletions modules/ip6/datapath/ndp_na_output.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ static uint16_t ndp_na_output_process(
icmp6->type = ICMP6_TYPE_NEIGH_ADVERT;
icmp6->code = 0;
na = PAYLOAD(icmp6);
na->override = 1;
na->router = 1;
na->solicited = remote != NULL;
na->flags = ICMP6_NA_F_OVERRIDE | ICMP6_NA_F_ROUTER;
if (remote != NULL)
na->flags |= ICMP6_NA_F_SOLICITED;
na->target = l3->ipv6;
opt = PAYLOAD(na);
opt->type = ICMP6_OPT_TARGET_LLADDR;
Expand Down