From: Cedric Jehasse <cedric.jehasse@luminex.be>
When creating a flower classifier with an ipv4 address the
flow_dissector has both FLOW_DISSECTOR_KEY_IPV4_ADDRS and
FLOW_DISSECTOR_KEY_IPV6_ADDRS bits set in used_keys.
This happens because ipv4/ipv6 fields are a union and
FL_KEY_SET_IF_MASKED() will interpret either being set as both.
Removing the unions fixes this behavior without needing special handling
for union fields.
Example of a command that caused FLOW_DISSECTOR_KEY_IPV4_ADDRS and
FLOW_DISSECTOR_KEY_IPV6_ADDRS to be set:
tc filter add dev p1 ingress protocol ip flower skip_sw \
dst_ip 224.0.1.129 action trap
Signed-off-by: Cedric Jehasse <cedric.jehasse@luminex.be>
---
net/sched/cls_flower.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 7669371c1354..b95dbe847dde 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -59,18 +59,14 @@ struct fl_flow_key {
struct flow_dissector_key_eth_addrs eth;
struct flow_dissector_key_vlan vlan;
struct flow_dissector_key_vlan cvlan;
- union {
- struct flow_dissector_key_ipv4_addrs ipv4;
- struct flow_dissector_key_ipv6_addrs ipv6;
- };
+ struct flow_dissector_key_ipv4_addrs ipv4;
+ struct flow_dissector_key_ipv6_addrs ipv6;
struct flow_dissector_key_ports tp;
struct flow_dissector_key_icmp icmp;
struct flow_dissector_key_arp arp;
struct flow_dissector_key_keyid enc_key_id;
- union {
- struct flow_dissector_key_ipv4_addrs enc_ipv4;
- struct flow_dissector_key_ipv6_addrs enc_ipv6;
- };
+ struct flow_dissector_key_ipv4_addrs enc_ipv4;
+ struct flow_dissector_key_ipv6_addrs enc_ipv6;
struct flow_dissector_key_ports enc_tp;
struct flow_dissector_key_mpls mpls;
struct flow_dissector_key_tcp tcp;
--
2.43.0On Wed, Mar 11, 2026 at 11:46:18AM +0100, Cedric Jehasse via B4 Relay wrote:
> From: Cedric Jehasse <cedric.jehasse@luminex.be>
>
> When creating a flower classifier with an ipv4 address the
> flow_dissector has both FLOW_DISSECTOR_KEY_IPV4_ADDRS and
> FLOW_DISSECTOR_KEY_IPV6_ADDRS bits set in used_keys.
> This happens because ipv4/ipv6 fields are a union and
> FL_KEY_SET_IF_MASKED() will interpret either being set as both.
>
> Removing the unions fixes this behavior without needing special handling
> for union fields.
>
> Example of a command that caused FLOW_DISSECTOR_KEY_IPV4_ADDRS and
> FLOW_DISSECTOR_KEY_IPV6_ADDRS to be set:
> tc filter add dev p1 ingress protocol ip flower skip_sw \
> dst_ip 224.0.1.129 action trap
>
> Signed-off-by: Cedric Jehasse <cedric.jehasse@luminex.be>
Hi Cedric,
This doesn't seem to be the right approach to me.
It seems to me that the use of a union is intentional here, as either IPv4
or IPv6 addresses can be present in each case - never both. And that
control.addr_type and enc_control.addr_type are intended to allow
differentiation of the address type in use for each of these unions.
> ---
> net/sched/cls_flower.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 7669371c1354..b95dbe847dde 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -59,18 +59,14 @@ struct fl_flow_key {
> struct flow_dissector_key_eth_addrs eth;
> struct flow_dissector_key_vlan vlan;
> struct flow_dissector_key_vlan cvlan;
> - union {
> - struct flow_dissector_key_ipv4_addrs ipv4;
> - struct flow_dissector_key_ipv6_addrs ipv6;
> - };
> + struct flow_dissector_key_ipv4_addrs ipv4;
> + struct flow_dissector_key_ipv6_addrs ipv6;
> struct flow_dissector_key_ports tp;
> struct flow_dissector_key_icmp icmp;
> struct flow_dissector_key_arp arp;
> struct flow_dissector_key_keyid enc_key_id;
> - union {
> - struct flow_dissector_key_ipv4_addrs enc_ipv4;
> - struct flow_dissector_key_ipv6_addrs enc_ipv6;
> - };
> + struct flow_dissector_key_ipv4_addrs enc_ipv4;
> + struct flow_dissector_key_ipv6_addrs enc_ipv6;
> struct flow_dissector_key_ports enc_tp;
> struct flow_dissector_key_mpls mpls;
> struct flow_dissector_key_tcp tcp;
>
> --
> 2.43.0
>
>
On Fri, 13 Mar 2026 13:26:51 +0000 Simon Horman wrote: > It seems to me that the use of a union is intentional here, as either IPv4 > or IPv6 addresses can be present in each case - never both. And that > control.addr_type and enc_control.addr_type are intended to allow > differentiation of the address type in use for each of these unions. My reading was that the initial author simply wanted to save space in the struct. As the commit message explains this leads to complications in the logic which sets the keys. The alternative is to complicate FL_KEY_SET_IF_MASKED - doable, but given that the union feels like a micro-optimization in the first place the simpler approach of separating fields seems okay too? (TBH my mind also initially went down the FL_KEY_SET_IF_MASKED rabbit hole but once I saw the simplicity of Cedric's patch I changed my mind)
On Sat, Mar 14, 2026 at 10:00:02AM -0700, Jakub Kicinski wrote: > On Fri, 13 Mar 2026 13:26:51 +0000 Simon Horman wrote: > > It seems to me that the use of a union is intentional here, as either IPv4 > > or IPv6 addresses can be present in each case - never both. And that > > control.addr_type and enc_control.addr_type are intended to allow > > differentiation of the address type in use for each of these unions. > > My reading was that the initial author simply wanted to save space in > the struct. > > As the commit message explains this leads to complications in the logic > which sets the keys. The alternative is to complicate > FL_KEY_SET_IF_MASKED - doable, but given that the union feels like a > micro-optimization in the first place the simpler approach of separating > fields seems okay too? (TBH my mind also initially went down the > FL_KEY_SET_IF_MASKED rabbit hole but once I saw the simplicity of > Cedric's patch I changed my mind) Sure, now this has been put to me more than once I agree. But if we go this way, then can we also simplify some of the existing logic? As a follow-up?
On Mon, 16 Mar 2026 08:34:47 +0000 Simon Horman wrote: > On Sat, Mar 14, 2026 at 10:00:02AM -0700, Jakub Kicinski wrote: > > On Fri, 13 Mar 2026 13:26:51 +0000 Simon Horman wrote: > > > It seems to me that the use of a union is intentional here, as either IPv4 > > > or IPv6 addresses can be present in each case - never both. And that > > > control.addr_type and enc_control.addr_type are intended to allow > > > differentiation of the address type in use for each of these unions. > > > > My reading was that the initial author simply wanted to save space in > > the struct. > > > > As the commit message explains this leads to complications in the logic > > which sets the keys. The alternative is to complicate > > FL_KEY_SET_IF_MASKED - doable, but given that the union feels like a > > micro-optimization in the first place the simpler approach of separating > > fields seems okay too? (TBH my mind also initially went down the > > FL_KEY_SET_IF_MASKED rabbit hole but once I saw the simplicity of > > Cedric's patch I changed my mind) > > Sure, now this has been put to me more than once I agree. > > But if we go this way, then can we also simplify some of the existing logic? > As a follow-up? Which logic do you have in mind? Sorry if I'm being slow.
On Mon, Mar 16, 2026 at 04:19:17PM -0700, Jakub Kicinski wrote: > On Mon, 16 Mar 2026 08:34:47 +0000 Simon Horman wrote: > > On Sat, Mar 14, 2026 at 10:00:02AM -0700, Jakub Kicinski wrote: > > > On Fri, 13 Mar 2026 13:26:51 +0000 Simon Horman wrote: > > > > It seems to me that the use of a union is intentional here, as either IPv4 > > > > or IPv6 addresses can be present in each case - never both. And that > > > > control.addr_type and enc_control.addr_type are intended to allow > > > > differentiation of the address type in use for each of these unions. > > > > > > My reading was that the initial author simply wanted to save space in > > > the struct. > > > > > > As the commit message explains this leads to complications in the logic > > > which sets the keys. The alternative is to complicate > > > FL_KEY_SET_IF_MASKED - doable, but given that the union feels like a > > > micro-optimization in the first place the simpler approach of separating > > > fields seems okay too? (TBH my mind also initially went down the > > > FL_KEY_SET_IF_MASKED rabbit hole but once I saw the simplicity of > > > Cedric's patch I changed my mind) > > > > Sure, now this has been put to me more than once I agree. > > > > But if we go this way, then can we also simplify some of the existing logic? > > As a follow-up? > > Which logic do you have in mind? Sorry if I'm being slow. Sorry for being vague. Let me do my homework and come back if I have a concrete suggestion.
© 2016 - 2026 Red Hat, Inc.