[ovs-dev] [PATCH ovn v5 2/2] pinctrl: Handle arp/nd for other address families.

[Felix Huettner via dev]

Previously we could only generate ARP requests from IPv4 packets
and NS requests from IPv6 packets. This was the case because we rely on
information in the packet to generate the ARP/NS requests.

However in case of ARP/NS requests originating from the Logical_Router
pipeline for nexthop lookups we overwrite the affected fields
afterwards. This overwrite is done by the userdata openflow actions.
Because of this we actually do not rely on any information of the IPv4/6
packets in these cases.

Unfortunately we can not easily determine if we are actually later
overwriting the affected fields. The approach now is to use the fields
from the IP header if we have a matching IP version and default to some
values otherwise. In case we overwrite this data afterwards we are
generally good. If we do not overwrite this data because of some bug we
will send out invalid ARP/NS requests. They will hopefully be dropped by
the rest of the network.

The alternative would have been to introduce new arp/nd_ns actions where
we guarantee this overwrite. This would not suffer from the above
limitations, but would require a coordination on upgrades between all
ovn-controllers and northd.

Signed-off-by: Felix Huettner 
---
v4->v5: rebase
v4: newly added

 controller/pinctrl.c |  52 +++--
 lib/actions.c|   4 +-
 northd/northd.c  |   9 +-
 tests/ovn-northd.at  |   8 +-
 tests/ovn.at | 268 ++-
 5 files changed, 320 insertions(+), 21 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f2e382a44..4c520bd5e 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1574,9 +1574,11 @@ pinctrl_handle_arp(struct rconn *swconn, const struct 
flow *ip_flow,
const struct ofputil_packet_in *pin,
struct ofpbuf *userdata, const struct ofpbuf *continuation)
 {
-/* This action only works for IP packets, and the switch should only send
- * us IP packets this way, but check here just to be sure. */
-if (ip_flow->dl_type != htons(ETH_TYPE_IP)) {
+uint16_t dl_type = ntohs(ip_flow->dl_type);
+
+/* This action only works for IPv4 or IPv6 packets, and the switch should
+ * only send us IP packets this way, but check here just to be sure. */
+if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(&rl, "ARP action on non-IP packet (Ethertype %"PRIx16")",
  ntohs(ip_flow->dl_type));
@@ -1600,9 +1602,25 @@ pinctrl_handle_arp(struct rconn *swconn, const struct 
flow *ip_flow,
 struct arp_eth_header *arp = dp_packet_l3(&packet);
 arp->ar_op = htons(ARP_OP_REQUEST);
 arp->ar_sha = ip_flow->dl_src;
-put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
 arp->ar_tha = eth_addr_zero;
-put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);
+
+/* We might be here without actually currently handling an IPv4 packet.
+ * This can happen in the case where we route IPv6 packets over an IPv4
+ * link.
+ * In these cases we have no destination IPv4 address from the packet that
+ * we can reuse. But we receive the actual destination IPv4 address via
+ * userdata anyway, so what we set for now is irrelevant.
+ * This is just a hope since we do not parse the userdata. If we land here
+ * for whatever reason without being an IPv4 packet and without userdata we
+ * will send out a wrong packet.
+ */
+if (ip_flow->dl_type == htons(ETH_TYPE_IP)) {
+put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
+put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);
+} else {
+put_16aligned_be32(&arp->ar_spa, 0);
+put_16aligned_be32(&arp->ar_tpa, 0);
+}
 
 if (ip_flow->vlans[0].tci & htons(VLAN_CFI)) {
 eth_push_vlan(&packet, htons(ETH_TYPE_VLAN_8021Q),
@@ -6741,8 +6759,11 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct 
flow *ip_flow,
  struct ofpbuf *userdata,
  const struct ofpbuf *continuation)
 {
-/* This action only works for IPv6 packets. */
-if (get_dl_type(ip_flow) != htons(ETH_TYPE_IPV6)) {
+uint16_t dl_type = ntohs(ip_flow->dl_type);
+
+/* This action only works for IPv4 or IPv6 packets, and the switch should
+ * only send us IP packets this way, but check here just to be sure. */
+if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(&rl, "NS action on non-IPv6 packet");
 return;
@@ -6758,8 +6779,23 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct 
flow *ip_flow,
 dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
 
 in6_generate_lla(ip_flow->dl_src, &ipv6_src);
+
+/* We might be here without actually currently handling an IPv6 packet.
+ * This can happen in the case where we route IPv

[ovs-dev] [PATCH ovn v5 1/2] northd: Handle routing for other address families.

[Felix Huettner via dev]

In most cases IPv4 packets are routed only over other IPv4 networks and
IPv6 packets are routed only over IPv6 networks. However there is no
inherent reason for this limitation. Routing IPv4 packets over IPv6
networks just requires the router to contain a route for an IPv4 network
with an IPv6 nexthop.

This was previously prevented in OVN in ovn-nbctl and northd. By
removing these filters the forwarding will work if the mac addresses are
prepopulated.

If the mac addresses are not prepopulated we will attempt to resolve them using
the original address family of the packet and not the address family of the
nexthop. This will fail and we will not forward the packet.

This feature can for example be used by service providers to
interconnect multiple IPv4 networks of a customer without needing to
negotiate free IPv4 addresses by just using any IPv6 address.

Signed-off-by: Felix Huettner 
---
v4->v5: rebase
v3->v4:
  - additional tests
  - add additional regbit for nexthop address family
v2->v3: fix uninitialized variable
v1->v2:
  - move ipv4 info to parsed_route
  - add tests for lr-route-add
  - switch tests to use fmt_pkt
  - some minor test cleanups

 NEWS  |   4 +
 northd/northd.c   |  83 +++---
 tests/ovn-nbctl.at|  26 +-
 tests/ovn-northd.at   | 128 ++---
 tests/ovn.at  | 645 ++
 utilities/ovn-nbctl.c |  12 +-
 6 files changed, 818 insertions(+), 80 deletions(-)

diff --git a/NEWS b/NEWS
index 3bdc55172..14345c96b 100644
--- a/NEWS
+++ b/NEWS
@@ -31,6 +31,10 @@ Post v24.03.0
 has been renamed to "options:ic-route-denylist" in order to comply with
 inclusive language guidelines. The previous name is still recognized to
 aid with backwards compatibility.
+  - Allow Static Routes where the address families of ip_prefix and nexthop
+diverge (e.g. IPv4 packets over IPv6 links). This is currently limited to
+nexthops that have their mac addresses prepopulated (so
+dynamic_neigh_routers must be false).
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/northd/northd.c b/northd/northd.c
index b0d0c4747..e697973ce 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -156,6 +156,7 @@ static bool default_acl_drop;
 #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
 #define REGBIT_DHCP_RELAY_REQ_CHK "reg9[7]"
 #define REGBIT_DHCP_RELAY_RESP_CHK "reg9[8]"
+#define REGBIT_NEXTHOP_IS_IPV4"reg9[9]"
 
 /* Register to store the eth address associated to a router port for packets
  * received in S_ROUTER_IN_ADMISSION.
@@ -265,7 +266,8 @@ static bool default_acl_drop;
  * | |   LOOKUP_NEIGHBOR_RESULT/ |   | |
  * | |   SKIP_LOOKUP_NEIGHBOR/   |   | |
  * | |REGBIT_DHCP_RELAY_REQ_CHK/ |   | |
- * | |REGBIT_DHCP_RELAY_RESP_CHK}|   | |
+ * | |REGBIT_DHCP_RELAY_RESP_CHK |   | |
+ * | |REGBIT_NEXTHOP_IS_IPV4}|   | |
  * | |   |   | |
  * | | REG_ORIG_TP_DPORT_ROUTER  |   | |
  * | |   |   | |
@@ -10084,13 +10086,15 @@ build_routing_policy_flow(struct lflow_table *lflows, 
struct ovn_datapath *od,
   "outport = %s; "
   "flags.loopback = 1; "
   REG_ECMP_GROUP_ID" = 0; "
+  REGBIT_NEXTHOP_IS_IPV4" = %d; "
   "next;",
   is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
   nexthop,
   is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
   lrp_addr_s,
   out_port->lrp_networks.ea_s,
-  out_port->json_key);
+  out_port->json_key,
+  is_ipv4);
 
 } else if (!strcmp(rule->action, "drop")) {
 ds_put_cstr(&actions, debug_drop_action());
@@ -10185,13 +10189,15 @@ build_ecmp_routing_policy_flows(struct lflow_table 
*lflows,
   "eth.src = %s; "
   "outport = %s; "
   "flags.loopback = 1; "
+  REGBIT_NEXTHOP_IS_IPV4" = %d; "
   "next;",
   is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
   rule->nexthops[i],
   is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
   lrp_addr_s,
   out_port->lrp_networks.ea_s,
-  out_port->json_key);
+  out_port->json_key,
+  is_ipv4);
 
 ds_clear(&match);
 ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && "
@@ -10309,6 +10315,8 @@ struct parsed_route {
 const struct nbrec_logical_router_static_route *route;
 bool ecmp_symmetric_reply;
 bool is_discard_route;
+bool is_ipv4_prefix;
+bool is_ipv4_nexthop;
 };
 
 static uint32_t

Re: [ovs-dev] [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action

[Adrián Moreno]

On Tue, Jun 11, 2024 at 09:54:49AM GMT, Aaron Conole wrote:
> Adrián Moreno  writes:
>
> > On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote:
> >> Adrian Moreno  writes:
> >>
> >> > Add support for a new action: emit_sample.
> >> >
> >> > This action accepts a u32 group id and a variable-length cookie and uses
> >> > the psample multicast group to make the packet available for
> >> > observability.
> >> >
> >> > The maximum length of the user-defined cookie is set to 16, same as
> >> > tc_cookie, to discourage using cookies that will not be offloadable.
> >> >
> >> > Signed-off-by: Adrian Moreno 
> >> > ---
> >>
> >> I saw some of the nits Simon raised - I'll add one more below.
> >>
> >> I haven't gone through the series thoroughly enough to make a detailed
> >> review.
> >>
> >> >  Documentation/netlink/specs/ovs_flow.yaml | 17 
> >> >  include/uapi/linux/openvswitch.h  | 25 
> >> >  net/openvswitch/actions.c | 50 +++
> >> >  net/openvswitch/flow_netlink.c| 33 ++-
> >> >  4 files changed, 124 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
> >> > b/Documentation/netlink/specs/ovs_flow.yaml
> >> > index 4fdfc6b5cae9..a7ab5593a24f 100644
> >> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> >> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> >> > @@ -727,6 +727,12 @@ attribute-sets:
> >> >  name: dec-ttl
> >> >  type: nest
> >> >  nested-attributes: dec-ttl-attrs
> >> > +  -
> >> > +name: emit-sample
> >> > +type: nest
> >> > +nested-attributes: emit-sample-attrs
> >> > +doc: |
> >> > +  Sends a packet sample to psample for external observation.
> >> >-
> >> >  name: tunnel-key-attrs
> >> >  enum-name: ovs-tunnel-key-attr
> >> > @@ -938,6 +944,17 @@ attribute-sets:
> >> >-
> >> >  name: gbp
> >> >  type: u32
> >> > +  -
> >> > +name: emit-sample-attrs
> >> > +enum-name: ovs-emit-sample-attr
> >> > +name-prefix: ovs-emit-sample-attr-
> >> > +attributes:
> >> > +  -
> >> > +name: group
> >> > +type: u32
> >> > +  -
> >> > +name: cookie
> >> > +type: binary
> >> >
> >> >  operations:
> >> >name-prefix: ovs-flow-cmd-
> >> > diff --git a/include/uapi/linux/openvswitch.h 
> >> > b/include/uapi/linux/openvswitch.h
> >> > index efc82c318fa2..a0e9dde0584a 100644
> >> > --- a/include/uapi/linux/openvswitch.h
> >> > +++ b/include/uapi/linux/openvswitch.h
> >> > @@ -914,6 +914,30 @@ struct check_pkt_len_arg {
> >> >  };
> >> >  #endif
> >> >
> >> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> >> > +/**
> >> > + * enum ovs_emit_sample_attr - Attributes for 
> >> > %OVS_ACTION_ATTR_EMIT_SAMPLE
> >> > + * action.
> >> > + *
> >> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of 
> >> > the
> >> > + * sample.
> >> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
> >> > contains
> >> > + * user-defined metadata. The maximum length is 16 bytes.
> >> > + *
> >> > + * Sends the packet to the psample multicast group with the specified 
> >> > group and
> >> > + * cookie. It is possible to combine this action with the
> >> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being 
> >> > emitted.
> >> > + */
> >> > +enum ovs_emit_sample_attr {
> >> > +OVS_EMIT_SAMPLE_ATTR_UNPSEC,
> >> > +OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
> >> > +OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified 
> >> > cookie. */
> >> > +__OVS_EMIT_SAMPLE_ATTR_MAX
> >> > +};
> >> > +
> >> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> >> > +
> >> > +
> >> >  /**
> >> >   * enum ovs_action_attr - Action types.
> >> >   *
> >> > @@ -1004,6 +1028,7 @@ enum ovs_action_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 error code. */
> >> > +OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. 
> >> > */
> >> >
> >> >  __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 964225580824..3b4dba0ded59 100644
> >> > --- a/net/openvswitch/actions.c
> >> > +++ b/net/openvswitch/actions.c
> >> > @@ -24,6 +24,11 @@
> >> >  #include 
> >> >  #include 
> >> >  #include 
> >> > +
> >> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> >> > +#include 
> >> > +#endif
> >> > +
> >> >  #include 
> >> >
> >> >  #include "datapath.h"
> >> > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, 
> >> > struct sw_flow_key *key)
> >> >  return 0;
> >> >

Re: [ovs-dev] [PATCH ovn v4 1/2] northd: Handle routing for other address families.

[0-day Robot]

Bleep bloop.  Greetings Felix Huettner, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 northd: Handle routing for other address families.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v4 2/2] pinctrl: Handle arp/nd for other address families.

[Felix Huettner via dev]

Previously we could only generate ARP requests from IPv4 packets
and NS requests from IPv6 packets. This was the case because we rely on
information in the packet to generate the ARP/NS requests.

However in case of ARP/NS requests originating from the Logical_Router
pipeline for nexthop lookups we overwrite the affected fields
afterwards. This overwrite is done by the userdata openflow actions.
Because of this we actually do not rely on any information of the IPv4/6
packets in these cases.

Unfortunately we can not easily determine if we are actually later
overwriting the affected fields. The approach now is to use the fields
from the IP header if we have a matching IP version and default to some
values otherwise. In case we overwrite this data afterwards we are
generally good. If we do not overwrite this data because of some bug we
will send out invalid ARP/NS requests. They will hopefully be dropped by
the rest of the network.

The alternative would have been to introduce new arp/nd_ns actions where
we guarantee this overwrite. This would not suffer from the above
limitations, but would require a coordination on upgrades between all
ovn-controllers and northd.

Signed-off-by: Felix Huettner 
---
v4: newly added

 controller/pinctrl.c |  52 +++--
 lib/actions.c|   4 +-
 northd/northd.c  |   9 +-
 tests/ovn-northd.at  |   8 +-
 tests/ovn.at | 268 ++-
 5 files changed, 320 insertions(+), 21 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 6a2c3dc68..3358a0caf 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1573,9 +1573,11 @@ pinctrl_handle_arp(struct rconn *swconn, const struct 
flow *ip_flow,
const struct ofputil_packet_in *pin,
struct ofpbuf *userdata, const struct ofpbuf *continuation)
 {
-/* This action only works for IP packets, and the switch should only send
- * us IP packets this way, but check here just to be sure. */
-if (ip_flow->dl_type != htons(ETH_TYPE_IP)) {
+uint16_t dl_type = ntohs(ip_flow->dl_type);
+
+/* This action only works for IPv4 or IPv6 packets, and the switch should
+ * only send us IP packets this way, but check here just to be sure. */
+if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(&rl, "ARP action on non-IP packet (Ethertype %"PRIx16")",
  ntohs(ip_flow->dl_type));
@@ -1599,9 +1601,25 @@ pinctrl_handle_arp(struct rconn *swconn, const struct 
flow *ip_flow,
 struct arp_eth_header *arp = dp_packet_l3(&packet);
 arp->ar_op = htons(ARP_OP_REQUEST);
 arp->ar_sha = ip_flow->dl_src;
-put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
 arp->ar_tha = eth_addr_zero;
-put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);
+
+/* We might be here without actually currently handling an IPv4 packet.
+ * This can happen in the case where we route IPv6 packets over an IPv4
+ * link.
+ * In these cases we have no destination IPv4 address from the packet that
+ * we can reuse. But we receive the actual destination IPv4 address via
+ * userdata anyway, so what we set for now is irrelevant.
+ * This is just a hope since we do not parse the userdata. If we land here
+ * for whatever reason without being an IPv4 packet and without userdata we
+ * will send out a wrong packet.
+ */
+if (ip_flow->dl_type == htons(ETH_TYPE_IP)) {
+put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
+put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);
+} else {
+put_16aligned_be32(&arp->ar_spa, 0);
+put_16aligned_be32(&arp->ar_tpa, 0);
+}
 
 if (ip_flow->vlans[0].tci & htons(VLAN_CFI)) {
 eth_push_vlan(&packet, htons(ETH_TYPE_VLAN_8021Q),
@@ -6773,8 +6791,11 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct 
flow *ip_flow,
  struct ofpbuf *userdata,
  const struct ofpbuf *continuation)
 {
-/* This action only works for IPv6 packets. */
-if (get_dl_type(ip_flow) != htons(ETH_TYPE_IPV6)) {
+uint16_t dl_type = ntohs(ip_flow->dl_type);
+
+/* This action only works for IPv4 or IPv6 packets, and the switch should
+ * only send us IP packets this way, but check here just to be sure. */
+if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(&rl, "NS action on non-IPv6 packet");
 return;
@@ -6790,8 +6811,23 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct 
flow *ip_flow,
 dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
 
 in6_generate_lla(ip_flow->dl_src, &ipv6_src);
+
+/* We might be here without actually currently handling an IPv6 packet.
+ * This can happen in the case where we route IPv4 packets over 

[ovs-dev] [PATCH ovn v4 1/2] northd: Handle routing for other address families.

[Felix Huettner via dev]

In most cases IPv4 packets are routed only over other IPv4 networks and
IPv6 packets are routed only over IPv6 networks. However there is no
inherent reason for this limitation. Routing IPv4 packets over IPv6
networks just requires the router to contain a route for an IPv4 network
with an IPv6 nexthop.

This was previously prevented in OVN in ovn-nbctl and northd. By
removing these filters the forwarding will work if the mac addresses are
prepopulated.

If the mac addresses are not prepopulated we will attempt to resolve them using
the original address family of the packet and not the address family of the
nexthop. This will fail and we will not forward the packet.

This feature can for example be used by service providers to
interconnect multiple IPv4 networks of a customer without needing to
negotiate free IPv4 addresses by just using any IPv6 address.

Signed-off-by: Felix Huettner 
---
v3->v4:
  - additional tests
  - add additional regbit for nexthop address family
v2->v3: fix uninitialized variable
v1->v2:
  - move ipv4 info to parsed_route
  - add tests for lr-route-add
  - switch tests to use fmt_pkt
  - some minor test cleanups

 NEWS  |   4 +
 northd/northd.c   |  83 +++---
 tests/ovn-nbctl.at|  26 +-
 tests/ovn-northd.at   | 128 ++---
 tests/ovn.at  | 645 ++
 utilities/ovn-nbctl.c |  12 +-
 6 files changed, 818 insertions(+), 80 deletions(-)

diff --git a/NEWS b/NEWS
index 81c958f9a..7b53b8aa7 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ Post v24.03.0
 MAC addresses configured on the LSP with "unknown", are learnt via the
 OVN native FDB.
   - Add support for ovsdb-server `--config-file` option in ovn-ctl.
+  - Allow Static Routes where the address families of ip_prefix and nexthop
+diverge (e.g. IPv4 packets over IPv6 links). This is currently limited to
+nexthops that have their mac addresses prepopulated (so
+dynamic_neigh_routers must be false).
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/northd/northd.c b/northd/northd.c
index c8a5efa01..60fe2a3d3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -155,6 +155,7 @@ static bool default_acl_drop;
 #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
 #define REGBIT_DHCP_RELAY_REQ_CHK "reg9[7]"
 #define REGBIT_DHCP_RELAY_RESP_CHK "reg9[8]"
+#define REGBIT_NEXTHOP_IS_IPV4"reg9[9]"
 
 /* Register to store the eth address associated to a router port for packets
  * received in S_ROUTER_IN_ADMISSION.
@@ -264,7 +265,8 @@ static bool default_acl_drop;
  * | |   LOOKUP_NEIGHBOR_RESULT/ |   | |
  * | |   SKIP_LOOKUP_NEIGHBOR/   |   | |
  * | |REGBIT_DHCP_RELAY_REQ_CHK/ |   | |
- * | |REGBIT_DHCP_RELAY_RESP_CHK}|   | |
+ * | |REGBIT_DHCP_RELAY_RESP_CHK |   | |
+ * | |REGBIT_NEXTHOP_IS_IPV4}|   | |
  * | |   |   | |
  * | | REG_ORIG_TP_DPORT_ROUTER  |   | |
  * | |   |   | |
@@ -10100,13 +10102,15 @@ build_routing_policy_flow(struct lflow_table *lflows, 
struct ovn_datapath *od,
   "outport = %s; "
   "flags.loopback = 1; "
   REG_ECMP_GROUP_ID" = 0; "
+  REGBIT_NEXTHOP_IS_IPV4" = %d; "
   "next;",
   is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
   nexthop,
   is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
   lrp_addr_s,
   out_port->lrp_networks.ea_s,
-  out_port->json_key);
+  out_port->json_key,
+  is_ipv4);
 
 } else if (!strcmp(rule->action, "drop")) {
 ds_put_cstr(&actions, debug_drop_action());
@@ -10201,13 +10205,15 @@ build_ecmp_routing_policy_flows(struct lflow_table 
*lflows,
   "eth.src = %s; "
   "outport = %s; "
   "flags.loopback = 1; "
+  REGBIT_NEXTHOP_IS_IPV4" = %d; "
   "next;",
   is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
   rule->nexthops[i],
   is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
   lrp_addr_s,
   out_port->lrp_networks.ea_s,
-  out_port->json_key);
+  out_port->json_key,
+  is_ipv4);
 
 ds_clear(&match);
 ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && "
@@ -10325,6 +10331,8 @@ struct parsed_route {
 const struct nbrec_logical_router_static_route *route;
 bool ecmp_symmetric_reply;
 bool is_discard_route;
+bool is_ipv4_prefix;
+bool is_ipv4_nexthop;
 };
 
 static uint32_t
@@ -10350,6 +10358,8 @@ parsed_routes_add

Re: [ovs-dev] [PATCH net-next 2/2] selftests: openvswitch: set value to nla flags

[Adrián Moreno]

On Mon, Jun 03, 2024 at 03:02:46PM GMT, Aaron Conole wrote:
> Adrian Moreno  writes:
>
> > Netlink flags, although they don't have payload at the netlink level,
> > are represented as having a "True" value in pyroute2.
> >
> > Without it, trying to add a flow with a flag-type action (e.g: pop_vlan)
> > fails with the following traceback:
> >
> > Traceback (most recent call last):
> >   File "[...]/ovs-dpctl.py", line 2498, in 
> > sys.exit(main(sys.argv))
> >  ^^
> >   File "[...]/ovs-dpctl.py", line 2487, in main
> > ovsflow.add_flow(rep["dpifindex"], flow)
> >   File "[...]/ovs-dpctl.py", line 2136, in add_flow
> > reply = self.nlm_request(
> > ^
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 822, in nlm_request
> > return tuple(self._genlm_request(*argv, **kwarg))
> >  ^^^
> >   File "[...]/pyroute2/netlink/generic/__init__.py", line 126, in
> > nlm_request
> > return tuple(super().nlm_request(*argv, **kwarg))
> >^^
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1124, in nlm_request
> > self.put(msg, msg_type, msg_flags, msg_seq=msg_seq)
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 389, in put
> > self.sendto_gate(msg, addr)
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1056, in sendto_gate
> > msg.encode()
> >   File "[...]/pyroute2/netlink/__init__.py", line 1245, in encode
> > offset = self.encode_nlas(offset)
> >  
> >   File "[...]/pyroute2/netlink/__init__.py", line 1560, in encode_nlas
> > nla_instance.setvalue(cell[1])
> >   File "[...]/pyroute2/netlink/__init__.py", line 1265, in setvalue
> > nlv.setvalue(nla_tuple[1])
> >  ~^^^
> > IndexError: list index out of range
> >
> > Signed-off-by: Adrian Moreno 
> > ---
>
> Acked-by: Aaron Conole 
>
> I don't know which pyroute2 version I had used when I tested this
> previously, but even on my current system I get this error now.  Thanks
> for the fix.
>

Thanks Aaron. I'll resend as v2 with your ack as a stand-alone patch
since the other patch of this series will be fixed by your soon-to-come
series.

> >  tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py 
> > b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index b76907ac0092..a2395c3f37a1 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -537,7 +537,7 @@ class ovsactions(nla):
> >  for flat_act in parse_flat_map:
> >  if parse_starts_block(actstr, flat_act[0], False):
> >  actstr = actstr[len(flat_act[0]):]
> > -self["attrs"].append([flat_act[1]])
> > +self["attrs"].append([flat_act[1], True])
> >  actstr = actstr[strspn(actstr, ", ") :]
> >  parsed = True
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next 1/2] selftests: openvswitch: fix action formatting

[Aaron Conole]

Adrián Moreno  writes:

> On Mon, Jun 03, 2024 at 03:00:03PM GMT, Aaron Conole wrote:
>> Adrian Moreno  writes:
>>
>> > In the action formatting function ("dpstr"), the iteration is made over
>> > the nla_map, so if there are more than one attribute from the same type
>> > we only print the first one.
>> >
>> > Fix this by iterating over the actual attributes.
>> >
>> > Signed-off-by: Adrian Moreno 
>> > ---
>> >  .../selftests/net/openvswitch/ovs-dpctl.py| 48 +++
>> >  1 file changed, 27 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py 
>> > b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > index 1dd057afd3fb..b76907ac0092 100644
>> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > @@ -437,40 +437,46 @@ class ovsactions(nla):
>> >  def dpstr(self, more=False):
>> >  print_str = ""
>> >
>> > -for field in self.nla_map:
>> > -if field[1] == "none" or self.get_attr(field[0]) is None:
>> > +for attr_name, value in self["attrs"]:
>> > +attr_desc = next(filter(lambda x: x[0] == attr_name, 
>> > self.nla_map),
>> > + None)
>> > +if not attr_desc:
>> > +raise ValueError("Unknown attribute: %s" % attr)
>> > +
>> > +attr_type = attr_desc[1]
>> > +
>> > +if attr_type == "none":
>>
>> I agree, this is an issue.  BUT I think it might be better to just
>> filter by field type up front.  See:
>>
>> https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36#diff-3e72e7405c6bb4e9842bed5f63883ca930387086bb40d4034e92ed83a5decb4bR441
>>
>> That version I think ends up being much easier to follow.  If you want
>> to take it for your series, feel free.  If you disagree, maybe there's
>> something I'm not considering about it.
>>
>
> I agree. It's better to check field attribute names first. I found this
> during manual testing of the "emit_sample" series but I ended up not
> needing it for the automated one, so I'm OK waiting for your cleanup
> series.

I'll get stuff out this week for it.

> In fact, I also have some patches that try some rework of this part. In
> particular, I tried to unify all attributes under a common base class
> that would handle printing and parsing. That way, most cases would fall
> into "print_str += datum.dpstr(more)" and the "if/elif" block would
> shrink significantly.

That sounds very good.

>> NOTE that version is just a bunch of independent changes that are
>> squashed together.  I have a cleaner version.
>>
>> I can also bundle up the series I have so far and submit, but I didn't
>> want to do that until I got all the pmtu.sh support working.  Maybe it
>> makes sense to send it now though.  Simon, Jakub - wdyt?
>>
>> >  continue
>> >  if print_str != "":
>> >  print_str += ","
>> >
>> > -if field[1] == "uint32":
>> > -if field[0] == "OVS_ACTION_ATTR_OUTPUT":
>> > -print_str += "%d" % int(self.get_attr(field[0]))
>> > -elif field[0] == "OVS_ACTION_ATTR_RECIRC":
>> > -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(%d)" % int(self.get_attr(field[0]))
>> > -elif field[1] == "flag":
>> > -if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
>> > +if attr_type == "uint32":
>> > +if attr_name == "OVS_ACTION_ATTR_OUTPUT":
>> > +print_str += "%d" % int(value)
>> > +elif attr_name == "OVS_ACTION_ATTR_RECIRC":
>> > +print_str += "recirc(0x%x)" % int(value)
>> > +elif attr_name == "OVS_ACTION_ATTR_TRUNC":
>> > +print_str += "trunc(%d)" % int(value)
>> > +elif attr_name == "OVS_ACTION_ATTR_DROP":
>> > +print_str += "drop(%d)" % int(value)
>> > +elif attr_type == "flag":
>> > +if attr_name == "OVS_ACTION_ATTR_CT_CLEAR":
>> >  print_str += "ct_clear"
>> > -elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
>> > +elif attr_name == "OVS_ACTION_ATTR_POP_VLAN":
>> >  print_str += "pop_vlan"
>> > -elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
>> > +elif attr_name == "OVS_ACTION_ATTR_POP_ETH":
>> >  print_str += "pop_eth"
>> > -elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
>> > +elif attr_name == "OVS_ACTION_ATTR_POP_NSH":
>> >

Re: [ovs-dev] [PATCH v2 2/2] ipf: Handle common case of ipf defragmentation.

[Aaron Conole]

Ilya Maximets  writes:

> On 6/5/24 16:54, Aaron Conole wrote:
>> Mike Pattrick  writes:
>> 
>>> When conntrack is reassembling packet fragments, the same reassembly
>>> context can be shared across multiple threads handling different packets
>>> simultaneously. Once a full packet is assembled, it is added to a packet
>>> batch for processing, in the case where there are multiple different pmd
>>> threads accessing conntrack simultaneously, there is a race condition
>>> where the reassembled packet may be added to an arbitrary batch even if
>>> the current batch is available.
>>>
>>> When this happens, the packet may be handled incorrectly as it is
>>> inserted into a random openflow execution pipeline, instead of the
>>> pipeline for that packets flow.
>>>
>>> This change makes a best effort attempt to try to add the defragmented
>>> packet to the current batch. directly. This should succeed most of the
>>> time.
>>>
>>> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
>>> Reported-at: https://issues.redhat.com/browse/FDP-560
>>> Signed-off-by: Mike Pattrick 
>>> ---
>> 
>> The patch overall looks good to me.  I'm considering applying with the
>> following addition:
>> 
>>   diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>   index 6b293770dd..d9b9e0c23f 100755
>>   --- a/utilities/checkpatch.py
>>   +++ b/utilities/checkpatch.py
>>   @@ -63,7 +63,8 @@ def open_spell_check_dict():
>>  'dhcpv6', 'opts', 'metadata', 'geneve', 'mutex',
>>  'netdev', 'netdevs', 'subtable', 'virtio', 
>> 'qos',
>>  'policer', 'datapath', 'tunctl', 'attr', 
>> 'ethernet',
>>   -  'ether', 'defrag', 'defragment', 'loopback', 
>> 'sflow',
>>   +  'ether', 'defrag', 'defragment', 'defragmented',
>>   +  'loopback', 'sflow',
>>  'acl', 'initializer', 'recirc', 'xlated', 
>> 'unclosed',
>>  'netlink', 'msec', 'usec', 'nsec', 'ms', 'us', 
>> 'ns',
>>  'kilobits', 'kbps', 'kilobytes', 'megabytes', 
>> 'mbps',
>> 
>> 
>> unless anyone objects.  This is to squelch:
>> 
>> == Checking 16f6885353c2 ("ipf: Handle common case of ipf defragmentation.") 
>> ==
>> WARNING: Possible misspelled word: "defragmented"
>> Did you mean:  ['defragment ed', 'defragment-ed', 'defragment']
>> Lines checked: 129, Warnings: 1, Errors: 0
>
> It doesn't affect CI today, so can be a separate patch, I think.  We
> have a few more
> words like this in relatively recent commits, like 'poller' or
> 'autovalidator', these
> can be bundled in that separate commit as well.
>
> Though updating the dictionary along with the patch that is using the
> word sounds OK
> to me as well.

That makes sense to me.

I've been thinking of adding a spell-check test to the robot.  Rather
than the existing apply check doing the spell checking.  The spell
checker would only ever generate a warning.  WDYT?

> Best regards, Ilya Maximets.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action

[Aaron Conole]

Adrián Moreno  writes:

> On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote:
>> Adrian Moreno  writes:
>>
>> > Add support for a new action: emit_sample.
>> >
>> > This action accepts a u32 group id and a variable-length cookie and uses
>> > the psample multicast group to make the packet available for
>> > observability.
>> >
>> > The maximum length of the user-defined cookie is set to 16, same as
>> > tc_cookie, to discourage using cookies that will not be offloadable.
>> >
>> > Signed-off-by: Adrian Moreno 
>> > ---
>>
>> I saw some of the nits Simon raised - I'll add one more below.
>>
>> I haven't gone through the series thoroughly enough to make a detailed
>> review.
>>
>> >  Documentation/netlink/specs/ovs_flow.yaml | 17 
>> >  include/uapi/linux/openvswitch.h  | 25 
>> >  net/openvswitch/actions.c | 50 +++
>> >  net/openvswitch/flow_netlink.c| 33 ++-
>> >  4 files changed, 124 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
>> > b/Documentation/netlink/specs/ovs_flow.yaml
>> > index 4fdfc6b5cae9..a7ab5593a24f 100644
>> > --- a/Documentation/netlink/specs/ovs_flow.yaml
>> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
>> > @@ -727,6 +727,12 @@ attribute-sets:
>> >  name: dec-ttl
>> >  type: nest
>> >  nested-attributes: dec-ttl-attrs
>> > +  -
>> > +name: emit-sample
>> > +type: nest
>> > +nested-attributes: emit-sample-attrs
>> > +doc: |
>> > +  Sends a packet sample to psample for external observation.
>> >-
>> >  name: tunnel-key-attrs
>> >  enum-name: ovs-tunnel-key-attr
>> > @@ -938,6 +944,17 @@ attribute-sets:
>> >-
>> >  name: gbp
>> >  type: u32
>> > +  -
>> > +name: emit-sample-attrs
>> > +enum-name: ovs-emit-sample-attr
>> > +name-prefix: ovs-emit-sample-attr-
>> > +attributes:
>> > +  -
>> > +name: group
>> > +type: u32
>> > +  -
>> > +name: cookie
>> > +type: binary
>> >
>> >  operations:
>> >name-prefix: ovs-flow-cmd-
>> > diff --git a/include/uapi/linux/openvswitch.h 
>> > b/include/uapi/linux/openvswitch.h
>> > index efc82c318fa2..a0e9dde0584a 100644
>> > --- a/include/uapi/linux/openvswitch.h
>> > +++ b/include/uapi/linux/openvswitch.h
>> > @@ -914,6 +914,30 @@ struct check_pkt_len_arg {
>> >  };
>> >  #endif
>> >
>> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>> > +/**
>> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>> > + * action.
>> > + *
>> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of 
>> > the
>> > + * sample.
>> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
>> > contains
>> > + * user-defined metadata. The maximum length is 16 bytes.
>> > + *
>> > + * Sends the packet to the psample multicast group with the specified 
>> > group and
>> > + * cookie. It is possible to combine this action with the
>> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being 
>> > emitted.
>> > + */
>> > +enum ovs_emit_sample_attr {
>> > +  OVS_EMIT_SAMPLE_ATTR_UNPSEC,
>> > +  OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
>> > +  OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
>> > +  __OVS_EMIT_SAMPLE_ATTR_MAX
>> > +};
>> > +
>> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>> > +
>> > +
>> >  /**
>> >   * enum ovs_action_attr - Action types.
>> >   *
>> > @@ -1004,6 +1028,7 @@ enum ovs_action_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 error code. */
>> > +  OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>> >
>> >__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 964225580824..3b4dba0ded59 100644
>> > --- a/net/openvswitch/actions.c
>> > +++ b/net/openvswitch/actions.c
>> > @@ -24,6 +24,11 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +
>> > +#if IS_ENABLED(CONFIG_PSAMPLE)
>> > +#include 
>> > +#endif
>> > +
>> >  #include 
>> >
>> >  #include "datapath.h"
>> > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, 
>> > struct sw_flow_key *key)
>> >return 0;
>> >  }
>> >
>> > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb,
>> > + const struct sw_flow_key *key,
>> > + const struct nlattr *attr)
>> > +{
>> > +#if IS_ENABLED(CONFIG_PSAMPLE)
>> > +  struct psample_group psample_group = {};
>> > +  struct psample_metadata md = {};
>> > +  struct vport *input_vport;
>> > +  const struct nlattr *a;
>> > +  int

[ovs-dev] [BUG][meter] ovs crash when add meter openflow

[Simon Jones]

Hi all,

I'm using ovs-dpdk with this patch:
```
commit 4c226944f7c55c9d6e7c85f7c33c5ce11c35ce54
Author: Jianbo Liu 
Date:   Fri Jul 8 03:06:26 2022 +

netdev-offload-tc: Implement meter offload API for tc
```
This patch is for offload flow meter by tc.

Now I found a bug: ovs crash when add meter openflow.

1. How to produce:
(NOTICE: This bug is not always reproducible.)
```
Add these commands:

ovs-ofctl -O OpenFlow13 add-meter br-int
meter=1,kbps,band=type=drop,rate=1000
ovs-ofctl -O OpenFlow13 add-flow br-int in_port=\"pf0vf0\",ip,nw_src=
16.0.0.0/24,nw_dst=48.0.0.0/24,nw_proto=17,actions=\"meter:1,output:p0\"
ovs-ofctl -O OpenFlow13 add-flow br-int
in_port=\"pf0vf0\",udp6,ipv6_src=2001:4860:0:2001::/64,ipv6_dst=2001:0:4137:9350:8000:f12a:b9c8:2815,actions=\"meter:1,output:p0\"

Then ovs crash, this is core file call trace:

(gdb) bt
#0  id_pool_alloc_id (pool=0x0, id_=id_@entry=0x7fff180c841c) at
lib/id-pool.c:112
#1  0x0055f0a0 in meter_alloc_police_index
(police_index=0x7fff180c841c) at lib/netdev-offload-tc.c:2530
#2  meter_tc_set_policer (meter_id=..., config=0x7fff180c8538) at
lib/netdev-offload-tc.c:2567
#3  0x004af207 in meter_offload_set (meter_id=meter_id@entry=...,
config=config@entry=0x7fff180c8538) at lib/netdev-offload.c:207
#4  0x00474c2b in dpif_netdev_meter_set (dpif=,
meter_id=..., config=0x7fff180c8538) at lib/dpif-netdev.c:9137
#5  0x0048e3fd in dpif_meter_set (dpif=0x28cfd10, meter_id=...,
config=) at lib/dpif.c:1973
#6  0x0042daf8 in meter_set (ofproto_=0x28cc670,
meter_id=0x7fff180c8590, config=) at
ofproto/ofproto-dpif.c:6751
#7  0x00423e91 in handle_add_meter (mm=0x7fff180c8530,
ofproto=0x28cc670) at ofproto/ofproto.c:6905
#8  handle_meter_mod (ofconn=0x29073d0, oh=0x291bd40) at
ofproto/ofproto.c:7013
#9  0x00426d21 in handle_single_part_openflow
(type=OFPTYPE_METER_MOD, oh=0x291bd40, ofconn=0x29073d0) at
ofproto/ofproto.c:8668
#10 handle_openflow (ofconn=0x29073d0, msgs=0x7fff180c88c0) at
ofproto/ofproto.c:8843
#11 0x0045b3b4 in ofconn_run (handle_openflow=0x4267b0
, ofconn=0x29073d0) at ofproto/connmgr.c:1329
#12 connmgr_run (mgr=0x272a200, handle_openflow=handle_openflow@entry=0x4267b0
) at ofproto/connmgr.c:356
#13 0x004209be in ofproto_run (p=0x28cc670) at
ofproto/ofproto.c:1964
#14 0x0040da44 in bridge_run__ () at vswitchd/bridge.c:3251
#15 0x004138ec in bridge_run () at vswitchd/bridge.c:3310
#16 0x0040955d in main (argc=, argv=)
at vswitchd/ovs-vswitchd.c:129

```

2. Check code:
```
Notice
#0  id_pool_alloc_id (pool=0x0, id_=id_@entry=0x7fff180c841c) at
lib/id-pool.c:112
the @pool param is NULL.

This is happened ONLY when @netdev_tc_init_flow_api is NOT called.
But in code, ONLY after @netdev_tc_init_flow_api is called, the
@handle_meter_mod could works on netdev with system type.
```

3. My question:
- How this BUG happen?
- Is there patch to solve this BUG?



Simon Jones
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action

[Adrián Moreno]

On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote:
> Adrian Moreno  writes:
>
> > Add support for a new action: emit_sample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
>
> I saw some of the nits Simon raised - I'll add one more below.
>
> I haven't gone through the series thoroughly enough to make a detailed
> review.
>
> >  Documentation/netlink/specs/ovs_flow.yaml | 17 
> >  include/uapi/linux/openvswitch.h  | 25 
> >  net/openvswitch/actions.c | 50 +++
> >  net/openvswitch/flow_netlink.c| 33 ++-
> >  4 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..a7ab5593a24f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> >  name: dec-ttl
> >  type: nest
> >  nested-attributes: dec-ttl-attrs
> > +  -
> > +name: emit-sample
> > +type: nest
> > +nested-attributes: emit-sample-attrs
> > +doc: |
> > +  Sends a packet sample to psample for external observation.
> >-
> >  name: tunnel-key-attrs
> >  enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >-
> >  name: gbp
> >  type: u32
> > +  -
> > +name: emit-sample-attrs
> > +enum-name: ovs-emit-sample-attr
> > +name-prefix: ovs-emit-sample-attr-
> > +attributes:
> > +  -
> > +name: group
> > +type: u32
> > +  -
> > +name: cookie
> > +type: binary
> >
> >  operations:
> >name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h 
> > b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..a0e9dde0584a 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,30 @@ struct check_pkt_len_arg {
> >  };
> >  #endif
> >
> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
> > contains
> > + * user-defined metadata. The maximum length is 16 bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified 
> > group and
> > + * cookie. It is possible to combine this action with the
> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being 
> > emitted.
> > + */
> > +enum ovs_emit_sample_attr {
> > +   OVS_EMIT_SAMPLE_ATTR_UNPSEC,
> > +   OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
> > +   OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> > +   __OVS_EMIT_SAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> > +
> > +
> >  /**
> >   * enum ovs_action_attr - Action types.
> >   *
> > @@ -1004,6 +1028,7 @@ enum ovs_action_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 error code. */
> > +   OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
> >
> > __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 964225580824..3b4dba0ded59 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -24,6 +24,11 @@
> >  #include 
> >  #include 
> >  #include 
> > +
> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > +#include 
> > +#endif
> > +
> >  #include 
> >
> >  #include "datapath.h"
> > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, 
> > struct sw_flow_key *key)
> > return 0;
> >  }
> >
> > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb,
> > +  const struct sw_flow_key *key,
> > +  const struct nlattr *attr)
> > +{
> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > +   struct psample_group psample_group = {};
> > +   struct psample_metadata md = {};
> > +   struct vport *input_vport;
> > +   const struct nlattr *a;
> > +   int rem;
> > +
> > +   for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
> > +a = nla_next(a, &rem)) {
> > +   switch (nla_type(a)) {
>

Re: [ovs-dev] [PATCH ovn v1] controller: Fix issue with ct_commit encode.

[Martin Kalcok]

> On 11 Jun 2024, at 00:07, Dumitru Ceara  wrote:
> 
> On 6/10/24 18:05, martin.kal...@canonical.com 
>  wrote:
>> On Mon, 2024-06-10 at 12:54 +, Naveen Yerramneni wrote:
>>> Action length is getting set incorrectly during ct_commit encode
>>> due to which ct action is getting skipped  during phsycial flows
>>> creation. This issue is noticed only if ct_commit is prefixed
>>> with other actions.
>>> 
>>> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; };
>>> without fix: encodes as set_field:0x2/0x2-
 xreg4
>>> with fix: encodes as set_field:0x2/0x2-
 xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1-
 ct_mark))
>>> 
>>> Signed-off-by: Naveen Yerramneni 
>>> ---
> 
> Thanks Naveen, for the catch!
> 
>>> v1:
> 
> Well, this should actually be v2 but that's a technicality.
> 
>>>   - Addressed review comments from Ales.
>>> ---
>>>  lib/actions.c | 3 +++
>>>  tests/ovn.at  | 3 +++
>>>  2 files changed, 6 insertions(+)
>>> 
>>> diff --git a/lib/actions.c b/lib/actions.c
>>> index 361d55009..794d2e916 100644
>>> --- a/lib/actions.c
>>> +++ b/lib/actions.c
>>> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>>>  const struct ovnact_encode_params *ep
>>> OVS_UNUSED,
>>>  struct ofpbuf *ofpacts)
>>>  {
>>> +size_t ct_offset = ofpacts->size;
>>> +ofpbuf_pull(ofpacts, ct_offset);
> 
> Nit: missing new line (but I can fix that up at apply time).
> 
>>>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>>>  ct->flags = NX_CT_F_COMMIT;
>>>  ct->recirc_table = NX_CT_RECIRC_NONE;
>>> @@ -791,6 +793,7 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>>>  ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>>>  ct = ofpacts->header;
>>>  ofpact_finish(ofpacts, &ct->ofpact);
>>> +ofpbuf_push_uninit(ofpacts, ct_offset);
>> 
>> Just a thought here. I was just wondering if this wouldn't be a good
>> opportunity to also replace the "manual" way of finishing the action
>> encoding (as @amusil referred to it in this review [0]) with the more
>> explicit approach (example included in [0]).
>> 
> 
> I agree that we should do that but we should do it in multiple
> functions, e.g.:
> 
> encode_CT_NEXT()
> encode_ct_nat()
> encode_ct_lb()
> 
> I think it's fine to do that in a follow up patch.  Martin, do you have
> time to post one?  Otherwise I'll put it on my list of things to do in
> the future.

Sure, I’ll find some time to make the proposal this week.

> 
> Regarding the current bug fix I'm planning to apply and backport this
> patch, pending ovsrobot re-run results.
> 
> Recheck-request: github-robot
> 
> Regards,
> Dumitru
> 
>> [0]
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html
>> 
>>>  }
>>>  
>>>  static void
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index dc6aafd53..d4f62f487 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -1315,6 +1315,9 @@ ct_commit {
>>> ct_label=0x181716151413121110090807060504030201; };
>>>  141-bit constant is not compatible with 128-bit field ct_label.
>>>  ct_commit { ip4.dst = 192.168.0.1; };
>>>  Field ip4.dst is not modifiable.
>>> +reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; };
>>> +encodes as set_field:0x2/0x2-
 xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1-
 ct_mark))
>>> +has prereqs ip
>>>  
>>>  # Legact ct_commit_v1 action.
>>>  ct_commit();
>> 
>> Martin.
>> ___
>> dev mailing list
>> d...@openvswitch.org 
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Martin.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev