Message-ID: <a2dec2d0-84be-4a4f-bfd4-b5f56219ac82@redhat.com>
Date: Tue, 26 Aug 2025 18:17:09 +0200
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 v15 14/15] net: homa: create homa_plumbing.c

On 8/18/25 10:55 PM, John Ousterhout wrote:
> +/* This variable contains the address of the statically-allocated struct homa
> + * used throughout Homa. This variable should almost never be used directly:
> + * it should be passed as a parameter to functions that need it. This
> + * variable is used only by a few functions called from Linux where there
> + * is no struct homa* available.
> + */
> +static struct homa *global_homa = &homa_data;

No need for this, use hame_data directly everywhere.

> +static struct proto homav6_prot = {
> +	.name		   = "HOMAv6",
> +	.owner		   = THIS_MODULE,
> +	.close		   = homa_close,
> +	.connect	   = ip6_datagram_connect,
> +	.ioctl		   = homa_ioctl,
> +	.init		   = homa_socket,
> +	.destroy	   = homa_sock_destroy,
> +	.setsockopt	   = homa_setsockopt,
> +	.getsockopt	   = homa_getsockopt,
> +	.sendmsg	   = homa_sendmsg,
> +	.recvmsg	   = homa_recvmsg,
> +	.hash		   = homa_hash,
> +	.unhash		   = homa_unhash,
> +	.obj_size	   = sizeof(struct homa_v6_sock),
> +	.ipv6_pinfo_offset = offsetof(struct homa_v6_sock, inet6),
> +
> +	.no_autobind       = 1,

Minor nit: no empty line above

> +};
> +
> +/* Top-level structure describing the Homa protocol. */
> +static struct inet_protosw homa_protosw = {
> +	.type              = SOCK_DGRAM,
> +	.protocol          = IPPROTO_HOMA,
> +	.prot              = &homa_prot,
> +	.ops               = &homa_proto_ops,
> +	.flags             = INET_PROTOSW_REUSE,
> +};
> +
> +static struct inet_protosw homav6_protosw = {
> +	.type              = SOCK_DGRAM,
> +	.protocol          = IPPROTO_HOMA,
> +	.prot              = &homav6_prot,
> +	.ops               = &homav6_proto_ops,
> +	.flags             = INET_PROTOSW_REUSE,
> +};
> +
> +/* This structure is used by IP to deliver incoming Homa packets to us. */
> +static struct net_protocol homa_protocol = {
> +	.handler =	homa_softirq,
> +	.err_handler =	homa_err_handler_v4,
> +	.no_policy =     1,
> +};
> +
> +static struct inet6_protocol homav6_protocol = {
> +	.handler =	homa_softirq,
> +	.err_handler =	homa_err_handler_v6,
> +	.flags =        INET6_PROTO_NOPOLICY | INET6_PROTO_FINAL,
> +};
> +
> +/* Sizes of the headers for each Homa packet type, in bytes. */
> +static u16 header_lengths[] = {
> +	sizeof(struct homa_data_hdr),
> +	0,
> +	sizeof(struct homa_resend_hdr),
> +	sizeof(struct homa_rpc_unknown_hdr),
> +	sizeof(struct homa_busy_hdr),
> +	0,
> +	0,
> +	sizeof(struct homa_need_ack_hdr),
> +	sizeof(struct homa_ack_hdr)
> +};
> +
> +/* Thread that runs timer code to detect lost packets and crashed peers. */
> +static struct task_struct *timer_kthread;
> +static DECLARE_COMPLETION(timer_thread_done);
> +
> +/* Used to wakeup timer_kthread at regular intervals. */
> +static struct hrtimer hrtimer;
> +
> +/* Nonzero is an indication to the timer thread that it should exit. */
> +static int timer_thread_exit;
> +
> +/**
> + * homa_load() - invoked when this module is loaded into the Linux kernel
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int __init homa_load(void)
> +{
> +	struct homa *homa = global_homa;
> +	bool init_protocol6 = false;
> +	bool init_protosw6 = false;
> +	bool init_protocol = false;
> +	bool init_protosw = false;
> +	bool init_net_ops = false;
> +	bool init_proto6 = false;
> +	bool init_proto = false;
> +	bool init_homa = false;
> +	int status;
> +
> +	/* Compile-time validations that no packet header is longer
> +	 * than HOMA_MAX_HEADER.
> +	 */
> +	BUILD_BUG_ON(sizeof(struct homa_data_hdr) > HOMA_MAX_HEADER);
> +	BUILD_BUG_ON(sizeof(struct homa_resend_hdr) > HOMA_MAX_HEADER);
> +	BUILD_BUG_ON(sizeof(struct homa_rpc_unknown_hdr) > HOMA_MAX_HEADER);
> +	BUILD_BUG_ON(sizeof(struct homa_busy_hdr) > HOMA_MAX_HEADER);
> +	BUILD_BUG_ON(sizeof(struct homa_need_ack_hdr) > HOMA_MAX_HEADER);
> +	BUILD_BUG_ON(sizeof(struct homa_ack_hdr) > HOMA_MAX_HEADER);
> +
> +	/* Extra constraints on data packets:
> +	 * - Ensure minimum header length so Homa doesn't have to worry about
> +	 *   padding data packets.
> +	 * - Make sure data packet headers are a multiple of 4 bytes (needed
> +	 *   for TCP/TSO compatibility).
> +	 */
> +	BUILD_BUG_ON(sizeof(struct homa_data_hdr) < HOMA_MIN_PKT_LENGTH);
> +	BUILD_BUG_ON((sizeof(struct homa_data_hdr) -
> +		      sizeof(struct homa_seg_hdr)) & 0x3);
> +
> +	/* Detect size changes in uAPI structs. */
> +	BUILD_BUG_ON(sizeof(struct homa_sendmsg_args) != 24);
> +	BUILD_BUG_ON(sizeof(struct homa_recvmsg_args) != 88);
> +
> +	pr_err("Homa module loading\n");

