[PATCH net-next 0/2] net: openvswitch: add drop action

[PATCH net-next 0/2] net: openvswitch: add drop action

[Eric Garver]

Prior to this series the "drop" action was implicit by an empty set of
actions. This series adds support for an explicit drop action. The
primary motivation is to allow passing xlate_error from userspace such
that xlater_error can be passed to kfree_skb_reason() and therefore
traced.                                                               

Eric Garver (2):
  net: openvswitch: add drop reasons
  net: openvswitch: add drop action

 include/net/dropreason.h                      |  6 ++++
 include/uapi/linux/openvswitch.h              |  2 ++
 net/openvswitch/actions.c                     | 13 +++++++
 net/openvswitch/datapath.c                    | 17 ++++++++++
 net/openvswitch/drop.h                        | 34 +++++++++++++++++++
 net/openvswitch/flow_netlink.c                | 12 ++++++-
 .../selftests/net/openvswitch/ovs-dpctl.py    |  3 ++
 7 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 net/openvswitch/drop.h

-- 
2.39.0

[PATCH net-next 1/2] net: openvswitch: add drop reasons

[Eric Garver]

These are counterparts to userspace's xlate_error values.

Signed-off-by: Eric Garver <eric@garver.life>
---
 include/net/dropreason.h   |  6 ++++++
 net/openvswitch/datapath.c | 17 +++++++++++++++++
 net/openvswitch/drop.h     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 net/openvswitch/drop.h

diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index 685fb37df8e8..653675bba758 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -23,6 +23,12 @@ enum skb_drop_reason_subsys {
 	 */
 	SKB_DROP_REASON_SUBSYS_MAC80211_MONITOR,
 
+	/**
+	 * @SKB_DROP_REASON_SUBSYS_OPENVSWITCH: openvswitch drop reasons
+	 * see net/openvswitch/drop.h
+	 */
+	SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
+
 	/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
 	SKB_DROP_REASON_SUBSYS_NUM
 };
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a6d2a0b1aa21..4ebdc52856ab 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -48,6 +48,7 @@
 #include "openvswitch_trace.h"
 #include "vport-internal_dev.h"
 #include "vport-netdev.h"
+#include "drop.h"
 
 unsigned int ovs_net_id __read_mostly;
 
@@ -2702,6 +2703,18 @@ static struct pernet_operations ovs_net_ops = {
 	.size = sizeof(struct ovs_net),
 };
 
+static const char * const ovs_drop_reasons[] = {
+	[0] = "OVS_XLATE_OK",
+#define S(x) #x,
+	OVS_DROP_REASONS(S)
+#undef S
+};
+
+static struct drop_reason_list drop_reason_list_ovs = {
+	.reasons = ovs_drop_reasons,
+	.n_reasons = ARRAY_SIZE(ovs_drop_reasons),
+};
+
 static int __init dp_init(void)
 {
 	int err;
@@ -2743,6 +2756,9 @@ static int __init dp_init(void)
 	if (err < 0)
 		goto error_unreg_netdev;
 
+	drop_reasons_register_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
+				     &drop_reason_list_ovs);
+
 	return 0;
 
 error_unreg_netdev:
@@ -2769,6 +2785,7 @@ static void dp_cleanup(void)
 	ovs_netdev_exit();
 	unregister_netdevice_notifier(&ovs_dp_device_notifier);
 	unregister_pernet_device(&ovs_net_ops);
+	drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH);
 	rcu_barrier();
 	ovs_vport_exit();
 	ovs_flow_exit();
diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h
new file mode 100644
index 000000000000..787eda0083c1
--- /dev/null
+++ b/net/openvswitch/drop.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * openvswitch drop reason list
+ */
+
+#ifndef OPENVSWITCH_DROP_H
+#define OPENVSWITCH_DROP_H
+#include <net/dropreason.h>
+
+/* these are counterparts to userspace xlate_error */
+#define OVS_DROP_REASONS(R)                      \
+	R(OVS_XLATE_BRIDGE_NOT_FOUND)            \
+	R(OVS_XLATE_RECURSION_TOO_DEEP)          \
+	R(OVS_XLATE_TOO_MANY_RESUBMITS)          \
+	R(OVS_XLATE_STACK_TOO_DEEP)              \
+	R(OVS_XLATE_NO_RECIRCULATION_CONTEXT)    \
+	R(OVS_XLATE_RECIRCULATION_CONFLICT)      \
+	R(OVS_XLATE_TOO_MANY_MPLS_LABELS)        \
+	R(OVS_XLATE_INVALID_TUNNEL_METADATA)     \
+	R(OVS_XLATE_UNSUPPORTED_PACKET_TYPE)     \
+	R(OVS_XLATE_CONGESTION_DROP)             \
+	R(OVS_XLATE_FORWARDING_DISABLED)         \
+	/* deliberate comment for trailing \ */
+
+enum ovs_drop_reason {
+	OVS_XLATE_OK = SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
+			SKB_DROP_REASON_SUBSYS_SHIFT,
+#define ENUM(x) x,
+	OVS_DROP_REASONS(ENUM)
+#undef ENUM
+	OVS_XLATE_MAX,
+};
+
+#endif /* OPENVSWITCH_DROP_H */
-- 
2.39.0

[PATCH net-next 2/2] net: openvswitch: add drop action

[Eric Garver]

This adds an explicit drop action. This is used by OVS to drop packets
for which it cannot determine what to do. An explicit action in the
kernel allows passing the reason _why_ the packet is being dropped. We
can then use perf tracing to match on the drop reason.

e.g. trace all OVS dropped skbs

 # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
 [..]
 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
  location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)

reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)

