diff mbox series

[net-next,v2,06/12] net: vxlan: make vxlan_set_mac() return drop reasons

Message ID 20240830020001.79377-7-dongml2@chinatelecom.cn (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: vxlan: add skb drop reasons support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 17 this patch: 17
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Menglong Dong <menglong8.dong@gmail.com>' != 'Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-30--21-00 (tests: 713)

Commit Message

Menglong Dong Aug. 30, 2024, 1:59 a.m. UTC
Change the return type of vxlan_set_mac() from bool to enum
skb_drop_reason. In this commit, two drop reasons are introduced:

  VXLAN_DROP_INVALID_SMAC
  VXLAN_DROP_ENTRY_EXISTS

To make it easier to document the reasons in drivers/net/vxlan/drop.h,
we don't define the enum vxlan_drop_reason with the macro
VXLAN_DROP_REASONS(), but hand by hand.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/drop.h       |  9 +++++++++
 drivers/net/vxlan/vxlan_core.c | 12 ++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)

Comments

Alexander Lobakin Aug. 30, 2024, 2:57 p.m. UTC | #1
From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:55 +0800

> Change the return type of vxlan_set_mac() from bool to enum
> skb_drop_reason. In this commit, two drop reasons are introduced:
> 
>   VXLAN_DROP_INVALID_SMAC
>   VXLAN_DROP_ENTRY_EXISTS
> 
> To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> we don't define the enum vxlan_drop_reason with the macro
> VXLAN_DROP_REASONS(), but hand by hand.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  drivers/net/vxlan/drop.h       |  9 +++++++++
>  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 6bcc6894fbbd..876b4a9de92f 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -9,11 +9,20 @@
>  #include <net/dropreason.h>
>  
>  #define VXLAN_DROP_REASONS(R)			\
> +	R(VXLAN_DROP_INVALID_SMAC)		\
> +	R(VXLAN_DROP_ENTRY_EXISTS)		\

To Jakub:

In our recent conversation, you said you dislike templates much. What
about this one? :>

>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
>  	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
>  				SKB_DROP_REASON_SUBSYS_SHIFT,
> +	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> +	VXLAN_DROP_INVALID_SMAC,
> +	/**
> +	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> +	 * one pointing to a nexthop
> +	 */

Maybe you'd do a proper kdoc for this enum at the top?

> +	VXLAN_DROP_ENTRY_EXISTS,
>  };
>  
>  static inline void
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 76b217d166ef..58c175432a15 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
>  	unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
>  }
>  
> -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> -			  struct vxlan_sock *vs,
> -			  struct sk_buff *skb, __be32 vni)
> +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> +					  struct vxlan_sock *vs,
> +					  struct sk_buff *skb, __be32 vni)
>  {
>  	union vxlan_addr saddr;
>  	u32 ifindex = skb->dev->ifindex;
> @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	/* Ignore packet loops (and multicast echo) */
>  	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> -		return false;
> +		return (u32)VXLAN_DROP_INVALID_SMAC;
>  
>  	/* Get address from the outer IP header */
>  	if (vxlan_get_sk_family(vs) == AF_INET) {
> @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
>  	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> -		return false;
> +		return (u32)VXLAN_DROP_ENTRY_EXISTS;
>  
> -	return true;
> +	return (u32)SKB_NOT_DROPPED_YET;

Redundant cast.

>  }

Don't you need to adjust the call site accordingly? Previously, this
function returned false in case of error and true otherwise, now it
returns a non-zero in case of error and 0 (NOT_DROPPED_YET == 0) if
everything went fine.
IOW the call site now needs to check whether the return value !=
NOT_DROPPED_YET instead of checking whether it returned false. You
inverted the return code logic, but didn't touch the call site.

>  
>  static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,

Thanks,
Olek
Simon Horman Aug. 30, 2024, 3:31 p.m. UTC | #2
On Fri, Aug 30, 2024 at 09:59:55AM +0800, Menglong Dong wrote:
> Change the return type of vxlan_set_mac() from bool to enum
> skb_drop_reason. In this commit, two drop reasons are introduced:
> 
>   VXLAN_DROP_INVALID_SMAC
>   VXLAN_DROP_ENTRY_EXISTS
> 
> To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> we don't define the enum vxlan_drop_reason with the macro
> VXLAN_DROP_REASONS(), but hand by hand.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  drivers/net/vxlan/drop.h       |  9 +++++++++
>  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 6bcc6894fbbd..876b4a9de92f 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -9,11 +9,20 @@
>  #include <net/dropreason.h>
>  
>  #define VXLAN_DROP_REASONS(R)			\
> +	R(VXLAN_DROP_INVALID_SMAC)		\
> +	R(VXLAN_DROP_ENTRY_EXISTS)		\
>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
>  	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
>  				SKB_DROP_REASON_SUBSYS_SHIFT,
> +	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> +	VXLAN_DROP_INVALID_SMAC,
> +	/**
> +	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> +	 * one pointing to a nexthop

Maybe it is clearer to write: one -> an entry

> +	 */
> +	VXLAN_DROP_ENTRY_EXISTS,
>  };
>  
>  static inline void

...
Jakub Kicinski Aug. 30, 2024, 11:26 p.m. UTC | #3
On Fri, 30 Aug 2024 09:59:55 +0800 Menglong Dong wrote:
> -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> -			  struct vxlan_sock *vs,
> -			  struct sk_buff *skb, __be32 vni)
> +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> +					  struct vxlan_sock *vs,
> +					  struct sk_buff *skb, __be32 vni)
>  {
>  	union vxlan_addr saddr;
>  	u32 ifindex = skb->dev->ifindex;
> @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	/* Ignore packet loops (and multicast echo) */
>  	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> -		return false;
> +		return (u32)VXLAN_DROP_INVALID_SMAC;

It's the MAC address of the local interface, not just invalid...

>  	/* Get address from the outer IP header */
>  	if (vxlan_get_sk_family(vs) == AF_INET) {
> @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
>  	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> -		return false;
> +		return (u32)VXLAN_DROP_ENTRY_EXISTS;

... because it's vxlan_snoop() that checks:

        if (!is_valid_ether_addr(src_mac))
Menglong Dong Sept. 1, 2024, 12:37 p.m. UTC | #4
On Fri, Aug 30, 2024 at 11:31 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Aug 30, 2024 at 09:59:55AM +0800, Menglong Dong wrote:
> > Change the return type of vxlan_set_mac() from bool to enum
> > skb_drop_reason. In this commit, two drop reasons are introduced:
> >
> >   VXLAN_DROP_INVALID_SMAC
> >   VXLAN_DROP_ENTRY_EXISTS
> >
> > To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> > we don't define the enum vxlan_drop_reason with the macro
> > VXLAN_DROP_REASONS(), but hand by hand.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  drivers/net/vxlan/drop.h       |  9 +++++++++
> >  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 6bcc6894fbbd..876b4a9de92f 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -9,11 +9,20 @@
> >  #include <net/dropreason.h>
> >
> >  #define VXLAN_DROP_REASONS(R)                        \
> > +     R(VXLAN_DROP_INVALID_SMAC)              \
> > +     R(VXLAN_DROP_ENTRY_EXISTS)              \
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> >       __VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
> >                               SKB_DROP_REASON_SUBSYS_SHIFT,
> > +     /** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> > +     VXLAN_DROP_INVALID_SMAC,
> > +     /**
> > +      * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> > +      * one pointing to a nexthop
>
> Maybe it is clearer to write: one -> an entry
>

Okay!

> > +      */
> > +     VXLAN_DROP_ENTRY_EXISTS,
> >  };
> >
> >  static inline void
>
> ...
Menglong Dong Sept. 1, 2024, 12:47 p.m. UTC | #5
On Sat, Aug 31, 2024 at 7:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 30 Aug 2024 09:59:55 +0800 Menglong Dong wrote:
> > -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > -                       struct vxlan_sock *vs,
> > -                       struct sk_buff *skb, __be32 vni)
> > +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> > +                                       struct vxlan_sock *vs,
> > +                                       struct sk_buff *skb, __be32 vni)
> >  {
> >       union vxlan_addr saddr;
> >       u32 ifindex = skb->dev->ifindex;
> > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       /* Ignore packet loops (and multicast echo) */
> >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > -             return false;
> > +             return (u32)VXLAN_DROP_INVALID_SMAC;
>
> It's the MAC address of the local interface, not just invalid...
>

Yeah, my mistake. It seems that we need to add a
VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
or can we reuse VXLAN_DROP_INVALID_SMAC here too?

> >       /* Get address from the outer IP header */
> >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > -             return false;
> > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
>
> ... because it's vxlan_snoop() that checks:
>
>         if (!is_valid_ether_addr(src_mac))

It seems that we need to make vxlan_snoop() return skb drop reasons
too, and we need to add a new patch, which makes this series too many
patches. Any  advice?

I'll see if I can combine some of them into one.

Thanks!
Menglong Dong

> --
> pw-bot: cr
Menglong Dong Sept. 1, 2024, 12:56 p.m. UTC | #6
On Fri, Aug 30, 2024 at 10:58 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:55 +0800
>
> > Change the return type of vxlan_set_mac() from bool to enum
> > skb_drop_reason. In this commit, two drop reasons are introduced:
> >
> >   VXLAN_DROP_INVALID_SMAC
> >   VXLAN_DROP_ENTRY_EXISTS
> >
> > To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> > we don't define the enum vxlan_drop_reason with the macro
> > VXLAN_DROP_REASONS(), but hand by hand.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  drivers/net/vxlan/drop.h       |  9 +++++++++
> >  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 6bcc6894fbbd..876b4a9de92f 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -9,11 +9,20 @@
> >  #include <net/dropreason.h>
> >
> >  #define VXLAN_DROP_REASONS(R)                        \
> > +     R(VXLAN_DROP_INVALID_SMAC)              \
> > +     R(VXLAN_DROP_ENTRY_EXISTS)              \
>
> To Jakub:
>
> In our recent conversation, you said you dislike templates much. What
> about this one? :>
>
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> >       __VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
> >                               SKB_DROP_REASON_SUBSYS_SHIFT,
> > +     /** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> > +     VXLAN_DROP_INVALID_SMAC,
> > +     /**
> > +      * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> > +      * one pointing to a nexthop
> > +      */
>
> Maybe you'd do a proper kdoc for this enum at the top?
>

Do you mean the enum vxlan_drop_reason? Yeah, that makes
sense, and I'll doc it.

> > +     VXLAN_DROP_ENTRY_EXISTS,
> >  };
> >
> >  static inline void
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index 76b217d166ef..58c175432a15 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
> >       unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
> >  }
> >
> > -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > -                       struct vxlan_sock *vs,
> > -                       struct sk_buff *skb, __be32 vni)
> > +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> > +                                       struct vxlan_sock *vs,
> > +                                       struct sk_buff *skb, __be32 vni)
> >  {
> >       union vxlan_addr saddr;
> >       u32 ifindex = skb->dev->ifindex;
> > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       /* Ignore packet loops (and multicast echo) */
> >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > -             return false;
> > +             return (u32)VXLAN_DROP_INVALID_SMAC;
> >
> >       /* Get address from the outer IP header */
> >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > -             return false;
> > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
> >
> > -     return true;
> > +     return (u32)SKB_NOT_DROPPED_YET;
>
> Redundant cast.
>

Oops, I'll remove it.

> >  }
>
> Don't you need to adjust the call site accordingly? Previously, this
> function returned false in case of error and true otherwise, now it
> returns a non-zero in case of error and 0 (NOT_DROPPED_YET == 0) if
> everything went fine.
> IOW the call site now needs to check whether the return value !=
> NOT_DROPPED_YET instead of checking whether it returned false. You
> inverted the return code logic, but didn't touch the call site.
>

