Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()
From: Menglong Dong
Date: Wed Aug 21 2024 - 08:51:43 EST
On Tue, Aug 20, 2024 at 6:59 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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@xxxxxxxxxx> 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