netdev - [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240603185647.2310748-8-amorenoz@redhat.com>
Date: Mon,  3 Jun 2024 20:56:41 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: netdev@...r.kernel.org
Cc: aconole@...hat.com,
	echaudro@...hat.com,
	horms@...nel.org,
	i.maximets@....org,
	dev@...nvswitch.org,
	Adrian Moreno <amorenoz@...hat.com>,
	Pravin B Shelar <pshelar@....org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample

The OVS_ACTION_ATTR_SAMPLE action is, in essence,
observability-oriented.

Apart from some corner case in which it's used a replacement of clone()
for old kernels, it's really only used for sFlow, IPFIX and now,
local emit_sample.

With this in mind, it doesn't make much sense to report
OVS_DROP_LAST_ACTION inside sample actions.

For instance, if the flow:

  actions:sample(..,emit_sample(..)),2

triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
confusing for users since the packet did reach its destination.

This patch makes internal action execution silently consume the skb
instead of notifying a drop for this case.

Unfortunately, this patch does not remove all potential sources of
confusion since, if the sample action itself is the last action, e.g:

    actions:sample(..,emit_sample(..))

we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.

Sadly, this case is difficult to solve without breaking the
optimization by which the skb is not cloned on last sample actions.
But, given explicit drop actions are now supported, OVS can just add one
after the last sample() and rewrite the flow as:

    actions:sample(..,emit_sample(..)),drop

Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
---
 net/openvswitch/actions.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 33f6d93ba5e4..54fc1abcff95 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
 static struct action_flow_keys __percpu *flow_keys;
 static DEFINE_PER_CPU(int, exec_actions_level);

+static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
+{
+	/* Do not emit packet drops inside sample(). */
+	if (OVS_CB(skb)->probability)
+		consume_skb(skb);
+	else
+		ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+}
+
 /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
  * space. Return NULL if out of key spaces.
  */
@@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 	if ((arg->probability != U32_MAX) &&
 	    (!arg->probability || get_random_u32() > arg->probability)) {
 		if (last)
-			ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+			ovs_drop_skb_last_action(skb);
 		return 0;
 	}

@@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 		}
 	}

-	ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+	ovs_drop_skb_last_action(skb);
 	return 0;
 }

-- 
2.45.1