netdev - Re: [net-next v3 3/7] net: openvswitch: add explicit drop action

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9d3d0ed6-7d34-711b-85ea-89521fc53eca@redhat.com>
Date: Wed, 9 Aug 2023 09:03:50 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: Ilya Maximets <i.maximets@....org>, netdev@...r.kernel.org
Cc: Eric Garver <eric@...ver.life>, aconole@...hat.com, dev@...nvswitch.org
Subject: Re: [net-next v3 3/7] net: openvswitch: add explicit drop action



On 8/8/23 20:10, Ilya Maximets wrote:
> On 8/7/23 18:45, Adrian Moreno wrote:
>> From: Eric Garver <eric@...ver.life>
>>
>> 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: 196611)
>>
>> reason: 196611 --> 0x30003 (OVS_DROP_EXPLICIT_ACTION)
>>
>> Signed-off-by: Eric Garver <eric@...ver.life>
>> Co-developed-by: Adrian Moreno <amorenoz@...hat.com>
>> 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                       | 10 +++++++++-
>>   tools/testing/selftests/net/openvswitch/ovs-dpctl.py |  3 +++
>>   5 files changed, 25 insertions(+), 1 deletion(-)
> 
> <snip>
> 
>> 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)	\
> 
> These drop reasons are a bit unclear as well.  Especially since we
> have OVS_DROP_ACTION_ERROR and OVS_DROP_EXPLICIT_ACTION_ERROR that
> mean completely different things while having similar names.
> 
> Maybe remove the 'ACTION' part from these and add a word 'with'?
> E.g. OVS_DROP_EXPLICIT and OVS_DROP_EXPLICIT_WITH_ERROR.  I suppose,
> 'WITH' can also be shortened to 'W'.  It's fairly obvious that
> explicit drops are caused by the explicit drop action.
> 
> What do you think?

Agree: OVS_DROP_EXPLICIT{,_WITH_ERROR} are better names. Thanks!

> 
> Best regards, Ilya Maximets.
> 

-- 
Adrián Moreno