netdev - Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add 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]

Date: Fri, 30 Jun 2023 15:25:30 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Eric Garver <eric@...ver.life>, netdev@...r.kernel.org,
	dev@...nvswitch.org, Pravin B Shelar <pshelar@....org>,
	Ilya Maximets <i.maximets@....org>
Subject: Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

On Fri, Jun 30, 2023 at 08:29:58AM -0400, Eric Garver wrote:
> On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> > On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > > 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. We
> > > can then use perf tracing to match on the drop reason.
> > > 
> > > 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_XLATE_RECURSION_TOO_DEEP)
> > > 
> > > Signed-off-by: Eric Garver <eric@...ver.life>
> > 
> > ...
> > 
> > > --- a/net/openvswitch/actions.c
> > > +++ b/net/openvswitch/actions.c
> > > @@ -32,6 +32,7 @@
> > >  #include "vport.h"
> > >  #include "flow_netlink.h"
> > >  #include "openvswitch_trace.h"
> > > +#include "drop.h"
> > >  
> > >  struct deferred_action {
> > >  	struct sk_buff *skb;
> > > @@ -1477,6 +1478,18 @@ 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:
> > > +			u32 reason = nla_get_u32(a);
> > > +
> > > +			reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > > +					SKB_DROP_REASON_SUBSYS_SHIFT;
> > > +
> > > +			if (reason == OVS_XLATE_OK)
> > > +				break;
> > > +
> > > +			kfree_skb_reason(skb, reason);
> > > +			return 0;
> > >  		}
> > 
> > Hi Eric,
> > 
> > thanks for your patches. This is an interesting new feature.
> > 
> > unfortunately clang-16 doesn't seem to like this very much.
> > I think that it is due to the declaration of reason not
> > being enclosed in a block - { }.
> > 
> >   net/openvswitch/actions.c:1483:4: error: expected expression
> >                           u32 reason = nla_get_u32(a);
> >                           ^
> >   net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
> >                           reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> >                           ^
> >   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
> >                           if (reason == OVS_XLATE_OK)
> >                               ^
> >   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 'reason'
> >                           kfree_skb_reason(skb, reason);
> >                                                 ^
> >   4 errors generated.
> > 
> > 
> > net-next is currently closed.
> > So please provide a v2 once it reposts, after 10th July.
> 
> oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.

Thanks Eric,

much appreciated.