[v2,16/16] ipv6: add function to check ipv6 version
Checks
Commit Message
This is a slow path version to verify the version field in IPv6 headers.
Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
Notes:
v2: new patch
app/test/test_net_ipv6.c | 16 ++++++++++++++++
lib/net/rte_ip6.h | 17 +++++++++++++++++
2 files changed, 33 insertions(+)
Comments
> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Tuesday, 1 October 2024 10.17
>
> This is a slow path version to verify the version field in IPv6
> headers.
It doesn't look slow to me. No need to mention slow path here.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
>
> Notes:
> v2: new patch
>
> app/test/test_net_ipv6.c | 16 ++++++++++++++++
> lib/net/rte_ip6.h | 17 +++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/app/test/test_net_ipv6.c b/app/test/test_net_ipv6.c
> index b087b5c60d73..79a43a2b5491 100644
> --- a/app/test/test_net_ipv6.c
> +++ b/app/test/test_net_ipv6.c
> @@ -11,6 +11,21 @@ static const struct rte_ipv6_addr bcast_addr = {
> };
> static const struct rte_ipv6_addr zero_addr = { 0 };
>
> +static int
> +test_ipv6_check_version(void)
> +{
> + struct rte_ipv6_hdr h;
> +
> + h.vtc_flow = 0;
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> + h.vtc_flow = RTE_BE32(0x7f00ba44);
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> + h.vtc_flow = RTE_BE32(0x6badcaca);
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), 0, "");
> +
> + return 0;
> +}
> +
> static int
> test_ipv6_addr_mask(void)
> {
> @@ -191,6 +206,7 @@ test_ether_mcast_from_ipv6(void)
> static int
> test_net_ipv6(void)
> {
> + TEST_ASSERT_SUCCESS(test_ipv6_check_version(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_mask(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_eq_prefix(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_kind(), "");
> diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
> index c552fa54c095..de3ddc0348c5 100644
> --- a/lib/net/rte_ip6.h
> +++ b/lib/net/rte_ip6.h
> @@ -323,6 +323,23 @@ struct rte_ipv6_hdr {
> struct rte_ipv6_addr dst_addr; /**< IP address of
> destination host(s). */
> } __rte_packed;
>
> +#define RTE_IPV6_VTC_FLOW_VERSION RTE_BE32(0x60000000)
> +#define RTE_IPV6_VTC_FLOW_VERSION_MASK RTE_BE32(0xf0000000)
> +
> +/**
> + * Check that the IPv6 header version field is valid according to RFC
> 8200 section 3.
> + *
> + * @return
> + * 0 if the version field is valid. -EINVAL otherwise.
> + */
> +static inline int rte_ipv6_check_version(const struct rte_ipv6_hdr
> *ip)
> +{
> + rte_be32_t v = ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK;
> + if (v != RTE_IPV6_VTC_FLOW_VERSION)
> + return -EINVAL;
> + return 0;
> +}
Personally, I would prefer following the convention of rte_ether functions to return boolean (as int)...
static inline int rte_is_<zero/unicast/multicast/broadcast/universal/local_admin/valid_assigned>_ether_addr(const struct rte_ether_addr *ea)
static inline int rte_is_ipv6_version(const struct rte_ipv6_hdr *ip)
{
return ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK == RTE_IPV6_VTC_FLOW_VERSION;
}
Or, at your preference, an optimized variant:
static inline int rte_is_version_ipv6(const struct rte_ipv6_hdr *ip)
{
return *(const unsigned char *)ip & 0xf0 == 0x60;
}
The first nibble is also used for version in IPv4, so an IPv4 variant would look similar:
static inline int rte_is_version_ipv4(const struct rte_ip_hdr *ip)
{
return *(const unsigned char *)ip & 0xf0 == 0x40;
}
> This is a slow path version to verify the version field in IPv6 headers.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
>
> Notes:
> v2: new patch
>
> app/test/test_net_ipv6.c | 16 ++++++++++++++++
> lib/net/rte_ip6.h | 17 +++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/app/test/test_net_ipv6.c b/app/test/test_net_ipv6.c
> index b087b5c60d73..79a43a2b5491 100644
> --- a/app/test/test_net_ipv6.c
> +++ b/app/test/test_net_ipv6.c
> @@ -11,6 +11,21 @@ static const struct rte_ipv6_addr bcast_addr = {
> };
> static const struct rte_ipv6_addr zero_addr = { 0 };
>
> +static int
> +test_ipv6_check_version(void)
> +{
> + struct rte_ipv6_hdr h;
> +
> + h.vtc_flow = 0;
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> + h.vtc_flow = RTE_BE32(0x7f00ba44);
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
> + h.vtc_flow = RTE_BE32(0x6badcaca);
> + TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), 0, "");
> +
> + return 0;
> +}
> +
> static int
> test_ipv6_addr_mask(void)
> {
> @@ -191,6 +206,7 @@ test_ether_mcast_from_ipv6(void)
> static int
> test_net_ipv6(void)
> {
> + TEST_ASSERT_SUCCESS(test_ipv6_check_version(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_mask(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_eq_prefix(), "");
> TEST_ASSERT_SUCCESS(test_ipv6_addr_kind(), "");
> diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
> index c552fa54c095..de3ddc0348c5 100644
> --- a/lib/net/rte_ip6.h
> +++ b/lib/net/rte_ip6.h
> @@ -323,6 +323,23 @@ struct rte_ipv6_hdr {
> struct rte_ipv6_addr dst_addr; /**< IP address of destination host(s). */
> } __rte_packed;
>
> +#define RTE_IPV6_VTC_FLOW_VERSION RTE_BE32(0x60000000)
> +#define RTE_IPV6_VTC_FLOW_VERSION_MASK RTE_BE32(0xf0000000)
> +
> +/**
> + * Check that the IPv6 header version field is valid according to RFC 8200 section 3.
> + *
> + * @return
> + * 0 if the version field is valid. -EINVAL otherwise.
Probably 0/1 as return values will be nicer.
> + */
If it is a new one, then it probably needs to be rte_experimental.
> +static inline int rte_ipv6_check_version(const struct rte_ipv6_hdr *ip)
> +{
> + rte_be32_t v = ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK;
> + if (v != RTE_IPV6_VTC_FLOW_VERSION)
> + return -EINVAL;
> + return 0;
> +}
> +
> /* IPv6 routing extension type definition. */
> #define RTE_IPV6_SRCRT_TYPE_4 4
>
> --
> 2.46.1
>
Hi Morten,
Morten Brørup, Oct 06, 2024 at 11:02:
> Personally, I would prefer following the convention of rte_ether functions to return boolean (as int)...
>
> static inline int rte_is_<zero/unicast/multicast/broadcast/universal/local_admin/valid_assigned>_ether_addr(const struct rte_ether_addr *ea)
Sorry, I haven't followed your recommendation in v3, but I have a good
reason :)
I find that functions following that naming scheme are impossible to
find since they all start with the same "rte_is_" prefix regardless of
the DPDK module in which they are defined. It it is particularly
annoying when using code completion with clangd or similar tools.
rte_is_power_of_2 <rte_bitops.h>
rte_is_aligned <rte_common.h>
rte_is_same_ether_addr <rte_ether.h>
rte_is_zero_ether_addr <rte_ether.h>
rte_is_unicast_ether_addr <rte_ether.h>
rte_is_multicast_ether_addr <rte_ether.h>
rte_is_broadcast_ether_addr <rte_ether.h>
rte_is_universal_ether_addr <rte_ether.h>
rte_is_local_admin_ether_addr <rte_ether.h>
rte_is_valid_assigned_ether_addr <rte_ether.h>
The ethernet address functions are even more extreme as they end all
with the same suffix:
rte_<action>_<namespace>_<object>
All other public API seems to follow the inverse logic:
rte_<namespace>_<object>_<action>
It does not follow the natural English language but more of an inverse
polish notation. However, it feels more user friendly and better
discoverable.
rte_ipv6_addr_eq
rte_ipv6_addr_eq_prefix
rte_ipv6_addr_is_linklocal
rte_ipv6_addr_is_loopback
rte_ipv6_addr_is_mcast
rte_ipv6_addr_is_sitelocal
rte_ipv6_addr_is_unspec
rte_ipv6_addr_is_v4compat
rte_ipv6_addr_is_v4mapped
rte_ipv6_addr_mask
rte_ipv6_llocal_from_ethernet
rte_ipv6_mask_depth
rte_ipv6_mc_scope
rte_ipv6_solnode_from_addr
I hope I'm making sense :)
> static inline int rte_is_ipv6_version(const struct rte_ipv6_hdr *ip)
> {
> return ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK == RTE_IPV6_VTC_FLOW_VERSION;
> }
>
> Or, at your preference, an optimized variant:
> static inline int rte_is_version_ipv6(const struct rte_ipv6_hdr *ip)
> {
> return *(const unsigned char *)ip & 0xf0 == 0x60;
> }
>
> The first nibble is also used for version in IPv4, so an IPv4 variant would look similar:
> static inline int rte_is_version_ipv4(const struct rte_ip_hdr *ip)
> {
> return *(const unsigned char *)ip & 0xf0 == 0x40;
> }
I had missed this in ipv4. I could rework it for v4 if there are other
remarks.
Thanks for the review!
> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Thursday, 10 October 2024 22.01
>
> Hi Morten,
>
> Morten Brørup, Oct 06, 2024 at 11:02:
> > Personally, I would prefer following the convention of rte_ether
> functions to return boolean (as int)...
> >
> > static inline int
> rte_is_<zero/unicast/multicast/broadcast/universal/local_admin/valid_as
> signed>_ether_addr(const struct rte_ether_addr *ea)
>
> Sorry, I haven't followed your recommendation in v3, but I have a good
> reason :)
>
> I find that functions following that naming scheme are impossible to
> find since they all start with the same "rte_is_" prefix regardless of
> the DPDK module in which they are defined. It it is particularly
> annoying when using code completion with clangd or similar tools.
>
> rte_is_power_of_2 <rte_bitops.h>
> rte_is_aligned <rte_common.h>
> rte_is_same_ether_addr <rte_ether.h>
> rte_is_zero_ether_addr <rte_ether.h>
> rte_is_unicast_ether_addr <rte_ether.h>
> rte_is_multicast_ether_addr <rte_ether.h>
> rte_is_broadcast_ether_addr <rte_ether.h>
> rte_is_universal_ether_addr <rte_ether.h>
> rte_is_local_admin_ether_addr <rte_ether.h>
> rte_is_valid_assigned_ether_addr <rte_ether.h>
>
> The ethernet address functions are even more extreme as they end all
> with the same suffix:
>
> rte_<action>_<namespace>_<object>
>
> All other public API seems to follow the inverse logic:
>
> rte_<namespace>_<object>_<action>
>
> It does not follow the natural English language but more of an inverse
> polish notation. However, it feels more user friendly and better
> discoverable.
>
> rte_ipv6_addr_eq
> rte_ipv6_addr_eq_prefix
> rte_ipv6_addr_is_linklocal
> rte_ipv6_addr_is_loopback
> rte_ipv6_addr_is_mcast
> rte_ipv6_addr_is_sitelocal
> rte_ipv6_addr_is_unspec
> rte_ipv6_addr_is_v4compat
> rte_ipv6_addr_is_v4mapped
> rte_ipv6_addr_mask
> rte_ipv6_llocal_from_ethernet
> rte_ipv6_mask_depth
> rte_ipv6_mc_scope
> rte_ipv6_solnode_from_addr
>
> I hope I'm making sense :)
Excellent explanation.
You have convinced me, so I now agree with your decision.
Then, in some other future patch, the ether_addr functions should be renamed to follow this convention too. And for backwards compatibility, their old names can be preserved as macros/functions calling the new names.
>
> > static inline int rte_is_ipv6_version(const struct rte_ipv6_hdr *ip)
> > {
> > return ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK ==
> RTE_IPV6_VTC_FLOW_VERSION;
> > }
> >
> > Or, at your preference, an optimized variant:
> > static inline int rte_is_version_ipv6(const struct rte_ipv6_hdr *ip)
> > {
> > return *(const unsigned char *)ip & 0xf0 == 0x60;
> > }
> >
> > The first nibble is also used for version in IPv4, so an IPv4 variant
> would look similar:
> > static inline int rte_is_version_ipv4(const struct rte_ip_hdr *ip)
> > {
> > return *(const unsigned char *)ip & 0xf0 == 0x40;
> > }
>
> I had missed this in ipv4. I could rework it for v4 if there are other
> remarks.
>
> Thanks for the review!
@@ -11,6 +11,21 @@ static const struct rte_ipv6_addr bcast_addr = {
};
static const struct rte_ipv6_addr zero_addr = { 0 };
+static int
+test_ipv6_check_version(void)
+{
+ struct rte_ipv6_hdr h;
+
+ h.vtc_flow = 0;
+ TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
+ h.vtc_flow = RTE_BE32(0x7f00ba44);
+ TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), -EINVAL, "");
+ h.vtc_flow = RTE_BE32(0x6badcaca);
+ TEST_ASSERT_EQUAL(rte_ipv6_check_version(&h), 0, "");
+
+ return 0;
+}
+
static int
test_ipv6_addr_mask(void)
{
@@ -191,6 +206,7 @@ test_ether_mcast_from_ipv6(void)
static int
test_net_ipv6(void)
{
+ TEST_ASSERT_SUCCESS(test_ipv6_check_version(), "");
TEST_ASSERT_SUCCESS(test_ipv6_addr_mask(), "");
TEST_ASSERT_SUCCESS(test_ipv6_addr_eq_prefix(), "");
TEST_ASSERT_SUCCESS(test_ipv6_addr_kind(), "");
@@ -323,6 +323,23 @@ struct rte_ipv6_hdr {
struct rte_ipv6_addr dst_addr; /**< IP address of destination host(s). */
} __rte_packed;
+#define RTE_IPV6_VTC_FLOW_VERSION RTE_BE32(0x60000000)
+#define RTE_IPV6_VTC_FLOW_VERSION_MASK RTE_BE32(0xf0000000)
+
+/**
+ * Check that the IPv6 header version field is valid according to RFC 8200 section 3.
+ *
+ * @return
+ * 0 if the version field is valid. -EINVAL otherwise.
+ */
+static inline int rte_ipv6_check_version(const struct rte_ipv6_hdr *ip)
+{
+ rte_be32_t v = ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK;
+ if (v != RTE_IPV6_VTC_FLOW_VERSION)
+ return -EINVAL;
+ return 0;
+}
+
/* IPv6 routing extension type definition. */
#define RTE_IPV6_SRCRT_TYPE_4 4