Re: [ovs-dev] [PATCH net-next 4/7] net: openvswitch: add misc error drop reasons

[Adrian Moreno]

On 7/24/23 17:23, Aaron Conole wrote:

Adrian Moreno <amore...@redhat.com> writes:

Use drop reasons from include/net/dropreason-core.h when a reasonable
candidate exists.

Signed-off-by: Adrian Moreno <amore...@redhat.com>
---
  net/openvswitch/actions.c   | 17 ++++++++++-------
  net/openvswitch/conntrack.c |  3 ++-
  net/openvswitch/drop.h      |  6 ++++++
  3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 9279bb186e9f..42fa1e7eb912 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

Did you consider putting in a drop reason when one of the actions fails
setting err?  For example, if dec_ttl fails with some error other than
EHOSTUNREACH, it will drop into the kfree_skb() case... maybe we should
have an action_failed drop reason that can be passed there.

Another good idea! Thanks Aaron.

@@ -782,7 +782,7 @@ static int ovs_vport_output(struct net *net, struct sock 
*sk,
        struct vport *vport = data->vport;

if (skb_cow_head(skb, data->l2_len) < 0) {

-               kfree_skb(skb);
+               kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
                return -ENOMEM;
        }

@@ -853,6 +853,7 @@ static void ovs_fragment(struct net *net, struct vport *vport,

                         struct sk_buff *skb, u16 mru,
                         struct sw_flow_key *key)
  {
+       enum ovs_drop_reason reason;
        u16 orig_network_offset = 0;

if (eth_p_mpls(skb->protocol)) {

@@ -862,6 +863,7 @@ static void ovs_fragment(struct net *net, struct vport 
*vport,

if (skb_network_offset(skb) > MAX_L2_LEN) {

                OVS_NLERR(1, "L2 header too long to fragment");
+               reason = OVS_DROP_FRAG_L2_TOO_LONG;
                goto err;
        }

@@ -902,12 +904,13 @@ static void ovs_fragment(struct net *net, struct vport *vport,

                WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
                          ovs_vport_name(vport), ntohs(key->eth.type), mru,
                          vport->dev->mtu);
+               reason = OVS_DROP_FRAG_INVALID_PROTO;
                goto err;
        }

return;

  err:
-       kfree_skb(skb);
+       kfree_skb_reason(skb, reason);
  }

static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

@@ -934,10 +937,10 @@ static void do_output(struct datapath *dp, struct sk_buff 
*skb, int out_port,

ovs_fragment(net, vport, skb, mru, key);

                } else {
-                       kfree_skb(skb);
+                       kfree_skb_reason(skb, SKB_DROP_REASON_PKT_TOO_BIG);
                }
        } else {
-               kfree_skb(skb);
+               kfree_skb_reason(skb, SKB_DROP_REASON_DEV_READY);
        }
  }

@@ -1011,7 +1014,7 @@ static int dec_ttl_exception_handler(struct datapath *dp, struct sk_buff *skb,

                return clone_execute(dp, skb, key, 0, nla_data(actions),
                                     nla_len(actions), true, false);

- consume_skb(skb);

+       kfree_skb_reason(skb, OVS_DROP_IP_TTL);
        return 0;
  }

@@ -1564,7 +1567,7 @@ static int clone_execute(struct datapath *dp, struct sk_buff *skb,

                /* Out of per CPU action FIFO space. Drop the 'skb' and
                 * log an error.
                 */
-               kfree_skb(skb);
+               kfree_skb_reason(skb, OVS_DROP_DEFERRED_LIMIT);

if (net_ratelimit()) {

                        if (actions) { /* Sample action */
@@ -1616,7 +1619,7 @@ int ovs_execute_actions(struct datapath *dp, struct 
sk_buff *skb,
        if (unlikely(level > OVS_RECURSION_LIMIT)) {
                net_crit_ratelimited("ovs: recursion limit reached on datapath %s, 
probable configuration error\n",
                                     ovs_dp_name(dp));
-               kfree_skb(skb);
+               kfree_skb_reason(skb, OVS_DROP_RECURSION_LIMIT);
                err = -ENETDOWN;
                goto out;
        }
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index fa955e892210..b03ebd4a8fae 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -29,6 +29,7 @@
  #include <net/netfilter/nf_conntrack_act_ct.h>

#include "datapath.h"

+#include "drop.h"
  #include "conntrack.h"
  #include "flow.h"
  #include "flow_netlink.h"
@@ -1035,7 +1036,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,

skb_push_rcsum(skb, nh_ofs);

        if (err)
-               kfree_skb(skb);
+               kfree_skb_reason(skb, OVS_DROP_CONNTRACK);
        return err;
  }

diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h

index 2440c836727f..744b8d1b93a3 100644
--- a/net/openvswitch/drop.h
+++ b/net/openvswitch/drop.h
@@ -12,6 +12,12 @@
        R(OVS_DROP_EXPLICIT_ACTION)             \
        R(OVS_DROP_EXPLICIT_ACTION_ERROR)       \
        R(OVS_DROP_METER)                       \
+       R(OVS_DROP_RECURSION_LIMIT)             \
+       R(OVS_DROP_DEFERRED_LIMIT)              \
+       R(OVS_DROP_FRAG_L2_TOO_LONG)            \
+       R(OVS_DROP_FRAG_INVALID_PROTO)          \
+       R(OVS_DROP_CONNTRACK)                   \
+       R(OVS_DROP_IP_TTL)                      \
        /* deliberate comment for trailing \ */

enum ovs_drop_reason {

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