Please use pr_notice() instead.

> +	status = proto_register(&homa_prot, 1);
> +	if (status != 0) {
> +		pr_err("proto_register failed for homa_prot: %d\n", status);
> +		goto error;
> +	}
> +	init_proto = true;

The standard way of handling the error paths it to avoid local flags and
use different goto labels.

> +
> +	status = proto_register(&homav6_prot, 1);
> +	if (status != 0) {
> +		pr_err("proto_register failed for homav6_prot: %d\n", status);
> +		goto error;
> +	}
> +	init_proto6 = true;
> +
> +	inet_register_protosw(&homa_protosw);
> +	init_protosw = true;
> +
> +	status = inet6_register_protosw(&homav6_protosw);
> +	if (status != 0) {
> +		pr_err("inet6_register_protosw failed in %s: %d\n", __func__,
> +		       status);
> +		goto error;
> +	}
> +	init_protosw6 = true;
> +
> +	status = inet_add_protocol(&homa_protocol, IPPROTO_HOMA);
> +	if (status != 0) {
> +		pr_err("inet_add_protocol failed in %s: %d\n", __func__,
> +		       status);
> +		goto error;
> +	}
> +	init_protocol = true;
> +
> +	status = inet6_add_protocol(&homav6_protocol, IPPROTO_HOMA);
> +	if (status != 0) {
> +		pr_err("inet6_add_protocol failed in %s: %d\n",  __func__,
> +		       status);
> +		goto error;
> +	}
> +	init_protocol6 = true;
> +
> +	status = homa_init(homa);
> +	if (status)
> +		goto error;
> +	init_homa = true;

home_init() should be likely the first call in this function

