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: <CADxym3ZEvUYwfvh2O5M+aYmLSMe_eZ8n=X_qBj8DiN8hh2OkaQ@mail.gmail.com>
Date: Sat, 17 Aug 2024 19:33:23 +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 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?

> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index e971c4785962..9a61f04bb95d 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
> >  /* Callback from net/ipv4/udp.c to receive packets */
> >  static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >  {
> > +     enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
>
> Do not call complex functions inline as variable init..

Okay!

>
> >       struct vxlan_vni_node *vninode = NULL;
> >       struct vxlan_dev *vxlan;
> >       struct vxlan_sock *vs;
> > @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >       int nh;
> >
> >       /* Need UDP and VXLAN header to be present */
> > -     if (!pskb_may_pull(skb, VXLAN_HLEN))
> > +     if (reason != SKB_NOT_DROPPED_YET)
>
> please don't compare against "not dropped yet", just:
>

Okay!

>         if (reason)
>
> > @@ -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?
So, we need to check if the drop reason is SKB_NOT_DROPPED_YET
when we call kfree_skb_reason().

We use "SKB_DR_OR(drop_reason, NOT_SPECIFIED);" in tcp_v4_rcv()
for this purpose. And we can remove SKB_DR_RESET and replace it
with "SKB_DR_OR(drop_reason, NOT_SPECIFIED);" here if you think
it's ok.

Thanks!
Menglong Dong
> >       /* Consume bad packet */
> > -     kfree_skb(skb);
> > +     kfree_skb_reason(skb, reason);
> >       return 0;
> >  }