| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 26 Jul 2023 10:31:27 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: Aaron Conole <aconole@...hat.com>
Cc: netdev@...r.kernel.org, Eric Garver <eric@...ver.life>,
dev@...nvswitch.org, i.maximets@....org
Subject: Re: [PATCH net-next 2/7] net: openvswitch: add explicit drop action
On 7/24/23 16:47, Aaron Conole wrote:
> Adrian Moreno <amorenoz@...hat.com> writes:
>
>> From: Eric Garver <eric@...ver.life>
>>
>> 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: 196610)
>>
>> reason: 196610 --> 0x30002 (OVS_DROP_EXPLICIT_ACTION)
>>
>> Signed-off-by: Eric Garver <eric@...ver.life>
>> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
>> ---
>> include/uapi/linux/openvswitch.h | 2 ++
>> net/openvswitch/actions.c | 9 +++++++++
>> net/openvswitch/drop.h | 2 ++
>> net/openvswitch/flow_netlink.c | 8 +++++++-
>> tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 3 +++
>> 5 files changed, 23 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 af676dcac2b4..194379d58b62 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 cdd10629c6be..f9e9c1610f6b 100644
>> --- a/net/openvswitch/drop.h
>> +++ b/net/openvswitch/drop.h
>> @@ -9,6 +9,8 @@
>>
>> #define OVS_DROP_REASONS(R) \
>> R(OVS_DROP_FLOW) \
>> + 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..244280922a14 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -38,6 +38,7 @@
>> #include <net/tun_proto.h>
>> #include <net/erspan.h>
>>
>> +#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,9 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>> skip_copy = true;
>> break;
>>
>> + case OVS_ACTION_ATTR_DROP:
>> + break;
>> +
>
> We may want to have an explicit check in this area to warn about an
> action list like:
>
> output:1,drop(something),output:2
>
> I see the action handling code will correctly return when we encounter
> drop action, so it should stop the current skb context from being
> accessed further. But it may also be good to prevent extra actions from
> being attempted in the first place.
>
Sounds like a good idea! Thanks. I'll add it in the next version.
>> 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 12ba5265b88f..61c4d7b75261 100644
>> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> @@ -280,6 +280,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):
>> @@ -426,6 +427,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"
>
--
Adrián Moreno
Powered by blists - more mailing lists