> +
> +	status = register_pernet_subsys(&homa_net_ops);
> +	if (status != 0) {
> +		pr_err("Homa got error from register_pernet_subsys: %d\n",
> +		       status);
> +		goto error;
> +	}
> +	init_net_ops = true;
> +
> +	timer_kthread = kthread_run(homa_timer_main, homa, "homa_timer");
> +	if (IS_ERR(timer_kthread)) {
> +		status = PTR_ERR(timer_kthread);
> +		pr_err("couldn't create Homa timer thread: error %d\n",
> +		       status);
> +		timer_kthread = NULL;
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	if (timer_kthread) {
> +		timer_thread_exit = 1;
> +		wake_up_process(timer_kthread);
> +		wait_for_completion(&timer_thread_done);
> +	}
> +	if (init_net_ops)
> +		unregister_pernet_subsys(&homa_net_ops);
> +	if (init_homa)
> +		homa_destroy(homa);
> +	if (init_protocol)
> +		inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> +	if (init_protocol6)
> +		inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> +	if (init_protosw)
> +		inet_unregister_protosw(&homa_protosw);
> +	if (init_protosw6)
> +		inet6_unregister_protosw(&homav6_protosw);
> +	if (init_proto)
> +		proto_unregister(&homa_prot);
> +	if (init_proto6)
> +		proto_unregister(&homav6_prot);
> +	return status;
> +}
> +
> +/**
> + * homa_unload() - invoked when this module is unloaded from the Linux kernel.
> + */
> +void __exit homa_unload(void)
> +{
> +	struct homa *homa = global_homa;
> +
> +	pr_notice("Homa module unloading\n");
> +
> +	unregister_pernet_subsys(&homa_net_ops);
> +	homa_destroy(homa);

home_destroy() should likely be the last call of this function.

> +/**
> + * homa_softirq() - This function is invoked at SoftIRQ level to handle
> + * incoming packets.
> + * @skb:   The incoming packet.
> + * Return: Always 0
> + */
> +int homa_softirq(struct sk_buff *skb)
> +{
> +	struct sk_buff *packets, *other_pkts, *next;
> +	struct sk_buff **prev_link, **other_link;
> +	struct homa_common_hdr *h;
> +	int header_offset;
> +
> +	/* skb may actually contain many distinct packets, linked through
> +	 * skb_shinfo(skb)->frag_list by the Homa GRO mechanism. Make a
> +	 * pass through the list to process all of the short packets,
> +	 * leaving the longer packets in the list. Also, perform various
> +	 * prep/cleanup/error checking functions.

It's hard to tell without the GRO/GSO code handy, but I guess the
implementation here could be simplified invoking __skb_gso_segment()...

> +	 */
> +	skb->next = skb_shinfo(skb)->frag_list;
> +	skb_shinfo(skb)->frag_list = NULL;
> +	packets = skb;
> +	prev_link = &packets;
> +	for (skb = packets; skb; skb = next) {
> +		next = skb->next;
> +
> +		/* Make the header available at skb->data, even if the packet
> +		 * is fragmented. One complication: it's possible that the IP
> +		 * header hasn't yet been removed (this happens for GRO packets
> +		 * on the frag_list, since they aren't handled explicitly by IP.

... at very least it will avoif this complication and will simplify the
list handling.

> +		 */
> +		if (!homa_make_header_avl(skb))
> +			goto discard;

It looks like the above is too aggressive, i.e. pskb_may_pull() may fail
for a correctly formatted homa_ack_hdr - or any other packet with hdr
size < HOMA_MAX_HEADER

> +		header_offset = skb_transport_header(skb) - skb->data;
> +		if (header_offset)
> +			__skb_pull(skb, header_offset);
> +
> +		/* Reject packets that are too short or have bogus types. */
> +		h = (struct homa_common_hdr *)skb->data;
> +		if (unlikely(skb->len < sizeof(struct homa_common_hdr) ||
> +			     h->type < DATA || h->type > MAX_OP ||
> +			     skb->len < header_lengths[h->type - DATA]))
> +			goto discard;
> +
> +		/* Process the packet now if it is a control packet or
> +		 * if it contains an entire short message.
> +		 */
> +		if (h->type != DATA || ntohl(((struct homa_data_hdr *)h)
> +				->message_length) < 1400) {

I could not fined where `message_length` is validated. AFAICS
data_hdr->message_length could be > skb->len.

Also I don't see how the condition checked above ensures that the pkt
contains the whole message.

/P

Powered by blists - more mailing lists