netdev - Re: [RFC] Exempt multicast address from five-second neighbor lifetime

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: <20201016145952.000054ad@intel.com>
Date:   Fri, 16 Oct 2020 14:59:52 -0700
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     Jeff Dike <jdike@...mai.com>, netdev@...r.kernel.org
Subject: Re: [RFC] Exempt multicast address from five-second neighbor
 lifetime

Hi Jeff, 

Jeff Dike wrote:


Your subject should indicate net or net-next as the tree, please see:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

> Commit 58956317c8de guarantees arp table entries a five-second lifetime.  We have some apps which make heavy use of multicast, and these can cause the table to overflow by filling it with multicast addresses which can't be GC-ed until their five seconds are up.
> This patch allows multicast addresses to be thrown out before they've lived out their five seconds.

Not sure how many patches you've submitted, but your commit message
should be wrapped at 68 or 72 characters or so.

> 
> Signed-off-by: Jeff Dike <jdike@...mai.com>

your triple-dash and a diffstat should be right here, did you hand edit
this mail instead of using git format-patch to generate it?

> 
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 81ee17594c32..22ced1381ede 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -204,6 +204,7 @@ struct neigh_table {
>  	int			(*pconstructor)(struct pneigh_entry *);
>  	void			(*pdestructor)(struct pneigh_entry *);
>  	void			(*proxy_redo)(struct sk_buff *skb);
> +	int			(*is_multicast)(const void *pkey);
>  	bool			(*allow_add)(const struct net_device *dev,
>  					     struct netlink_ext_ack *extack);
>  	char			*id;
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 8e39e28b0a8d..9500d28a43b0 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -235,6 +235,8 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>  
>  			write_lock(&n->lock);
>  			if ((n->nud_state == NUD_FAILED) ||
> +			    (tbl->is_multicast &&
> +			     tbl->is_multicast(n->primary_key)) ||
>  			    time_after(tref, n->updated))
>  				remove = true;
>  			write_unlock(&n->lock);
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 687971d83b4e..110d6d408edc 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -79,6 +79,7 @@
>  #include <linux/socket.h>
>  #include <linux/sockios.h>
>  #include <linux/errno.h>
> +#define __UAPI_DEF_IN_CLASS 1

Why is this added in the middle of the includes?

>  #include <linux/in.h>
>  #include <linux/mm.h>
>  #include <linux/inet.h>
> @@ -125,6 +126,7 @@ static int arp_constructor(struct neighbour *neigh);
>  static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb);
>  static void arp_error_report(struct neighbour *neigh, struct sk_buff *skb);
>  static void parp_redo(struct sk_buff *skb);
> +static int arp_is_multicast(const void *pkey);
>  
>  static const struct neigh_ops arp_generic_ops = {
>  	.family =		AF_INET,
> @@ -156,6 +158,7 @@ struct neigh_table arp_tbl = {
>  	.key_eq		= arp_key_eq,
>  	.constructor	= arp_constructor,
>  	.proxy_redo	= parp_redo,
> +	.is_multicast   = arp_is_multicast,
>  	.id		= "arp_cache",
>  	.parms		= {
>  		.tbl			= &arp_tbl,
> @@ -928,6 +931,10 @@ static void parp_redo(struct sk_buff *skb)
>  	arp_process(dev_net(skb->dev), NULL, skb);
>  }
>  
> +static int arp_is_multicast(const void *pkey)
> +{
> +	return IN_MULTICAST(htonl(*((u32 *) pkey)));
> +}

Why not just move this function up and skip the declaration above?

>  
>  /*
>   *	Receive an arp request from the device layer.
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 27f29b957ee7..b42c9314cc4e 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -81,6 +81,7 @@ static void ndisc_error_report(struct neighbour *neigh, struct sk_buff *skb);
>  static int pndisc_constructor(struct pneigh_entry *n);
>  static void pndisc_destructor(struct pneigh_entry *n);
>  static void pndisc_redo(struct sk_buff *skb);
> +static int ndisc_is_multicast(const void *pkey);
>  
>  static const struct neigh_ops ndisc_generic_ops = {
>  	.family =		AF_INET6,
> @@ -115,6 +116,7 @@ struct neigh_table nd_tbl = {
>  	.pconstructor =	pndisc_constructor,
>  	.pdestructor =	pndisc_destructor,
>  	.proxy_redo =	pndisc_redo,
> +	.is_multicast = ndisc_is_multicast,
>  	.allow_add  =   ndisc_allow_add,
>  	.id =		"ndisc_cache",
>  	.parms = {
> @@ -1706,6 +1708,11 @@ static void pndisc_redo(struct sk_buff *skb)
>  	kfree_skb(skb);
>  }
>  
> +static int ndisc_is_multicast(const void *pkey)
> +{
> +	return (((struct in6_addr *) pkey)->in6_u.u6_addr8[0] & 0xf0) == 0xf0;
> +}
> +

Again, just move this up above the first usage?

Does the above work on big and little endian, just seems suspicious
even though you're using a byte offset? Also I suspect this will
trigger a warning with sparse or with W=2 about pointer alignment.