Yeah, you are right. I need to change the call of this function
in this patch too, rather than the next patch. Thanks for reminding
me of this point!

> >
> >  static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
>
> Thanks,
> Olek
Jakub Kicinski Sept. 3, 2024, 1:12 a.m. UTC | #7
On Sun, 1 Sep 2024 20:47:27 +0800 Menglong Dong wrote:
> > > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > >
> > >       /* Ignore packet loops (and multicast echo) */
> > >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > > -             return false;
> > > +             return (u32)VXLAN_DROP_INVALID_SMAC;  
> >
> > It's the MAC address of the local interface, not just invalid...
> >  
> 
> Yeah, my mistake. It seems that we need to add a
> VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
> or can we reuse VXLAN_DROP_INVALID_SMAC here too?

Could you take a look at the bridge code and see if it has similar
checks? Learning the addresses and dropping frames which don't match
static FDB entries seem like fairly normal L2 switching drop reasons.
Perhaps we could add these as non-VXLAN specific?

The subsystem reason API was added for wireless, because wireless
folks had their own encoding, and they have their own development
tree (we don't merge their patches directly into net-next).
I keep thinking that we should add the VXLAN reason to the "core"
group rather than creating a subsystem..

> > >       /* Get address from the outer IP header */
> > >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > >
> > >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> > >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > > -             return false;
> > > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;  
> >
> > ... because it's vxlan_snoop() that checks:
> >
> >         if (!is_valid_ether_addr(src_mac))  
> 
> It seems that we need to make vxlan_snoop() return skb drop reasons
> too, and we need to add a new patch, which makes this series too many
> patches. Any  advice?

You could save some indentation by inverting the condition:

 	if (!(vxlan->cfg.flags & VXLAN_F_LEARN))
		return (u32)SKB_NOT_DROPPED_YET;

	return vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni);