Signed-off-by: Eric Garver <eric@garver.life>
---
 include/uapi/linux/openvswitch.h                    |  2 ++
 net/openvswitch/actions.c                           | 13 +++++++++++++
 net/openvswitch/flow_netlink.c                      | 12 +++++++++++-
 .../testing/selftests/net/openvswitch/ovs-dpctl.py  |  3 +++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index e94870e77ee9..a967dbca3574 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -965,6 +965,7 @@ struct check_pkt_len_arg {
  * start of the packet or at the start of the l3 header depending on the value
  * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
  * argument.
+ * @OVS_ACTION_ATTR_DROP: Explicit drop action.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -1002,6 +1003,7 @@ enum ovs_action_attr {
 	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
 	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
 	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
+	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
 
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index cab1e02b63e0..4ad9a45dc042 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -32,6 +32,7 @@
 #include "vport.h"
 #include "flow_netlink.h"
 #include "openvswitch_trace.h"
+#include "drop.h"
 
 struct deferred_action {
 	struct sk_buff *skb;
@@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 				return dec_ttl_exception_handler(dp, skb,
 								 key, a);
 			break;
+
+		case OVS_ACTION_ATTR_DROP:
+			u32 reason = nla_get_u32(a);
+
+			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
+					SKB_DROP_REASON_SUBSYS_SHIFT;
+
+			if (reason == OVS_XLATE_OK)
+				break;
+
+			kfree_skb_reason(skb, reason);
+			return 0;
 		}
 
 		if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 41116361433d..23d39eae9a0d 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -39,6 +39,7 @@
 #include <net/erspan.h>
 
 #include "flow_netlink.h"
+#include "drop.h"
 
 struct ovs_len_tbl {
 	int len;
@@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_RECIRC:
 		case OVS_ACTION_ATTR_TRUNC:
 		case OVS_ACTION_ATTR_USERSPACE:
+		case OVS_ACTION_ATTR_DROP:
 			break;
 
 		case OVS_ACTION_ATTR_CT:
@@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
 	/* Whenever new actions are added, the need to update this
 	 * function should be considered.
 	 */
-	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
+	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
 
 	if (!actions)
 		return;
@@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
 			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
 			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
+			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			skip_copy = true;
 			break;
 
+		case OVS_ACTION_ATTR_DROP:
+			if (nla_get_u32(a) >=
+			    u32_get_bits(OVS_XLATE_MAX,
+					 ~SKB_DROP_REASON_SUBSYS_MASK))
+				return -EINVAL;
+			break;
+
 		default:
 			OVS_NLERR(log, "Unknown Action type %d", type);
 			return -EINVAL;
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 1c8b36bc15d4..526ebad7d514 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -115,6 +115,7 @@ class ovsactions(nla):
         ("OVS_ACTION_ATTR_CHECK_PKT_LEN", "none"),
         ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
         ("OVS_ACTION_ATTR_DEC_TTL", "none"),
+        ("OVS_ACTION_ATTR_DROP", "uint32"),
     )
 
     class ctact(nla):
@@ -261,6 +262,8 @@ class ovsactions(nla):
                     print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
                 elif field[0] == "OVS_ACTION_ATTR_TRUNC":
                     print_str += "trunc(%d)" % int(self.get_attr(field[0]))
+                elif field[0] == "OVS_ACTION_ATTR_DROP":
+                    print_str += "drop"
             elif field[1] == "flag":
                 if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
                     print_str += "ct_clear"
-- 
2.39.0

Re: [PATCH net-next 2/2] net: openvswitch: add drop action

[kernel test robot]

Hi Eric,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Garver/net-openvswitch-add-drop-reasons/20230630-043307
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230629203005.2137107-3-eric%40garver.life
patch subject: [PATCH net-next 2/2] net: openvswitch: add drop action
config: mips-randconfig-r036-20230629 (https://download.01.org/0day-ci/archive/20230630/202306300645.towwST4X-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce: (https://download.01.org/0day-ci/archive/20230630/202306300645.towwST4X-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306300645.towwST4X-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/openvswitch/actions.c:1483:4: error: expected expression
                           u32 reason = nla_get_u32(a);
                           ^
>> net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
                           reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
                           ^
   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
                           if (reason == OVS_XLATE_OK)
                               ^
   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
                           kfree_skb_reason(skb, reason);
                                                 ^
   4 errors generated.


vim +1483 net/openvswitch/actions.c

  1285	
  1286	/* Execute a list of actions against 'skb'. */
  1287	static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
  1288				      struct sw_flow_key *key,
  1289				      const struct nlattr *attr, int len)
  1290	{
  1291		const struct nlattr *a;
  1292		int rem;
  1293	
  1294		for (a = attr, rem = len; rem > 0;
  1295		     a = nla_next(a, &rem)) {
  1296			int err = 0;
  1297	
  1298			if (trace_ovs_do_execute_action_enabled())
  1299				trace_ovs_do_execute_action(dp, skb, key, a, rem);
  1300	
  1301			switch (nla_type(a)) {
  1302			case OVS_ACTION_ATTR_OUTPUT: {
  1303				int port = nla_get_u32(a);
  1304				struct sk_buff *clone;
  1305	
  1306				/* Every output action needs a separate clone
  1307				 * of 'skb', In case the output action is the
  1308				 * last action, cloning can be avoided.
  1309				 */
  1310				if (nla_is_last(a, rem)) {
  1311					do_output(dp, skb, port, key);
  1312					/* 'skb' has been used for output.
  1313					 */
  1314					return 0;
  1315				}
  1316	
  1317				clone = skb_clone(skb, GFP_ATOMIC);
  1318				if (clone)
  1319					do_output(dp, clone, port, key);
  1320				OVS_CB(skb)->cutlen = 0;
  1321				break;
  1322			}
  1323	
  1324			case OVS_ACTION_ATTR_TRUNC: {
  1325				struct ovs_action_trunc *trunc = nla_data(a);
  1326	
  1327				if (skb->len > trunc->max_len)
  1328					OVS_CB(skb)->cutlen = skb->len - trunc->max_len;
  1329				break;
  1330			}
  1331	
  1332			case OVS_ACTION_ATTR_USERSPACE:
  1333				output_userspace(dp, skb, key, a, attr,
  1334							     len, OVS_CB(skb)->cutlen);
  1335				OVS_CB(skb)->cutlen = 0;
  1336				break;
  1337	
  1338			case OVS_ACTION_ATTR_HASH:
  1339				execute_hash(skb, key, a);
  1340				break;
  1341	
  1342			case OVS_ACTION_ATTR_PUSH_MPLS: {
  1343				struct ovs_action_push_mpls *mpls = nla_data(a);
  1344	
  1345				err = push_mpls(skb, key, mpls->mpls_lse,
  1346						mpls->mpls_ethertype, skb->mac_len);
  1347				break;
  1348			}
  1349			case OVS_ACTION_ATTR_ADD_MPLS: {
  1350				struct ovs_action_add_mpls *mpls = nla_data(a);
  1351				__u16 mac_len = 0;
  1352	
  1353				if (mpls->tun_flags & OVS_MPLS_L3_TUNNEL_FLAG_MASK)
  1354					mac_len = skb->mac_len;
  1355	
  1356				err = push_mpls(skb, key, mpls->mpls_lse,
  1357						mpls->mpls_ethertype, mac_len);
  1358				break;
  1359			}
  1360			case OVS_ACTION_ATTR_POP_MPLS:
  1361				err = pop_mpls(skb, key, nla_get_be16(a));
  1362				break;
  1363	
  1364			case OVS_ACTION_ATTR_PUSH_VLAN:
  1365				err = push_vlan(skb, key, nla_data(a));
  1366				break;
  1367	
  1368			case OVS_ACTION_ATTR_POP_VLAN:
  1369				err = pop_vlan(skb, key);
  1370				break;
  1371	
  1372			case OVS_ACTION_ATTR_RECIRC: {
  1373				bool last = nla_is_last(a, rem);
  1374	
  1375				err = execute_recirc(dp, skb, key, a, last);
  1376				if (last) {
  1377					/* If this is the last action, the skb has
  1378					 * been consumed or freed.
  1379					 * Return immediately.
  1380					 */
  1381					return err;
  1382				}
  1383				break;
  1384			}
  1385	
  1386			case OVS_ACTION_ATTR_SET:
  1387				err = execute_set_action(skb, key, nla_data(a));
  1388				break;
  1389	
  1390			case OVS_ACTION_ATTR_SET_MASKED:
  1391			case OVS_ACTION_ATTR_SET_TO_MASKED:
  1392				err = execute_masked_set_action(skb, key, nla_data(a));
  1393				break;
  1394	
  1395			case OVS_ACTION_ATTR_SAMPLE: {
  1396				bool last = nla_is_last(a, rem);
  1397	
  1398				err = sample(dp, skb, key, a, last);
  1399				if (last)
  1400					return err;
  1401	
  1402				break;
  1403			}
  1404	
  1405			case OVS_ACTION_ATTR_CT:
  1406				if (!is_flow_key_valid(key)) {
  1407					err = ovs_flow_key_update(skb, key);
  1408					if (err)
  1409						return err;
  1410				}
  1411	
  1412				err = ovs_ct_execute(ovs_dp_get_net(dp), skb, key,
  1413						     nla_data(a));
  1414	
  1415				/* Hide stolen IP fragments from user space. */
  1416				if (err)
  1417					return err == -EINPROGRESS ? 0 : err;
  1418				break;
  1419	
  1420			case OVS_ACTION_ATTR_CT_CLEAR:
  1421				err = ovs_ct_clear(skb, key);
  1422				break;
  1423	
  1424			case OVS_ACTION_ATTR_PUSH_ETH:
  1425				err = push_eth(skb, key, nla_data(a));
  1426				break;
  1427	
  1428			case OVS_ACTION_ATTR_POP_ETH:
  1429				err = pop_eth(skb, key);
  1430				break;
  1431	
  1432			case OVS_ACTION_ATTR_PUSH_NSH: {
  1433				u8 buffer[NSH_HDR_MAX_LEN];
  1434				struct nshhdr *nh = (struct nshhdr *)buffer;
  1435	
  1436				err = nsh_hdr_from_nlattr(nla_data(a), nh,
  1437							  NSH_HDR_MAX_LEN);
  1438				if (unlikely(err))
  1439					break;
  1440				err = push_nsh(skb, key, nh);
  1441				break;
  1442			}
  1443	
  1444			case OVS_ACTION_ATTR_POP_NSH:
  1445				err = pop_nsh(skb, key);
  1446				break;
  1447	
  1448			case OVS_ACTION_ATTR_METER:
  1449				if (ovs_meter_execute(dp, skb, key, nla_get_u32(a))) {
  1450					consume_skb(skb);
  1451					return 0;
  1452				}
  1453				break;
  1454	
  1455			case OVS_ACTION_ATTR_CLONE: {
  1456				bool last = nla_is_last(a, rem);
  1457	
  1458				err = clone(dp, skb, key, a, last);
  1459				if (last)
  1460					return err;
  1461	
  1462				break;
  1463			}
  1464	
  1465			case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
  1466				bool last = nla_is_last(a, rem);
  1467	
  1468				err = execute_check_pkt_len(dp, skb, key, a, last);
  1469				if (last)
  1470					return err;
  1471	
  1472				break;
  1473			}
  1474	
  1475			case OVS_ACTION_ATTR_DEC_TTL:
  1476				err = execute_dec_ttl(skb, key);
  1477				if (err == -EHOSTUNREACH)
  1478					return dec_ttl_exception_handler(dp, skb,
  1479									 key, a);
  1480				break;
  1481	
  1482			case OVS_ACTION_ATTR_DROP:
> 1483				u32 reason = nla_get_u32(a);
  1484	
> 1485				reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
  1486						SKB_DROP_REASON_SUBSYS_SHIFT;
  1487	
  1488				if (reason == OVS_XLATE_OK)
  1489					break;
  1490	
  1491				kfree_skb_reason(skb, reason);
  1492				return 0;
  1493			}
  1494	
  1495			if (unlikely(err)) {
  1496				kfree_skb(skb);
  1497				return err;
  1498			}
  1499		}
  1500	
  1501		consume_skb(skb);
  1502		return 0;
  1503	}
  1504	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 2/2] net: openvswitch: add drop action

