| Message ID | 20230722094238.2520044-5-amorenoz@redhat.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | openvswitch: add drop reasons | expand |
Adrian Moreno <amorenoz@redhat.com> writes: > Use drop reasons from include/net/dropreason-core.h when a reasonable > candidate exists. > > Signed-off-by: Adrian Moreno <amorenoz@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. > @@ -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 {
On 7/24/23 17:23, Aaron Conole wrote: > Adrian Moreno <amorenoz@redhat.com> writes: > >> Use drop reasons from include/net/dropreason-core.h when a reasonable >> candidate exists. >> >> Signed-off-by: Adrian Moreno <amorenoz@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 { >
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 @@ -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 {
Use drop reasons from include/net/dropreason-core.h when a reasonable candidate exists. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- net/openvswitch/actions.c | 17 ++++++++++------- net/openvswitch/conntrack.c | 3 ++- net/openvswitch/drop.h | 6 ++++++ 3 files changed, 18 insertions(+), 8 deletions(-)