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

[Menglong Dong]

On Tue, Aug 20, 2024 at 8:27 PM Ido Schimmel <idosch@xxxxxxxxxx> wrote:
>
> On Thu, Aug 15, 2024 at 08:42:58PM +0800, Menglong Dong wrote:
> > 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);
> >       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)
> >               goto drop;
> >
> >       unparsed = *vxlan_hdr(skb);
> > @@ -1690,6 +1691,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >               netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
> >                          ntohl(vxlan_hdr(skb)->vx_flags),
> >                          ntohl(vxlan_hdr(skb)->vx_vni));
> > +             reason = (u32)VXLAN_DROP_FLAGS;
>
> I don't find "FLAGS" very descriptive. AFAICT the reason is used in
> these two cases:
>
> 1. "I flag" is not set
> 2. The reserved fields are not zero
>
> Maybe call it VXLAN_DROP_INVALID_HDR ?
>

Yeah, that makes sense.

> And I agree with the comment about documenting these drop reasons like
> in include/net/dropreason-core.h
>

Okay, I'm planning to do it this way.

> >               /* Return non vxlan pkt */
> >               goto drop;
> >       }
> > @@ -1703,8 +1705,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >       vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
> >
> >       vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode);
> > -     if (!vxlan)
> > +     if (!vxlan) {
> > +             reason = (u32)VXLAN_DROP_VNI;
>
> Same comment here. Maybe VXLAN_DROP_VNI_NOT_FOUND ?
>

Yeah, sounds nice.

> >               goto drop;
> > +     }
> >
> >       /* For backwards compatibility, only allow reserved fields to be
> >        * used by VXLAN extensions if explicitly requested.
> > @@ -1717,12 +1721,16 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >       }
> >
> >       if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto,
> > -                                !net_eq(vxlan->net, dev_net(vxlan->dev))))
> > +                                !net_eq(vxlan->net, dev_net(vxlan->dev)))) {
> > +             reason = SKB_DROP_REASON_NOMEM;
> >               goto drop;
> > +     }
> >
> > -     if (vs->flags & VXLAN_F_REMCSUM_RX)
> > -             if (unlikely(!vxlan_remcsum(&unparsed, skb, vs->flags)))
> > +     if (vs->flags & VXLAN_F_REMCSUM_RX) {
> > +             reason = vxlan_remcsum(&unparsed, skb, vs->flags);
> > +             if (unlikely(reason != SKB_NOT_DROPPED_YET))
> >                       goto drop;
> > +     }
> >
> >       if (vxlan_collect_metadata(vs)) {
> >               IP_TUNNEL_DECLARE_FLAGS(flags) = { };
> > @@ -1732,8 +1740,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >               tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), flags,
> >                                        key32_to_tunnel_id(vni), sizeof(*md));
> >
> > -             if (!tun_dst)
> > +             if (!tun_dst) {
> > +                     reason = SKB_DROP_REASON_NOMEM;
> >                       goto drop;
> > +             }
> >
> >               md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
> >
> > @@ -1757,12 +1767,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >                * is more robust and provides a little more security in
> >                * adding extensions to VXLAN.
> >                */
> > +             reason = (u32)VXLAN_DROP_FLAGS;
> >               goto drop;
> >       }
> >
> >       if (!raw_proto) {
> > -             if (!vxlan_set_mac(vxlan, vs, skb, vni))
> > +             if (!vxlan_set_mac(vxlan, vs, skb, vni)) {
> > +                     reason = (u32)VXLAN_DROP_MAC;
>
> The function drops the packet for various reasons:
>
> 1. Source MAC is equal to the VXLAN device's MAC
> 2. Source MAC is invalid (all zeroes or multicast)
> 3. Trying to migrate a static entry or one pointing to a nexthop
>
> They are all quite obscure so it might be fine to fold them under the
> same reason, but I can't find a descriptive name.
>
> If you split 1-2 to one reason and 3 to another, then they can become
> VXLAN_DROP_INVALID_SMAC and VXLAN_DROP_ENTRY_EXISTS
>

Sounds great! Thanks for creating these names for me,
I'm really not good at naming :/

> >                       goto drop;
> > +             }