[kernel test robot]

Hi Eric,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Garver/net-openvswitch-add-drop-reasons/20230630-043307
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230629203005.2137107-3-eric%40garver.life
patch subject: [PATCH net-next 2/2] net: openvswitch: add drop action
config: hexagon-randconfig-r045-20230629 (https://download.01.org/0day-ci/archive/20230630/202306300609.tdRdZscy-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230630/202306300609.tdRdZscy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306300609.tdRdZscy-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/openvswitch/actions.c:8:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from net/openvswitch/actions.c:8:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from net/openvswitch/actions.c:8:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> net/openvswitch/actions.c:1483:4: error: expected expression
    1483 |                         u32 reason = nla_get_u32(a);
         |                         ^
>> net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
    1485 |                         reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
         |                         ^
   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
    1488 |                         if (reason == OVS_XLATE_OK)
         |                             ^
   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
    1491 |                         kfree_skb_reason(skb, reason);
         |                                               ^
   6 warnings and 6 errors generated.


vim +1483 net/openvswitch/actions.c

  1285	
  1286	/* Execute a list of actions against 'skb'. */
  1287	static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
  1288				      struct sw_flow_key *key,
  1289				      const struct nlattr *attr, int len)
  1290	{
  1291		const struct nlattr *a;
  1292		int rem;
  1293	
  1294		for (a = attr, rem = len; rem > 0;
  1295		     a = nla_next(a, &rem)) {
  1296			int err = 0;
  1297	
  1298			if (trace_ovs_do_execute_action_enabled())
  1299				trace_ovs_do_execute_action(dp, skb, key, a, rem);
  1300	
  1301			switch (nla_type(a)) {
  1302			case OVS_ACTION_ATTR_OUTPUT: {
  1303				int port = nla_get_u32(a);
  1304				struct sk_buff *clone;
  1305	
  1306				/* Every output action needs a separate clone
  1307				 * of 'skb', In case the output action is the
  1308				 * last action, cloning can be avoided.
  1309				 */
  1310				if (nla_is_last(a, rem)) {
  1311					do_output(dp, skb, port, key);
  1312					/* 'skb' has been used for output.
  1313					 */
  1314					return 0;
  1315				}
  1316	
  1317				clone = skb_clone(skb, GFP_ATOMIC);
  1318				if (clone)
  1319					do_output(dp, clone, port, key);
  1320				OVS_CB(skb)->cutlen = 0;
  1321				break;
  1322			}
  1323	
  1324			case OVS_ACTION_ATTR_TRUNC: {
  1325				struct ovs_action_trunc *trunc = nla_data(a);
  1326	
  1327				if (skb->len > trunc->max_len)
  1328					OVS_CB(skb)->cutlen = skb->len - trunc->max_len;
  1329				break;
  1330			}
  1331	
  1332			case OVS_ACTION_ATTR_USERSPACE:
  1333				output_userspace(dp, skb, key, a, attr,
  1334							     len, OVS_CB(skb)->cutlen);
  1335				OVS_CB(skb)->cutlen = 0;
  1336				break;
  1337	
  1338			case OVS_ACTION_ATTR_HASH:
  1339				execute_hash(skb, key, a);
  1340				break;
  1341	
  1342			case OVS_ACTION_ATTR_PUSH_MPLS: {
  1343				struct ovs_action_push_mpls *mpls = nla_data(a);
  1344	
  1345				err = push_mpls(skb, key, mpls->mpls_lse,
  1346						mpls->mpls_ethertype, skb->mac_len);
  1347				break;
  1348			}
  1349			case OVS_ACTION_ATTR_ADD_MPLS: {
  1350				struct ovs_action_add_mpls *mpls = nla_data(a);
  1351				__u16 mac_len = 0;
  1352	
  1353				if (mpls->tun_flags & OVS_MPLS_L3_TUNNEL_FLAG_MASK)
  1354					mac_len = skb->mac_len;
  1355	
  1356				err = push_mpls(skb, key, mpls->mpls_lse,
  1357						mpls->mpls_ethertype, mac_len);
  1358				break;
  1359			}
  1360			case OVS_ACTION_ATTR_POP_MPLS:
  1361				err = pop_mpls(skb, key, nla_get_be16(a));
  1362				break;
  1363	
  1364			case OVS_ACTION_ATTR_PUSH_VLAN:
  1365				err = push_vlan(skb, key, nla_data(a));
  1366				break;
  1367	
  1368			case OVS_ACTION_ATTR_POP_VLAN:
  1369				err = pop_vlan(skb, key);
  1370				break;
  1371	
  1372			case OVS_ACTION_ATTR_RECIRC: {
  1373				bool last = nla_is_last(a, rem);
  1374	
  1375				err = execute_recirc(dp, skb, key, a, last);
  1376				if (last) {
  1377					/* If this is the last action, the skb has
  1378					 * been consumed or freed.
  1379					 * Return immediately.
  1380					 */
  1381					return err;
  1382				}
  1383				break;
  1384			}
  1385	
  1386			case OVS_ACTION_ATTR_SET:
  1387				err = execute_set_action(skb, key, nla_data(a));
  1388				break;
  1389	
  1390			case OVS_ACTION_ATTR_SET_MASKED:
  1391			case OVS_ACTION_ATTR_SET_TO_MASKED:
  1392				err = execute_masked_set_action(skb, key, nla_data(a));
  1393				break;
  1394	
  1395			case OVS_ACTION_ATTR_SAMPLE: {
  1396				bool last = nla_is_last(a, rem);
  1397	
  1398				err = sample(dp, skb, key, a, last);
  1399				if (last)
  1400					return err;
  1401	
  1402				break;
  1403			}
  1404	
  1405			case OVS_ACTION_ATTR_CT:
  1406				if (!is_flow_key_valid(key)) {
  1407					err = ovs_flow_key_update(skb, key);
  1408					if (err)
  1409						return err;
  1410				}
  1411	
  1412				err = ovs_ct_execute(ovs_dp_get_net(dp), skb, key,
  1413						     nla_data(a));
  1414	
  1415				/* Hide stolen IP fragments from user space. */
  1416				if (err)
  1417					return err == -EINPROGRESS ? 0 : err;
  1418				break;
  1419	
  1420			case OVS_ACTION_ATTR_CT_CLEAR:
  1421				err = ovs_ct_clear(skb, key);
  1422				break;
  1423	
  1424			case OVS_ACTION_ATTR_PUSH_ETH:
  1425				err = push_eth(skb, key, nla_data(a));
  1426				break;
  1427	
  1428			case OVS_ACTION_ATTR_POP_ETH:
  1429				err = pop_eth(skb, key);
  1430				break;
  1431	
  1432			case OVS_ACTION_ATTR_PUSH_NSH: {
  1433				u8 buffer[NSH_HDR_MAX_LEN];
  1434				struct nshhdr *nh = (struct nshhdr *)buffer;
  1435	
  1436				err = nsh_hdr_from_nlattr(nla_data(a), nh,
  1437							  NSH_HDR_MAX_LEN);
  1438				if (unlikely(err))
  1439					break;
  1440				err = push_nsh(skb, key, nh);
  1441				break;
  1442			}
  1443	
  1444			case OVS_ACTION_ATTR_POP_NSH:
  1445				err = pop_nsh(skb, key);
  1446				break;
  1447	
  1448			case OVS_ACTION_ATTR_METER:
  1449				if (ovs_meter_execute(dp, skb, key, nla_get_u32(a))) {
  1450					consume_skb(skb);
  1451					return 0;
  1452				}
  1453				break;
  1454	
  1455			case OVS_ACTION_ATTR_CLONE: {
  1456				bool last = nla_is_last(a, rem);
  1457	
  1458				err = clone(dp, skb, key, a, last);
  1459				if (last)
  1460					return err;
  1461	
  1462				break;
  1463			}
  1464	
  1465			case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
  1466				bool last = nla_is_last(a, rem);
  1467	
  1468				err = execute_check_pkt_len(dp, skb, key, a, last);
  1469				if (last)
  1470					return err;
  1471	
  1472				break;
  1473			}
  1474	
  1475			case OVS_ACTION_ATTR_DEC_TTL:
  1476				err = execute_dec_ttl(skb, key);
  1477				if (err == -EHOSTUNREACH)
  1478					return dec_ttl_exception_handler(dp, skb,
  1479									 key, a);
  1480				break;
  1481	
  1482			case OVS_ACTION_ATTR_DROP:
> 1483				u32 reason = nla_get_u32(a);
  1484	
> 1485				reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
  1486						SKB_DROP_REASON_SUBSYS_SHIFT;
  1487	
  1488				if (reason == OVS_XLATE_OK)
  1489					break;
  1490	
  1491				kfree_skb_reason(skb, reason);
  1492				return 0;
  1493			}
  1494	
  1495			if (unlikely(err)) {
  1496				kfree_skb(skb);
  1497				return err;
  1498			}
  1499		}
  1500	
  1501		consume_skb(skb);
  1502		return 0;
  1503	}
  1504	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 2/2] net: openvswitch: add drop action

