netdev - Re: [PATCH net-next v6 07/12] net: homa: create homa_sock.h and homa

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: <afad8fa2-344f-4253-b929-a888b99e90a9@redhat.com>
Date: Fri, 24 Jan 2025 08:33:57 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: John Ousterhout <ouster@...stanford.edu>, netdev@...r.kernel.org
Cc: edumazet@...gle.com, horms@...nel.org, kuba@...nel.org
Subject: Re: [PATCH net-next v6 07/12] net: homa: create homa_sock.h and
 homa_sock.c

On 1/15/25 7:59 PM, John Ousterhout wrote:
> +/**
> + * homa_sock_init() - Constructor for homa_sock objects. This function
> + * initializes only the parts of the socket that are owned by Homa.
> + * @hsk:    Object to initialize.
> + * @homa:   Homa implementation that will manage the socket.
> + *
> + * Return: 0 for success, otherwise a negative errno.
> + */
> +int homa_sock_init(struct homa_sock *hsk, struct homa *homa)
> +{
> +	struct homa_socktab *socktab = homa->port_map;
> +	int starting_port;
> +	int result = 0;
> +	int i;
> +
> +	spin_lock_bh(&socktab->write_lock);
> +	atomic_set(&hsk->protect_count, 0);
> +	spin_lock_init(&hsk->lock);
> +	hsk->last_locker = "none";
> +	atomic_set(&hsk->protect_count, 0);
> +	hsk->homa = homa;
> +	hsk->ip_header_length = (hsk->inet.sk.sk_family == AF_INET)
> +			? HOMA_IPV4_HEADER_LENGTH : HOMA_IPV6_HEADER_LENGTH;
> +	hsk->shutdown = false;
> +	starting_port = homa->prev_default_port;
> +	while (1) {
> +		homa->prev_default_port++;
> +		if (homa->prev_default_port < HOMA_MIN_DEFAULT_PORT)
> +			homa->prev_default_port = HOMA_MIN_DEFAULT_PORT;
> +		if (!homa_sock_find(socktab, homa->prev_default_port))
> +			break;
> +		if (homa->prev_default_port == starting_port) {
> +			spin_unlock_bh(&socktab->write_lock);
> +			hsk->shutdown = true;
> +			return -EADDRNOTAVAIL;
> +		}
> +	}
> +	hsk->port = homa->prev_default_port;
> +	hsk->inet.inet_num = hsk->port;
> +	hsk->inet.inet_sport = htons(hsk->port);
> +	hsk->socktab_links.sock = hsk;
> +	hlist_add_head_rcu(&hsk->socktab_links.hash_links,
> +			   &socktab->buckets[homa_port_hash(hsk->port)]);

At this point the socket is apparently exposed to lookup from incoming
packets, but it's only partially initialized: bad things could happen.

> +/**
> + * homa_sock_find() - Returns the socket associated with a given port.
> + * @socktab:    Hash table in which to perform lookup.
> + * @port:       The port of interest.
> + * Return:      The socket that owns @port, or NULL if none.
> + *
> + * Note: this function uses RCU list-searching facilities, but it doesn't
> + * call rcu_read_lock. The caller should do that, if the caller cares (this
> + * way, the caller's use of the socket will also be protected).
> + */
> +struct homa_sock *homa_sock_find(struct homa_socktab *socktab,  __u16 port)

It would help the review if you reorder the code defining first the
basic helpers like this one and after the functions using them

> +{
> +	struct homa_socktab_links *link;
> +	struct homa_sock *result = NULL;
> +
> +	hlist_for_each_entry_rcu(link, &socktab->buckets[homa_port_hash(port)],
> +				 hash_links) {

This require the caller owing the rcu read lock, which is not always the
case in this patchset.

> +		struct homa_sock *hsk = link->sock;
> +
> +		if (hsk->port == port) {
> +			result = hsk;

The local port is the full key of the socket lookup? not even the
address? This simplify the code a bit, but is quite against user
expectation.

/P