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

[Paolo Abeni <pabeni@redhat.com>]

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