[Simon Horman]

On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
> 
> e.g. trace all OVS dropped skbs
> 
>  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
>  [..]
>  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
>   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
> 
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> 
> Signed-off-by: Eric Garver <eric@garver.life>

...

> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
>  #include "vport.h"
>  #include "flow_netlink.h"
>  #include "openvswitch_trace.h"
> +#include "drop.h"
>  
>  struct deferred_action {
>  	struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  				return dec_ttl_exception_handler(dp, skb,
>  								 key, a);
>  			break;
> +
> +		case OVS_ACTION_ATTR_DROP:
> +			u32 reason = nla_get_u32(a);
> +
> +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> +					SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> +			if (reason == OVS_XLATE_OK)
> +				break;
> +
> +			kfree_skb_reason(skb, reason);
> +			return 0;
>  		}

Hi Eric,

thanks for your patches. This is an interesting new feature.

unfortunately clang-16 doesn't seem to like this very much.
I think that it is due to the declaration of reason not
being enclosed in a block - { }.

  net/openvswitch/actions.c:1483:4: error: expected expression
                          u32 reason = nla_get_u32(a);
                          ^
  net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
                          reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
                          ^
  net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
                          if (reason == OVS_XLATE_OK)
                              ^
  net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
                          kfree_skb_reason(skb, reason);
                                                ^
  4 errors generated.


net-next is currently closed.
So please provide a v2 once it reposts, after 10th July.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

-- 
pw-bot: changes-requested

Re: [PATCH net-next 2/2] net: openvswitch: add drop action

[Eric Garver]

On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > This adds an explicit drop action. This is used by OVS to drop packets
> > for which it cannot determine what to do. An explicit action in the
> > kernel allows passing the reason _why_ the packet is being dropped. We
> > can then use perf tracing to match on the drop reason.
> > 
> > e.g. trace all OVS dropped skbs
> > 
> >  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
> >  [..]
> >  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
> >   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
> > 
> > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> > 
> > Signed-off-by: Eric Garver <eric@garver.life>
> 
> ...
> 
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -32,6 +32,7 @@
> >  #include "vport.h"
> >  #include "flow_netlink.h"
> >  #include "openvswitch_trace.h"
> > +#include "drop.h"
> >  
> >  struct deferred_action {
> >  	struct sk_buff *skb;
> > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  				return dec_ttl_exception_handler(dp, skb,
> >  								 key, a);
> >  			break;
> > +
> > +		case OVS_ACTION_ATTR_DROP:
> > +			u32 reason = nla_get_u32(a);
> > +
> > +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > +					SKB_DROP_REASON_SUBSYS_SHIFT;
> > +
> > +			if (reason == OVS_XLATE_OK)
> > +				break;
> > +
> > +			kfree_skb_reason(skb, reason);
> > +			return 0;
> >  		}
> 
> Hi Eric,
> 
> thanks for your patches. This is an interesting new feature.
> 
> unfortunately clang-16 doesn't seem to like this very much.
> I think that it is due to the declaration of reason not
> being enclosed in a block - { }.
> 
>   net/openvswitch/actions.c:1483:4: error: expected expression
>                           u32 reason = nla_get_u32(a);
>                           ^
>   net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
>                           reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
>                           ^
>   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
>                           if (reason == OVS_XLATE_OK)
>                               ^
>   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
>                           kfree_skb_reason(skb, reason);
>                                                 ^
>   4 errors generated.
> 
> 
> net-next is currently closed.
> So please provide a v2 once it reposts, after 10th July.

oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.

Thanks.
Eric.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Simon Horman]

On Fri, Jun 30, 2023 at 08:29:58AM -0400, Eric Garver wrote:
> On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> > On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > > This adds an explicit drop action. This is used by OVS to drop packets
> > > for which it cannot determine what to do. An explicit action in the
> > > kernel allows passing the reason _why_ the packet is being dropped. We
> > > can then use perf tracing to match on the drop reason.
> > > 
> > > e.g. trace all OVS dropped skbs
> > > 
> > >  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
> > >  [..]
> > >  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
> > >   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
> > > 
> > > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> > > 
> > > Signed-off-by: Eric Garver <eric@garver.life>
> > 
> > ...
> > 
> > > --- a/net/openvswitch/actions.c
> > > +++ b/net/openvswitch/actions.c
> > > @@ -32,6 +32,7 @@
> > >  #include "vport.h"
> > >  #include "flow_netlink.h"
> > >  #include "openvswitch_trace.h"
> > > +#include "drop.h"
> > >  
> > >  struct deferred_action {
> > >  	struct sk_buff *skb;
> > > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> > >  				return dec_ttl_exception_handler(dp, skb,
> > >  								 key, a);
> > >  			break;
> > > +
> > > +		case OVS_ACTION_ATTR_DROP:
> > > +			u32 reason = nla_get_u32(a);
> > > +
> > > +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > > +					SKB_DROP_REASON_SUBSYS_SHIFT;
> > > +
> > > +			if (reason == OVS_XLATE_OK)
> > > +				break;
> > > +
> > > +			kfree_skb_reason(skb, reason);
> > > +			return 0;
> > >  		}
> > 
> > Hi Eric,
> > 
> > thanks for your patches. This is an interesting new feature.
> > 
> > unfortunately clang-16 doesn't seem to like this very much.
> > I think that it is due to the declaration of reason not
> > being enclosed in a block - { }.
> > 
> >   net/openvswitch/actions.c:1483:4: error: expected expression
> >                           u32 reason = nla_get_u32(a);
> >                           ^
> >   net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
> >                           reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> >                           ^
> >   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
> >                           if (reason == OVS_XLATE_OK)
> >                               ^
> >   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
> >                           kfree_skb_reason(skb, reason);
> >                                                 ^
> >   4 errors generated.
> > 
> > 
> > net-next is currently closed.
> > So please provide a v2 once it reposts, after 10th July.
> 
> oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.

Thanks Eric,

much appreciated.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Aaron Conole]

Eric Garver <eric@garver.life> writes:

> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
>
> e.g. trace all OVS dropped skbs
>
>  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
>  [..]
>  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
>   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
>
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>
> Signed-off-by: Eric Garver <eric@garver.life>
> ---
>  include/uapi/linux/openvswitch.h                    |  2 ++
>  net/openvswitch/actions.c                           | 13 +++++++++++++
>  net/openvswitch/flow_netlink.c                      | 12 +++++++++++-
>  .../testing/selftests/net/openvswitch/ovs-dpctl.py  |  3 +++
>  4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index e94870e77ee9..a967dbca3574 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
>   * start of the packet or at the start of the l3 header depending on the value
>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>   * argument.
> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>   *
>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
>  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
> +	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
>  
>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>  				       * from userspace. */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index cab1e02b63e0..4ad9a45dc042 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
>  #include "vport.h"
>  #include "flow_netlink.h"
>  #include "openvswitch_trace.h"
> +#include "drop.h"
>  
>  struct deferred_action {
>  	struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  				return dec_ttl_exception_handler(dp, skb,
>  								 key, a);
>  			break;
> +
> +		case OVS_ACTION_ATTR_DROP:
> +			u32 reason = nla_get_u32(a);
> +
> +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> +					SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> +			if (reason == OVS_XLATE_OK)
> +				break;
> +
> +			kfree_skb_reason(skb, reason);
> +			return 0;
>  		}
>  
>  		if (unlikely(err)) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 41116361433d..23d39eae9a0d 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -39,6 +39,7 @@
>  #include <net/erspan.h>
>  
>  #include "flow_netlink.h"
> +#include "drop.h"
>  
>  struct ovs_len_tbl {
>  	int len;
> @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>  		case OVS_ACTION_ATTR_RECIRC:
>  		case OVS_ACTION_ATTR_TRUNC:
>  		case OVS_ACTION_ATTR_USERSPACE:
> +		case OVS_ACTION_ATTR_DROP:
>  			break;
>  
>  		case OVS_ACTION_ATTR_CT:
> @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
>  	/* Whenever new actions are added, the need to update this
>  	 * function should be considered.
>  	 */
> -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
> +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
>  
>  	if (!actions)
>  		return;
> @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
>  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> +			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
>  		};
>  		const struct ovs_action_push_vlan *vlan;
>  		int type = nla_type(a);
> @@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			skip_copy = true;
>  			break;
>  
> +		case OVS_ACTION_ATTR_DROP:
> +			if (nla_get_u32(a) >=
> +			    u32_get_bits(OVS_XLATE_MAX,
> +					 ~SKB_DROP_REASON_SUBSYS_MASK))
> +				return -EINVAL;
> +			break;
> +

If there's a case where the userspace sends a drop reason that isn't
known to the kernel, we will reject the flow, and the only "close" drop
will be OVS_XLATE_OK, which would be wrong.  Is there a reason to do
this?  For example, userspace might get new support for some kind of
flows and during that time might have a new xlate drop reason.  Maybe we
can have a reason code that OVS knows will exist, so that if this fails,
it can at least fall back to that?

>  		default:
>  			OVS_NLERR(log, "Unknown Action type %d", type);
>  			return -EINVAL;
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1c8b36bc15d4..526ebad7d514 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -115,6 +115,7 @@ class ovsactions(nla):
>          ("OVS_ACTION_ATTR_CHECK_PKT_LEN", "none"),
>          ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
>          ("OVS_ACTION_ATTR_DEC_TTL", "none"),
> +        ("OVS_ACTION_ATTR_DROP", "uint32"),
>      )
>  
>      class ctact(nla):
> @@ -261,6 +262,8 @@ class ovsactions(nla):
>                      print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
>                  elif field[0] == "OVS_ACTION_ATTR_TRUNC":
>                      print_str += "trunc(%d)" % int(self.get_attr(field[0]))
> +                elif field[0] == "OVS_ACTION_ATTR_DROP":
> +                    print_str += "drop"

Can we also include the reason here?

>              elif field[1] == "flag":
>                  if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
>                      print_str += "ct_clear"

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Eric Garver]

On Thu, Jul 06, 2023 at 08:54:16AM -0400, Aaron Conole wrote:
> Eric Garver <eric@garver.life> writes:
> 
> > This adds an explicit drop action. This is used by OVS to drop packets
> > for which it cannot determine what to do. An explicit action in the
> > kernel allows passing the reason _why_ the packet is being dropped. We
> > can then use perf tracing to match on the drop reason.
> >
> > e.g. trace all OVS dropped skbs
> >
> >  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
> >  [..]
> >  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
> >   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
> >
> > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> >
> > Signed-off-by: Eric Garver <eric@garver.life>
> > ---
> >  include/uapi/linux/openvswitch.h                    |  2 ++
> >  net/openvswitch/actions.c                           | 13 +++++++++++++
> >  net/openvswitch/flow_netlink.c                      | 12 +++++++++++-
> >  .../testing/selftests/net/openvswitch/ovs-dpctl.py  |  3 +++
> >  4 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index e94870e77ee9..a967dbca3574 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
> >   * start of the packet or at the start of the l3 header depending on the value
> >   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> >   * argument.
> > + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> >   *
> >   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
> >   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> > @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
> >  	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> >  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
> >  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
> > +	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
> >  
> >  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
> >  				       * from userspace. */
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index cab1e02b63e0..4ad9a45dc042 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -32,6 +32,7 @@
> >  #include "vport.h"
> >  #include "flow_netlink.h"
> >  #include "openvswitch_trace.h"
> > +#include "drop.h"
> >  
> >  struct deferred_action {
> >  	struct sk_buff *skb;
> > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  				return dec_ttl_exception_handler(dp, skb,
> >  								 key, a);
> >  			break;
> > +
> > +		case OVS_ACTION_ATTR_DROP:
> > +			u32 reason = nla_get_u32(a);
> > +
> > +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > +					SKB_DROP_REASON_SUBSYS_SHIFT;
> > +
> > +			if (reason == OVS_XLATE_OK)
> > +				break;
> > +
> > +			kfree_skb_reason(skb, reason);
> > +			return 0;
> >  		}
> >  
> >  		if (unlikely(err)) {
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 41116361433d..23d39eae9a0d 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -39,6 +39,7 @@
> >  #include <net/erspan.h>
> >  
> >  #include "flow_netlink.h"
> > +#include "drop.h"
> >  
> >  struct ovs_len_tbl {
> >  	int len;
> > @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> >  		case OVS_ACTION_ATTR_RECIRC:
> >  		case OVS_ACTION_ATTR_TRUNC:
> >  		case OVS_ACTION_ATTR_USERSPACE:
> > +		case OVS_ACTION_ATTR_DROP:
> >  			break;
> >  
> >  		case OVS_ACTION_ATTR_CT:
> > @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
> >  	/* Whenever new actions are added, the need to update this
> >  	 * function should be considered.
> >  	 */
> > -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
> > +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
> >  
> >  	if (!actions)
> >  		return;
> > @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >  			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> >  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
> >  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> > +			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
> >  		};
> >  		const struct ovs_action_push_vlan *vlan;
> >  		int type = nla_type(a);
> > @@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >  			skip_copy = true;
> >  			break;
> >  
> > +		case OVS_ACTION_ATTR_DROP:
> > +			if (nla_get_u32(a) >=
> > +			    u32_get_bits(OVS_XLATE_MAX,
> > +					 ~SKB_DROP_REASON_SUBSYS_MASK))
> > +				return -EINVAL;
> > +			break;
> > +
> 
> If there's a case where the userspace sends a drop reason that isn't
> known to the kernel, we will reject the flow, and the only "close" drop
> will be OVS_XLATE_OK, which would be wrong.  Is there a reason to do
> this?  For example, userspace might get new support for some kind of
> flows and during that time might have a new xlate drop reason.  Maybe we
> can have a reason code that OVS knows will exist, so that if this fails,
> it can at least fall back to that?

You're correct. It will reject the flow.

Maybe we clamp the value to OVS_XLATE_MAX if it's unknown. That makes
the skb drop reason less helpful, but no less helpful than today ;). At
least we won't reject the flow.

We could alias OVS_XLATE_MAX to OVS_XLATE_UNKNOWN. I prefer an explicit
value for OVS_XLATE_UNKNOWN, e.g. (u16)-1.

> >  		default:
> >  			OVS_NLERR(log, "Unknown Action type %d", type);
> >  			return -EINVAL;
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index 1c8b36bc15d4..526ebad7d514 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -115,6 +115,7 @@ class ovsactions(nla):
> >          ("OVS_ACTION_ATTR_CHECK_PKT_LEN", "none"),
> >          ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
> >          ("OVS_ACTION_ATTR_DEC_TTL", "none"),
> > +        ("OVS_ACTION_ATTR_DROP", "uint32"),
> >      )
> >  
> >      class ctact(nla):
> > @@ -261,6 +262,8 @@ class ovsactions(nla):
> >                      print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
> >                  elif field[0] == "OVS_ACTION_ATTR_TRUNC":
> >                      print_str += "trunc(%d)" % int(self.get_attr(field[0]))
> > +                elif field[0] == "OVS_ACTION_ATTR_DROP":
> > +                    print_str += "drop"
> 
> Can we also include the reason here?

It could. This mimics dpctl output, which does currently not include the
reason. So I went with parity.

