netdev - Re: [PATCH net-next v3 14/24] ovpn: implement multi-peer support

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: <ZldG5PNlvAkJ4fat@hog>
Date: Wed, 29 May 2024 17:16:52 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Sergey Ryazanov <ryazanov.s.a@...il.com>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 14/24] ovpn: implement multi-peer support

2024-05-28, 21:41:15 +0200, Antonio Quartulli wrote:
> On 28/05/2024 16:44, Sabrina Dubroca wrote:
> > Hi Antonio, I took a little break but I'm looking at your patches
> > again now.
> 
> Thanks Sabrina! Meanwhile I have been working on all your suggested changes.
> Right now I am familiarizing with the strparser.

Cool :)

> > 2024-05-06, 03:16:27 +0200, Antonio Quartulli wrote:
> > > +	index = ovpn_peer_index(ovpn->peers.by_id, &peer->id, sizeof(peer->id));
> > > +	hlist_add_head_rcu(&peer->hash_entry_id, &ovpn->peers.by_id[index]);
> > > +
> > > +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
> > > +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr,
> > > +					&peer->vpn_addrs.ipv4,
> > > +					sizeof(peer->vpn_addrs.ipv4));
> > > +		hlist_add_head_rcu(&peer->hash_entry_addr4,
> > > +				   &ovpn->peers.by_vpn_addr[index]);
> > > +	}
> > > +
> > > +	hlist_del_init_rcu(&peer->hash_entry_addr6);
> > 
> > Why are hash_entry_transp_addr and hash_entry_addr6 getting a
> > hlist_del_init_rcu() call, but not hash_entry_id and hash_entry_addr4?
> 
> I think not calling del_init_rcu on hash_entry_addr4 was a mistake.
> 
> Calling del_init_rcu on addr4, addr6 and transp_addr is needed to put them
> in a known state in case they are not hashed.

hlist_del_init_rcu does nothing if node is not already on a list.

> While hash_entry_id always goes through hlist_add_head_rcu, therefore
> del_init_rcu is useless (to my understanding).

I'm probably missing something about how this all fits together. In
patch 19, I see ovpn_nl_set_peer_doit can re-add a peer that is
already added (but I'm not sure why, since you don't allow changing
the addresses, so it won't actually be re-hashed).

I don't think doing a 2nd add of the same element to peers.by_id (or
any of the other hashtables) is correct, so I'd say you need
hlist_del_init_rcu for all of them.

-- 
Sabrina