netdev - Re: [net-next] neigh: remove duplicate check for same neigh

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]

Date:   Tue, 29 Nov 2016 09:22:49 -0700
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Zhang Shengju <zhangshengju@...s.chinamobile.com>,
        netdev@...r.kernel.org
Subject: Re: [net-next] neigh: remove duplicate check for same neigh

On 11/29/16 1:22 AM, Zhang Shengju wrote:
> @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
>  	rcu_read_lock_bh();
>  	nht = rcu_dereference_bh(tbl->nht);
>  
> -	for (h = s_h; h < (1 << nht->hash_shift); h++) {
> -		if (h > s_h)
> -			s_idx = 0;
> +	for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
>  		for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
>  		     n != NULL;
> -		     n = rcu_dereference_bh(n->next)) {
> -			if (!net_eq(dev_net(n->dev), net))
> -				continue;
> -			if (neigh_ifindex_filtered(n->dev, filter_idx))
> +		     n = rcu_dereference_bh(n->next), idx++) {

incrementing idx here ...

> +			if (idx < s_idx || !net_eq(dev_net(n->dev), net))
>  				continue;
> -			if (neigh_master_filtered(n->dev, filter_master_idx))
> +			if (neigh_dump_filtered(n->dev, filter_idx,
> +						filter_master_idx))
>  				continue;
> -			if (idx < s_idx)
> -				goto next;
>  			if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
>  					    cb->nlh->nlmsg_seq,
>  					    RTM_NEWNEIGH,

... causes a missed entry when an error happens here

idx is only incremented when a neigh has been successfully added to the skb.


Your intention is to save a few cycles by making idx an absolute index within the hash bucket and jumping to the next entry to be dumped without evaluating any filters per entry.

So why not keep it simple and do this:

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 2ae929f9bd06..2e49a85e696a 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2291,14 +2291,12 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
                for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
                     n != NULL;
                     n = rcu_dereference_bh(n->next)) {
-                       if (!net_eq(dev_net(n->dev), net))
-                               continue;
-                       if (neigh_ifindex_filtered(n->dev, filter_idx))
-                               continue;
-                       if (neigh_master_filtered(n->dev, filter_master_idx))
-                               continue;
                        if (idx < s_idx)
                                goto next;
+                       if (!net_eq(dev_net(n->dev), net) ||
+                           neigh_ifindex_filtered(n->dev, filter_idx) ||
+                           neigh_master_filtered(n->dev, filter_master_idx))
+                               goto next;
                        if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
                                            cb->nlh->nlmsg_seq,
                                            RTM_NEWNEIGH,