netdev - Re: [PATCH net-next v2 1/2] Convert neighbour-table to use hlist

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] [day] [month] [year] [list]

Message-ID: <20241008145310.85530-1-kuniyu@amazon.com>
Date: Tue, 8 Oct 2024 07:53:10 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <gnaaman@...venets.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2 1/2] Convert neighbour-table to use hlist

From: Gilad Naaman <gnaaman@...venets.com>
Date: Tue,  8 Oct 2024 07:38:55 +0000
> > > @@ -388,21 +366,20 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
> > >  
> > >  	for (i = 0; i < (1 << nht->hash_shift); i++) {
> > >  		struct neighbour *n;
> > > -		struct neighbour __rcu **np = &nht->hash_buckets[i];
> > > +		struct neighbour __rcu **np =
> > > +			(struct neighbour __rcu **)&nht->hash_buckets[i].first;
> > 
> > This will be no longer needed for doubly linked list,
> 
> This is not as-necessary with a doubly-linked list, but unfortunately
> I cannot eliminate it completely, as the `n` might be released in the loop
> body.
> 
> I can convert this function to use a `struct neighour *next` instead,
> if it is more palatable.

Yes, using hlist_for_each_entry_safe() is more preferable.

Mixing for() and while() is harder to read.


[...]
> > > @@ -693,11 +666,10 @@ ___neigh_create(struct neigh_table *tbl, const void *pkey,
> > >  		goto out_tbl_unlock;
> > >  	}
> > >  
> > > -	for (n1 = rcu_dereference_protected(nht->hash_buckets[hash_val],
> > > -					    lockdep_is_held(&tbl->lock));
> > > -	     n1 != NULL;
> > > -	     n1 = rcu_dereference_protected(n1->next,
> > > -			lockdep_is_held(&tbl->lock))) {
> > > +	hlist_for_each_entry_rcu(n1,
> > > +				 &nht->hash_buckets[hash_val],
> > > +				 list,
> > > +				 lockdep_is_held(&tbl->lock)) {
> > 
> > Let's define hlist_for_each_entry_rcu() as neigh-specific macro.
> 
> Can you elaborate on this?
> Do you want the `list` parameter to be eliminated?

I mean like

#define neigh_for_each(...)		\
	hlist_for_each_entry(...)

#define neigh_for_each_rcu(...)		\
	hlist_for_each_entry_rcu(...)

are better if there's repeated arguments.