netdev - Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan

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: <CADxym3YTH_AGnr6AZUO5PWL0nWDak7Kx+x-niA+Z-fmdsZ_OUA@mail.gmail.com>
Date: Wed, 21 Aug 2024 20:51:12 +0800
From: Menglong Dong <menglong8.dong@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, 
	dsahern@...nel.org, dongml2@...natelecom.cn, idosch@...dia.com, 
	amcohen@...dia.com, gnault@...hat.com, bpoirier@...dia.com, 
	b.galvani@...il.com, razor@...ckwall.org, petrm@...dia.com, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

On Tue, Aug 20, 2024 at 6:59 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote:
> > On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > >
> > > On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote:
> > > >  #define VXLAN_DROP_REASONS(R)                        \
> > > > +     R(VXLAN_DROP_FLAGS)                     \
> > > > +     R(VXLAN_DROP_VNI)                       \
> > > > +     R(VXLAN_DROP_MAC)                       \
> > >
> > > Drop reasons should be documented.
> >
> > Yeah, I wrote the code here just like what we did in
> > net/openvswitch/drop.h, which makes the definition of
> > enum ovs_drop_reason a call of VXLAN_DROP_REASONS().
> >
> > I think that we can define the enum ovs_drop_reason just like
> > what we do in include/net/dropreason-core.h, which can make
> > it easier to document the reasons.
> >
> > > I don't think name of a header field is a great fit for a reason.
> > >
> >
> > Enn...Do you mean the "VXLAN_DROP_" prefix?
>
> No, I mean the thing after VXLAN_DROP_, it's FLAGS, VNI, MAC,
> those are names of header fields.
>

Yeah, the reason here seems too simple. I use VXLAN_DROP_FLAGS
for any dropping out of vxlan flags. Just like what Ido advised, we can use
more descriptive reasons here, such as VXLAN_DROP_INVALID_HDR
for FLAGS, VXLAN_DROP_NO_VNI for vni not found, etc.

> > > > @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> > > >       return 0;
> > > >
> > > >  drop:
> > > > +     SKB_DR_RESET(reason);
> > >
> > > the name of this macro is very confusing, I don't think it should exist
> > > in the first place. nothing should goto drop without initialing reason
> > >
> >
> > It's for the case that we call a function which returns drop reasons.
> > For example, the reason now is assigned from:
> >
> >   reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
> >   if (reason) goto drop;
> >
> >   xxxxxx
> >   if (xx) goto drop;
> >
> > The reason now is SKB_NOT_DROPPED_YET when we "goto drop",
> > as we don't set a drop reason here, which is unnecessary in some cases.
> > And, we can't set the drop reason for every "drop" code path, can we?
>
> Why? It's like saying "we can't set return code before jumping to
> an error label". In my mind drop reasons and function return codes
> are very similar. So IDK why we need all the SK_DR_ macros when
> we are just fine without them for function return codes.

Of course we can. In my example above, we need to set
reason to SKB_DROP_REASON_NOT_SPECIFIED before we
jump to an error label:

reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
if (reason) goto drop;

xxxxxx
// we need to set reason here, or a WARN will be printed in
// kfree_skb_reason(), as reason now is SKB_NOT_DROPPED_YET.
reason = SKB_DROP_REASON_NOT_SPECIFIED;
if (xx) goto drop;

Should it be better to do it this way?

Thanks!
Menglong Dong