On Tue, Aug 20, 2024 at 6:59 AM Jakub Kicinski [off-list ref] wrote:

On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote:

quoted

On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote:

quoted

 #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.

quoted

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.

quoted

quoted

quoted

@@ -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