I have proposed adding the reason to all dpctl "drop" output, but was
planning on that being a follow up to this work.

> >              elif field[1] == "flag":
> >                  if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
> >                      print_str += "ct_clear"
> 
> 

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Ilya Maximets]

On 7/6/23 15:57, Eric Garver wrote:
> On Thu, Jul 06, 2023 at 08:54:16AM -0400, Aaron Conole wrote:
>> Eric Garver <eric@garver.life> writes:
>>
>>> This adds an explicit drop action. This is used by OVS to drop packets
>>> for which it cannot determine what to do. An explicit action in the
>>> kernel allows passing the reason _why_ the packet is being dropped. We
>>> can then use perf tracing to match on the drop reason.
>>>
>>> e.g. trace all OVS dropped skbs
>>>
>>>  # perf trace -e skb:kfree_skb --filter="reason >= 0x30000"
>>>  [..]
>>>  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xffffa0e8765f2000, \
>>>   location:0xffffffffc0d9b462, protocol: 2048, reason: 196610)
>>>
>>> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>>>
>>> Signed-off-by: Eric Garver <eric@garver.life>
>>> ---
>>>  include/uapi/linux/openvswitch.h                    |  2 ++
>>>  net/openvswitch/actions.c                           | 13 +++++++++++++
>>>  net/openvswitch/flow_netlink.c                      | 12 +++++++++++-
>>>  .../testing/selftests/net/openvswitch/ovs-dpctl.py  |  3 +++
>>>  4 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>> index e94870e77ee9..a967dbca3574 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
>>>   * start of the packet or at the start of the l3 header depending on the value
>>>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>>   * argument.
>>> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>>   *
>>>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>>>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
>>> @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
>>>  	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>>  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
>>>  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
>>> +	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
>>>  
>>>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>>>  				       * from userspace. */
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index cab1e02b63e0..4ad9a45dc042 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -32,6 +32,7 @@
>>>  #include "vport.h"
>>>  #include "flow_netlink.h"
>>>  #include "openvswitch_trace.h"
>>> +#include "drop.h"
>>>  
>>>  struct deferred_action {
>>>  	struct sk_buff *skb;
>>> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>>  				return dec_ttl_exception_handler(dp, skb,
>>>  								 key, a);
>>>  			break;
>>> +
>>> +		case OVS_ACTION_ATTR_DROP:
>>> +			u32 reason = nla_get_u32(a);
>>> +
>>> +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
>>> +					SKB_DROP_REASON_SUBSYS_SHIFT;
>>> +
>>> +			if (reason == OVS_XLATE_OK)
>>> +				break;
>>> +
>>> +			kfree_skb_reason(skb, reason);
>>> +			return 0;
>>>  		}
>>>  
>>>  		if (unlikely(err)) {
>>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>> index 41116361433d..23d39eae9a0d 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>> @@ -39,6 +39,7 @@
>>>  #include <net/erspan.h>
>>>  
>>>  #include "flow_netlink.h"
>>> +#include "drop.h"
>>>  
>>>  struct ovs_len_tbl {
>>>  	int len;
>>> @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>>>  		case OVS_ACTION_ATTR_RECIRC:
>>>  		case OVS_ACTION_ATTR_TRUNC:
>>>  		case OVS_ACTION_ATTR_USERSPACE:
>>> +		case OVS_ACTION_ATTR_DROP:
>>>  			break;
>>>  
>>>  		case OVS_ACTION_ATTR_CT:
>>> @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
>>>  	/* Whenever new actions are added, the need to update this
>>>  	 * function should be considered.
>>>  	 */
>>> -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
>>> +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
>>>  
>>>  	if (!actions)
>>>  		return;
>>> @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>>  			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>>>  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
>>>  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
>>> +			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
>>>  		};
>>>  		const struct ovs_action_push_vlan *vlan;
>>>  		int type = nla_type(a);
>>> @@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>>>  			skip_copy = true;
>>>  			break;
>>>  
>>> +		case OVS_ACTION_ATTR_DROP:
>>> +			if (nla_get_u32(a) >=
>>> +			    u32_get_bits(OVS_XLATE_MAX,
>>> +					 ~SKB_DROP_REASON_SUBSYS_MASK))
>>> +				return -EINVAL;
>>> +			break;
>>> +
>>
>> If there's a case where the userspace sends a drop reason that isn't
>> known to the kernel, we will reject the flow, and the only "close" drop
>> will be OVS_XLATE_OK, which would be wrong.  Is there a reason to do
>> this?  For example, userspace might get new support for some kind of
>> flows and during that time might have a new xlate drop reason.  Maybe we
>> can have a reason code that OVS knows will exist, so that if this fails,
>> it can at least fall back to that?
> 
> You're correct. It will reject the flow.
> 
> Maybe we clamp the value to OVS_XLATE_MAX if it's unknown. That makes
> the skb drop reason less helpful, but no less helpful than today ;). At
> least we won't reject the flow.
> 
> We could alias OVS_XLATE_MAX to OVS_XLATE_UNKNOWN. I prefer an explicit
> value for OVS_XLATE_UNKNOWN, e.g. (u16)-1.

A wild idea:  How about we do not define actual reasons?  i.e. define a
subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where
'value' is whatever userspace gives as long as it is within a subsystem
range?

The point is: drop reasons are not part of the uAPI, but by defining drop
reasons for openvswitch we're making this subset of drop reasons part of
the uAPI.  And that seems a bit shady.  Users can't really rely on
actual values of drop reasons anyway, because the subsystem offset will
not be part of the uAPI.  And it doesn't matter if they need to get them
from the kernel binary or from the userspace OVS binary.

So, it might be cleaner to not define them in the first place.  Thoughts?

CC: kernel maintainers

Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Jakub Kicinski]

On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote:
> A wild idea:  How about we do not define actual reasons?  i.e. define a
> subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where
> 'value' is whatever userspace gives as long as it is within a subsystem
> range?

That already exists, right? Johannes added it in the last release for WiFi.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Ilya Maximets]

On 7/7/23 17:00, Jakub Kicinski wrote:
> On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote:
>> A wild idea:  How about we do not define actual reasons?  i.e. define a
>> subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where
>> 'value' is whatever userspace gives as long as it is within a subsystem
>> range?
> 
> That already exists, right? Johannes added it in the last release for WiFi.

I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
to that on a surface.  However, looking closer, any value that can be passed
into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
kind of defined in net/mac80211/drop.h, unless I'm missing something (very
possible, because I don't really know wifi code).

The difference, I guess, is that for openvswitch values will be provided by
the userpsace application via netlink interface.  It'll be just a number not
defined anywhere in the kernel.  Only the subsystem itself will be defined
in order to occupy the range.  Garbage in, same garbage out, from the kernel's
perspective.

Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Ilya Maximets]

On 7/7/23 17:29, Ilya Maximets wrote:
> On 7/7/23 17:00, Jakub Kicinski wrote:
>> On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote:
>>> A wild idea:  How about we do not define actual reasons?  i.e. define a
>>> subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where
>>> 'value' is whatever userspace gives as long as it is within a subsystem
>>> range?
>>
>> That already exists, right? Johannes added it in the last release for WiFi.
> 
> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
> to that on a surface.  However, looking closer, any value that can be passed
> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
> possible, because I don't really know wifi code).
> 
> The difference, I guess, is that for openvswitch values will be provided by
> the userpsace application via netlink interface.  It'll be just a number not
> defined anywhere in the kernel.  Only the subsystem itself will be defined
> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
> perspective.

To be clear, I think, not defining them in this particular case is better.
Definition of every reason that userspace can come up with will add extra
uAPI maintenance cost/issues with no practical benefits.  Values are not
going to be used for anything outside reporting a drop reason and subsystem
offset is not part of uAPI anyway.

> 
> Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Jakub Kicinski]

On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
> >> That already exists, right? Johannes added it in the last release for WiFi.  
> > 
> > I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
> > to that on a surface.  However, looking closer, any value that can be passed
> > into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
> > kind of defined in net/mac80211/drop.h, unless I'm missing something (very
> > possible, because I don't really know wifi code).
> > 
> > The difference, I guess, is that for openvswitch values will be provided by
> > the userpsace application via netlink interface.  It'll be just a number not
> > defined anywhere in the kernel.  Only the subsystem itself will be defined
> > in order to occupy the range.  Garbage in, same garbage out, from the kernel's
> > perspective.  
> 
> To be clear, I think, not defining them in this particular case is better.
> Definition of every reason that userspace can come up with will add extra
> uAPI maintenance cost/issues with no practical benefits.  Values are not
> going to be used for anything outside reporting a drop reason and subsystem
> offset is not part of uAPI anyway.