But yes, I don't see a better way than having vxlan_snoop() return a
reason code :(

The patch limit count is 15, 12 is our preferred number but you can go
higher if it helps the clarity of the series.
Menglong Dong Sept. 4, 2024, 1:59 a.m. UTC | #8
On Tue, Sep 3, 2024 at 9:12 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 1 Sep 2024 20:47:27 +0800 Menglong Dong wrote:
> > > > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > > >
> > > >       /* Ignore packet loops (and multicast echo) */
> > > >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > > > -             return false;
> > > > +             return (u32)VXLAN_DROP_INVALID_SMAC;
> > >
> > > It's the MAC address of the local interface, not just invalid...
> > >
> >
> > Yeah, my mistake. It seems that we need to add a
> > VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
> > or can we reuse VXLAN_DROP_INVALID_SMAC here too?
>
> Could you take a look at the bridge code and see if it has similar
> checks? Learning the addresses and dropping frames which don't match
> static FDB entries seem like fairly normal L2 switching drop reasons.
> Perhaps we could add these as non-VXLAN specific?
>

Yeah, I'll have a look at that part.

> The subsystem reason API was added for wireless, because wireless
> folks had their own encoding, and they have their own development
> tree (we don't merge their patches directly into net-next).
> I keep thinking that we should add the VXLAN reason to the "core"
> group rather than creating a subsystem..
>

I'm hesitant about this in the beginning too, as VXLAN is a standard
tunnel protocol. And I'm still hesitant now, should I add them to the "core"?
which will make things simpler.

Enn......I'll add them to the "core" in the next version, and let's see
if it is better.

> > > >       /* Get address from the outer IP header */
> > > >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > > > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > > >
> > > >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> > > >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > > > -             return false;
> > > > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
> > >
> > > ... because it's vxlan_snoop() that checks:
> > >
> > >         if (!is_valid_ether_addr(src_mac))
> >
> > It seems that we need to make vxlan_snoop() return skb drop reasons
> > too, and we need to add a new patch, which makes this series too many
> > patches. Any  advice?
>
> You could save some indentation by inverting the condition:
>
>         if (!(vxlan->cfg.flags & VXLAN_F_LEARN))
>                 return (u32)SKB_NOT_DROPPED_YET;
>
>         return vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni);
>
> But yes, I don't see a better way than having vxlan_snoop() return a
> reason code :(
>
> The patch limit count is 15, 12 is our preferred number but you can go
> higher if it helps the clarity of the series.

Okay! I feel much better with your words :/

Thanks!
Menglong Dong
diff mbox series

Patch

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index 6bcc6894fbbd..876b4a9de92f 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -9,11 +9,20 @@ 
 #include <net/dropreason.h>
 
 #define VXLAN_DROP_REASONS(R)			\
+	R(VXLAN_DROP_INVALID_SMAC)		\
+	R(VXLAN_DROP_ENTRY_EXISTS)		\
 	/* deliberate comment for trailing \ */
 
 enum vxlan_drop_reason {
 	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
 				SKB_DROP_REASON_SUBSYS_SHIFT,
+	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
+	VXLAN_DROP_INVALID_SMAC,
+	/**
+	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
+	 * one pointing to a nexthop
+	 */
+	VXLAN_DROP_ENTRY_EXISTS,
 };
 
 static inline void
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 76b217d166ef..58c175432a15 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1607,9 +1607,9 @@  static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
 	unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
 }
 
-static bool vxlan_set_mac(struct vxlan_dev *vxlan,
-			  struct vxlan_sock *vs,
-			  struct sk_buff *skb, __be32 vni)
+static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
+					  struct vxlan_sock *vs,
+					  struct sk_buff *skb, __be32 vni)
 {
 	union vxlan_addr saddr;
 	u32 ifindex = skb->dev->ifindex;
@@ -1620,7 +1620,7 @@  static bool vxlan_set_mac(struct vxlan_dev *vxlan,
 
 	/* Ignore packet loops (and multicast echo) */
 	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
-		return false;
+		return (u32)VXLAN_DROP_INVALID_SMAC;
 
 	/* Get address from the outer IP header */
 	if (vxlan_get_sk_family(vs) == AF_INET) {
@@ -1635,9 +1635,9 @@  static bool vxlan_set_mac(struct vxlan_dev *vxlan,
 
 	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
 	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
-		return false;
+		return (u32)VXLAN_DROP_ENTRY_EXISTS;
 
-	return true;
+	return (u32)SKB_NOT_DROPPED_YET;
 }
 
 static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,