From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BB3BC15AC1 for ; Tue, 8 Aug 2023 15:56:05 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2E2149194 for ; Tue, 8 Aug 2023 08:55:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691510103; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hBgZ6tSFYAow6bPJhWSQOOZxeHZezlmeBosKs9WsABk=; b=QTVgd48kfbOtKuJh61DiK5LJFeq28aOS+jzLpqTt/ugrbl9yKXIpun+BHCZynBVxEo0JKY CPNmTu/Su7Ys4TOJo/gqs3I0PXhRjd33qha7dzAwog28XSzHjkj1d98bxh3RBDrc0GvSAK F0jnvUNGDjuGro9h+f3em1yn7flP92Y= Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-562-_qC0xogfNkSrxtov-iCb2w-1; Tue, 08 Aug 2023 10:53:09 -0400 X-MC-Unique: _qC0xogfNkSrxtov-iCb2w-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E2E003801BE0; Tue, 8 Aug 2023 14:53:08 +0000 (UTC) Received: from RHTPC1VM0NT (unknown [10.22.8.251]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9AE19492C13; Tue, 8 Aug 2023 14:53:08 +0000 (UTC) From: Aaron Conole To: Adrian Moreno Cc: netdev@vger.kernel.org, Eric Garver , i.maximets@ovn.org, dev@openvswitch.org Subject: Re: [net-next v3 3/7] net: openvswitch: add explicit drop action References: <20230807164551.553365-1-amorenoz@redhat.com> <20230807164551.553365-4-amorenoz@redhat.com> Date: Tue, 08 Aug 2023 10:53:08 -0400 In-Reply-To: <20230807164551.553365-4-amorenoz@redhat.com> (Adrian Moreno's message of "Mon, 7 Aug 2023 18:45:44 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Adrian Moreno writes: > From: Eric Garver > > From: 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 or > zero to indicate no particular error happened (i.e: OVS intentionally > dropped the packet). > > Since the error codes coming from userspace mean nothing for the kernel, > we squash all of them into only two drop reasons: > - OVS_DROP_EXPLICIT_ACTION_ERROR to indicate a non-zero value was passed > - OVS_DROP_EXPLICIT_ACTION to indicate a zero value was passed (no > error) > > 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: 196611) > > reason: 196611 --> 0x30003 (OVS_DROP_EXPLICIT_ACTION) > > Signed-off-by: Eric Garver > Co-developed-by: Adrian Moreno > Signed-off-by: Adrian Moreno > --- > include/uapi/linux/openvswitch.h | 2 ++ > net/openvswitch/actions.c | 9 +++++++++ > net/openvswitch/drop.h | 2 ++ > net/openvswitch/flow_netlink.c | 10 +++++++++- > tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 3 +++ > 5 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > index e94870e77ee9..efc82c318fa2 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 error code. */ > > __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 9b66a3334aaa..285b1243b94f 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -1485,6 +1485,15 @@ 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: { > + enum ovs_drop_reason reason = nla_get_u32(a) > + ? OVS_DROP_EXPLICIT_ACTION_ERROR > + : OVS_DROP_EXPLICIT_ACTION; > + > + kfree_skb_reason(skb, reason); > + return 0; > + } > } > > if (unlikely(err)) { > diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h > index 3cd6489a5a2b..be51ff5039fb 100644 > --- a/net/openvswitch/drop.h > +++ b/net/openvswitch/drop.h > @@ -10,6 +10,8 @@ > #define OVS_DROP_REASONS(R) \ > R(OVS_DROP_FLOW) \ > R(OVS_DROP_ACTION_ERROR) \ > + R(OVS_DROP_EXPLICIT_ACTION) \ > + R(OVS_DROP_EXPLICIT_ACTION_ERROR) \ > /* deliberate comment for trailing \ */ > > enum ovs_drop_reason { > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index 41116361433d..88965e2068ac 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -38,6 +38,7 @@ > #include > #include > > +#include "drop.h" > #include "flow_netlink.h" > > struct ovs_len_tbl { > @@ -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,11 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > skip_copy = true; > break; > > + case OVS_ACTION_ATTR_DROP: > + if (!nla_is_last(a, rem)) > + 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 fbdac15e3134..5fee330050c2 100644 > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py > @@ -301,6 +301,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): > @@ -447,6 +448,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" Any reason that you don't include the int(self.get_attr(field[0])) here and add it only to 7/7? > elif field[1] == "flag": > if field[0] == "OVS_ACTION_ATTR_CT_CLEAR": > print_str += "ct_clear"