Ah, I see. No, please don't stuff user space defined values into 
the drop reason. The reasons are for debugging the kernel stack 
itself. IOW it'd be abuse not reuse.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Ilya Maximets]

On 7/8/23 00:06, Jakub Kicinski wrote:
> On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
>>>> That already exists, right? Johannes added it in the last release for WiFi.  
>>>
>>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
>>> to that on a surface.  However, looking closer, any value that can be passed
>>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
>>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
>>> possible, because I don't really know wifi code).
>>>
>>> The difference, I guess, is that for openvswitch values will be provided by
>>> the userpsace application via netlink interface.  It'll be just a number not
>>> defined anywhere in the kernel.  Only the subsystem itself will be defined
>>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
>>> perspective.  
>>
>> To be clear, I think, not defining them in this particular case is better.
>> Definition of every reason that userspace can come up with will add extra
>> uAPI maintenance cost/issues with no practical benefits.  Values are not
>> going to be used for anything outside reporting a drop reason and subsystem
>> offset is not part of uAPI anyway.
> 
> Ah, I see. No, please don't stuff user space defined values into 
> the drop reason. The reasons are for debugging the kernel stack 
> itself. IOW it'd be abuse not reuse.

Makes sense.  I wasn't sure that's a good solution from a kernel perspective
either.  It's better than defining all these reasons, IMO, but it's not good
enough to be considered acceptable, I agree.

How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
One for an explicit drop action with a zero argument and one for an explicit
drop with non-zero argument.

The exact reason for the error can be retrieved by other means, i.e by looking
at the datapath flow dump or OVS logs/traces.

This way we can give a user who is catching packet drop traces a signal that
there was something wrong with an OVS flow and they can look up exact details
from the userspace / flow dump.

The point being, most of the flows will have a zero as a drop action argument,
i.e. a regular explicit packet drop.  It will be hard to figure out which flow
exactly we're hitting without looking at the full flow dump.  And if the value
is non-zero, then it should be immediately obvious which flow is to blame from
the dump, as we should not have a lot of such flows.

This would still allow us to avoid a maintenance burden of defining every case,
which are fairly meaningless for the kernel itself, while having 99% of the
information we may need.

Jakub, do you think this will be acceptable?

Eric, Adrian, Aaron, do you see any problems with such implementation?

P.S. There is a plan to add more drop reasons for other places in openvswitch
     module to catch more regular types of drops like memory issues or upcall
     failures.  So, the drop reason subsystem can be extended later.
     The explicit drop action is a bit of an odd case here.

Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Jakub Kicinski]

On Mon, 10 Jul 2023 18:51:19 +0200 Ilya Maximets wrote:
> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
> either.  It's better than defining all these reasons, IMO, but it's not good
> enough to be considered acceptable, I agree.
> 
> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
> One for an explicit drop action with a zero argument and one for an explicit
> drop with non-zero argument.
> 
> The exact reason for the error can be retrieved by other means, i.e by looking
> at the datapath flow dump or OVS logs/traces.
> 
> This way we can give a user who is catching packet drop traces a signal that
> there was something wrong with an OVS flow and they can look up exact details
> from the userspace / flow dump.
> 
> The point being, most of the flows will have a zero as a drop action argument,
> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
> exactly we're hitting without looking at the full flow dump.  And if the value
> is non-zero, then it should be immediately obvious which flow is to blame from
> the dump, as we should not have a lot of such flows.
> 
> This would still allow us to avoid a maintenance burden of defining every case,
> which are fairly meaningless for the kernel itself, while having 99% of the
> information we may need.
> 
> Jakub, do you think this will be acceptable?

As far as I understand what you're proposing, yes :)

> Eric, Adrian, Aaron, do you see any problems with such implementation?
> 
> P.S. There is a plan to add more drop reasons for other places in openvswitch
>      module to catch more regular types of drops like memory issues or upcall
>      failures.  So, the drop reason subsystem can be extended later.
>      The explicit drop action is a bit of an odd case here.

If you have more than ~4 OvS specific reasons, I wonder if it still
makes sense to create a reason group/subsystem for OvS (a'la WiFi)?

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Ilya Maximets]

On 7/10/23 19:01, Jakub Kicinski wrote:
> On Mon, 10 Jul 2023 18:51:19 +0200 Ilya Maximets wrote:
>> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
>> either.  It's better than defining all these reasons, IMO, but it's not good
>> enough to be considered acceptable, I agree.
>>
>> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
>> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
>> One for an explicit drop action with a zero argument and one for an explicit
>> drop with non-zero argument.
>>
>> The exact reason for the error can be retrieved by other means, i.e by looking
>> at the datapath flow dump or OVS logs/traces.
>>
>> This way we can give a user who is catching packet drop traces a signal that
>> there was something wrong with an OVS flow and they can look up exact details
>> from the userspace / flow dump.
>>
>> The point being, most of the flows will have a zero as a drop action argument,
>> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
>> exactly we're hitting without looking at the full flow dump.  And if the value
>> is non-zero, then it should be immediately obvious which flow is to blame from
>> the dump, as we should not have a lot of such flows.
>>
>> This would still allow us to avoid a maintenance burden of defining every case,
>> which are fairly meaningless for the kernel itself, while having 99% of the
>> information we may need.
>>
>> Jakub, do you think this will be acceptable?
> 
> As far as I understand what you're proposing, yes :)

OK.  Just to spell it all out:

Userspace will install a flow with an OVS_FLOW_CMD_NEW:

  match:ip,tcp,... actions:something,something,drop(0)
  match:ip,udp,... actions:something,something,drop(42)

drop() here represents the OVS_ACTION_ATTR_DROP.

Then, in net/openvswitch/actions.c:do_execute_actions(), while executing
these actions:

  case OVS_ACTION_ATTR_DROP:
      kfree_skb_reason(skb, nla_get_u32(a) ? OVS_DROP_ACTION_WITH_ERROR
                                           : OVS_DROP_ACTION);

Users can enable traces and catch the OVS_DROP_ACTION_WITH_ERROR.
Later they can dump flows with OVS_FLOW_CMD_GET and see that the
error value was 42.

> 
>> Eric, Adrian, Aaron, do you see any problems with such implementation?
>>
>> P.S. There is a plan to add more drop reasons for other places in openvswitch
>>      module to catch more regular types of drops like memory issues or upcall
>>      failures.  So, the drop reason subsystem can be extended later.
>>      The explicit drop action is a bit of an odd case here.
> 
> If you have more than ~4 OvS specific reasons, I wonder if it still
> makes sense to create a reason group/subsystem for OvS (a'la WiFi)?

I believe, we will easily have more than 4 OVS-specific reasons.  A few
from the top of my head:
  - upcall failure (failed to send a packet to userspace)
  - reached the limit for deferred actions
  - reached the recursion limit

So, creation of a reason group/subsystem seems reasonable to me.

Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Jakub Kicinski]

On Mon, 10 Jul 2023 20:39:11 +0200 Ilya Maximets wrote:
> > As far as I understand what you're proposing, yes :)  
> 
> OK.  Just to spell it all out:
> 
> Userspace will install a flow with an OVS_FLOW_CMD_NEW:
> 
>   match:ip,tcp,... actions:something,something,drop(0)
>   match:ip,udp,... actions:something,something,drop(42)
> 
> drop() here represents the OVS_ACTION_ATTR_DROP.
> 
> Then, in net/openvswitch/actions.c:do_execute_actions(), while executing
> these actions:
> 
>   case OVS_ACTION_ATTR_DROP:
>       kfree_skb_reason(skb, nla_get_u32(a) ? OVS_DROP_ACTION_WITH_ERROR
>                                            : OVS_DROP_ACTION);
> 
> Users can enable traces and catch the OVS_DROP_ACTION_WITH_ERROR.
> Later they can dump flows with OVS_FLOW_CMD_GET and see that the
> error value was 42.

nod

> >> Eric, Adrian, Aaron, do you see any problems with such implementation?
> >>
> >> P.S. There is a plan to add more drop reasons for other places in openvswitch
> >>      module to catch more regular types of drops like memory issues or upcall
> >>      failures.  So, the drop reason subsystem can be extended later.
> >>      The explicit drop action is a bit of an odd case here.  
> > 
> > If you have more than ~4 OvS specific reasons, I wonder if it still
> > makes sense to create a reason group/subsystem for OvS (a'la WiFi)?  
> 
> I believe, we will easily have more than 4 OVS-specific reasons.  A few
> from the top of my head:
>   - upcall failure (failed to send a packet to userspace)
>   - reached the limit for deferred actions
>   - reached the recursion limit
> 
> So, creation of a reason group/subsystem seems reasonable to me.

SG.

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Eric Garver]

On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:
> On 7/8/23 00:06, Jakub Kicinski wrote:
> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
> >>>> That already exists, right? Johannes added it in the last release for WiFi.  
> >>>
> >>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
> >>> to that on a surface.  However, looking closer, any value that can be passed
> >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
> >>> possible, because I don't really know wifi code).
> >>>
> >>> The difference, I guess, is that for openvswitch values will be provided by
> >>> the userpsace application via netlink interface.  It'll be just a number not
> >>> defined anywhere in the kernel.  Only the subsystem itself will be defined
> >>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
> >>> perspective.  
> >>
> >> To be clear, I think, not defining them in this particular case is better.
> >> Definition of every reason that userspace can come up with will add extra
> >> uAPI maintenance cost/issues with no practical benefits.  Values are not
> >> going to be used for anything outside reporting a drop reason and subsystem
> >> offset is not part of uAPI anyway.
> > 
> > Ah, I see. No, please don't stuff user space defined values into 
> > the drop reason. The reasons are for debugging the kernel stack 
> > itself. IOW it'd be abuse not reuse.
> 
> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
> either.  It's better than defining all these reasons, IMO, but it's not good
> enough to be considered acceptable, I agree.
> 
> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
> One for an explicit drop action with a zero argument and one for an explicit
> drop with non-zero argument.
> 
> The exact reason for the error can be retrieved by other means, i.e by looking
> at the datapath flow dump or OVS logs/traces.
> 
> This way we can give a user who is catching packet drop traces a signal that
> there was something wrong with an OVS flow and they can look up exact details
> from the userspace / flow dump.
> 
> The point being, most of the flows will have a zero as a drop action argument,
> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
> exactly we're hitting without looking at the full flow dump.  And if the value
> is non-zero, then it should be immediately obvious which flow is to blame from
> the dump, as we should not have a lot of such flows.
> 
> This would still allow us to avoid a maintenance burden of defining every case,
> which are fairly meaningless for the kernel itself, while having 99% of the
> information we may need.
> 
> Jakub, do you think this will be acceptable?
> 
> Eric, Adrian, Aaron, do you see any problems with such implementation?

I see no problems. I'm content with this approach.

> P.S. There is a plan to add more drop reasons for other places in openvswitch
>      module to catch more regular types of drops like memory issues or upcall
>      failures.  So, the drop reason subsystem can be extended later.
>      The explicit drop action is a bit of an odd case here.
> 
> Best regards, Ilya Maximets.
> 

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Aaron Conole]

Eric Garver <eric@garver.life> writes:

> On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:
>> On 7/8/23 00:06, Jakub Kicinski wrote:
>> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
>> >>>> That already exists, right? Johannes added it in the last release for WiFi.  
>> >>>
>> >>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
>> >>> to that on a surface.  However, looking closer, any value that can be passed
>> >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
>> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
>> >>> possible, because I don't really know wifi code).
>> >>>
>> >>> The difference, I guess, is that for openvswitch values will be provided by
>> >>> the userpsace application via netlink interface.  It'll be just a number not
>> >>> defined anywhere in the kernel.  Only the subsystem itself will be defined
>> >>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
>> >>> perspective.  
>> >>
>> >> To be clear, I think, not defining them in this particular case is better.
>> >> Definition of every reason that userspace can come up with will add extra
>> >> uAPI maintenance cost/issues with no practical benefits.  Values are not
>> >> going to be used for anything outside reporting a drop reason and subsystem
>> >> offset is not part of uAPI anyway.
>> > 
>> > Ah, I see. No, please don't stuff user space defined values into 
>> > the drop reason. The reasons are for debugging the kernel stack 
>> > itself. IOW it'd be abuse not reuse.
>> 
>> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
>> either.  It's better than defining all these reasons, IMO, but it's not good
>> enough to be considered acceptable, I agree.
>> 
>> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
>> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
>> One for an explicit drop action with a zero argument and one for an explicit
>> drop with non-zero argument.
>> 
>> The exact reason for the error can be retrieved by other means, i.e by looking
>> at the datapath flow dump or OVS logs/traces.
>> 
>> This way we can give a user who is catching packet drop traces a signal that
>> there was something wrong with an OVS flow and they can look up exact details
>> from the userspace / flow dump.
>> 
>> The point being, most of the flows will have a zero as a drop action argument,
>> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
>> exactly we're hitting without looking at the full flow dump.  And if the value
>> is non-zero, then it should be immediately obvious which flow is to blame from
>> the dump, as we should not have a lot of such flows.
>> 
>> This would still allow us to avoid a maintenance burden of defining every case,
>> which are fairly meaningless for the kernel itself, while having 99% of the
>> information we may need.
>> 
>> Jakub, do you think this will be acceptable?
>> 
>> Eric, Adrian, Aaron, do you see any problems with such implementation?
>
> I see no problems. I'm content with this approach.

+1

>> P.S. There is a plan to add more drop reasons for other places in openvswitch
>>      module to catch more regular types of drops like memory issues or upcall
>>      failures.  So, the drop reason subsystem can be extended later.
>>      The explicit drop action is a bit of an odd case here.
>> 
>> Best regards, Ilya Maximets.
>> 

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Adrian Moreno]

On 7/11/23 22:46, Aaron Conole wrote:
> Eric Garver <eric@garver.life> writes:
> 
>> On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:
>>> On 7/8/23 00:06, Jakub Kicinski wrote:
>>>> On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
>>>>>>> That already exists, right? Johannes added it in the last release for WiFi.
>>>>>>
>>>>>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
>>>>>> to that on a surface.  However, looking closer, any value that can be passed
>>>>>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
>>>>>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
>>>>>> possible, because I don't really know wifi code).
>>>>>>
>>>>>> The difference, I guess, is that for openvswitch values will be provided by
>>>>>> the userpsace application via netlink interface.  It'll be just a number not
>>>>>> defined anywhere in the kernel.  Only the subsystem itself will be defined
>>>>>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
>>>>>> perspective.
>>>>>
>>>>> To be clear, I think, not defining them in this particular case is better.
>>>>> Definition of every reason that userspace can come up with will add extra
>>>>> uAPI maintenance cost/issues with no practical benefits.  Values are not
>>>>> going to be used for anything outside reporting a drop reason and subsystem
>>>>> offset is not part of uAPI anyway.
>>>>
>>>> Ah, I see. No, please don't stuff user space defined values into
>>>> the drop reason. The reasons are for debugging the kernel stack
>>>> itself. IOW it'd be abuse not reuse.
>>>
>>> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
>>> either.  It's better than defining all these reasons, IMO, but it's not good
>>> enough to be considered acceptable, I agree.
>>>
>>> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
>>> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
>>> One for an explicit drop action with a zero argument and one for an explicit
>>> drop with non-zero argument.
>>>
>>> The exact reason for the error can be retrieved by other means, i.e by looking
>>> at the datapath flow dump or OVS logs/traces.
>>>
>>> This way we can give a user who is catching packet drop traces a signal that
>>> there was something wrong with an OVS flow and they can look up exact details
>>> from the userspace / flow dump.
>>>
>>> The point being, most of the flows will have a zero as a drop action argument,
>>> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
>>> exactly we're hitting without looking at the full flow dump.  And if the value
>>> is non-zero, then it should be immediately obvious which flow is to blame from
>>> the dump, as we should not have a lot of such flows.
>>>
>>> This would still allow us to avoid a maintenance burden of defining every case,
>>> which are fairly meaningless for the kernel itself, while having 99% of the
>>> information we may need.
>>>
>>> Jakub, do you think this will be acceptable?
>>>
>>> Eric, Adrian, Aaron, do you see any problems with such implementation?
>>
>> I see no problems. I'm content with this approach.
> 
> +1

Sounds like a good plan. Thanks.

> 
>>> P.S. There is a plan to add more drop reasons for other places in openvswitch
>>>       module to catch more regular types of drops like memory issues or upcall
>>>       failures.  So, the drop reason subsystem can be extended later.
>>>       The explicit drop action is a bit of an odd case here.
>>>
>>> Best regards, Ilya Maximets.
>>>
>