Re: [net-next,v17,11/14] net: homa: create homa_incoming.c

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: homa: create homa_incoming.c

This file contains most of the code for handling incoming packets,
including top-level dispatching code plus specific handlers for each
pack type. It also contains code for dispatching fully-received
messages to waiting application threads.

> diff --git a/net/homa/homa_impl.h b/net/homa/homa_impl.h
> index 4726762b688f..779abd7bb1b7 100644
> --- a/net/homa/homa_impl.h
> +++ b/net/homa/homa_impl.h

[ ... ]

> diff --git a/net/homa/homa_incoming.c b/net/homa/homa_incoming.c
> new file mode 100644
> index 000000000000..f083784921f4
> --- /dev/null
> +++ b/net/homa/homa_incoming.c

[ ... ]

> @@ -0,0 +1,887 @@

[ ... ]

> +/**
> + * homa_add_packet() - Add an incoming packet to the contents of a
> + * partially received message.
> + * @rpc:   Add the packet to the msgin for this RPC.
> + * @skb:   The new packet. This function takes ownership of the packet
> + *         (the packet will either be freed or added to rpc->msgin.packets).
> + */
> +void homa_add_packet(struct homa_rpc *rpc, struct sk_buff *skb)
> +	__must_hold(rpc->bucket->lock)
> +{
> +	struct homa_data_hdr *h = (struct homa_data_hdr *)skb->data;
> +	struct homa_gap *gap, *dummy, *gap2;
> +	int start = ntohl(h->seg.offset);
> +	int length = homa_data_len(skb);
> +	enum skb_drop_reason reason;
> +	int end = start + length;
> +
> +	if ((start + length) > rpc->msgin.length) {
> +		reason = SKB_DROP_REASON_PKT_TOO_BIG;
> +		goto discard;
> +	}
> +
> +	if (start == rpc->msgin.recv_end) {
> +		/* Common case: packet is sequential. */
> +		rpc->msgin.recv_end += length;
> +		goto keep;
> +	}
> +
> +	if (start > rpc->msgin.recv_end) {
> +		/* Packet creates a new gap. */
> +		if (!homa_gap_alloc(&rpc->msgin.gaps,
> +				    rpc->msgin.recv_end, start)) {
> +			reason = SKB_DROP_REASON_NOMEM;
> +			goto discard;
> +		}
> +		rpc->msgin.recv_end = end;
> +		goto keep;
> +	}
> +
> +	/* Must now check to see if the packet fills in part or all of
> +	 * an existing gap.
> +	 */
> +	list_for_each_entry_safe(gap, dummy, &rpc->msgin.gaps, links) {
> +		/* Is packet at the start of this gap? */
> +		if (start <= gap->start) {
> +			if (end <= gap->start)
> +				continue;
> +			if (start < gap->start) {
> +				reason = SKB_DROP_REASON_DUP_FRAG;
> +				goto discard;
> +			}
> +			if (end > gap->end) {
> +				reason = SKB_DROP_REASON_DUP_FRAG;
> +				goto discard;
> +			}
> +			gap->start = end;
> +			if (gap->start >= gap->end) {
> +				list_del(&gap->links);
> +				kfree(gap);
> +			}
> +			goto keep;
> +		}
> +
> +		/* Is packet at the end of this gap? BTW, at this point we know
> +		 * the packet can't cover the entire gap.
> +		 */
> +		if (end >= gap->end) {
> +			if (start >= gap->end)
> +				continue;
> +			if (end > gap->end) {
> +				reason = SKB_DROP_REASON_DUP_FRAG;
> +				goto discard;
> +			}
> +			gap->end = start;
> +			goto keep;
> +		}
> +
> +		/* Packet is in the middle of the gap; must split the gap. */
> +		gap2 = homa_gap_alloc(&gap->links, gap->start, start);
> +		if (!gap2) {
> +			reason = SKB_DROP_REASON_NOMEM;
> +			goto discard;
> +		}
> +		gap2->time = gap->time;
> +		gap->start = end;
> +		goto keep;
> +	}
> +
> +discard:
> +	kfree_skb_reason(skb, reason);
                           ^^^^^^

Can the 'reason' variable be used uninitialized here? When data has been
received sequentially (no gaps), a retransmitted duplicate packet with an
offset below recv_end won't match any of the early conditions (it doesn't
equal recv_end, it isn't greater than recv_end), so it falls through to the
gap traversal loop. With an empty gap list, the loop body never executes and
execution continues to the discard label where kfree_skb_reason() is called
with an uninitialized reason.

[ ... ]

> +/**
> + * homa_copy_to_user() - Copy as much data as possible from incoming
> + * packet buffers to buffers in user space.
> + * @rpc:     RPC for which data should be copied. Must be locked by caller.
> + * Return:   Zero for success or a negative errno if there is an error.
> + *           It is possible for the RPC to be freed while this function
> + *           executes (it releases and reacquires the RPC lock). If that
> + *           happens, -EINVAL will be returned and the state of @rpc
> + *           will be RPC_DEAD. Clears the RPC_PKTS_READY bit in @rpc->flags
> + *           if all available packets have been copied out.
> + */
> +int homa_copy_to_user(struct homa_rpc *rpc)
> +	__must_hold(rpc->bucket->lock)
> +{

[ ... ]

> +/**
> + * homa_dispatch_pkts() - Top-level function that processes a batch of packets,
> + * all related to the same RPC.
> + * @skb:       First packet in the batch, linked through skb->next.
> + */
> +void homa_dispatch_pkts(struct sk_buff *skb)
> +{

[ ... ]

> +		switch (h->common.type) {
> +		case DATA:
> +			homa_data_pkt(skb, rpc);
> +			break;
> +		case RESEND:
> +			homa_resend_pkt(skb, rpc, hsk);
> +			break;
> +		case RPC_UNKNOWN:
> +			homa_rpc_unknown_pkt(skb, rpc);
> +			break;
> +		case BUSY:
> +			/* Nothing to do for these packets except reset
> +			 * silent_ticks, which happened above.
> +			 */
> +			goto discard;
> +		case NEED_ACK:
> +			homa_need_ack_pkt(skb, hsk, rpc);
> +			break;
> +		case ACK:
> +			homa_ack_pkt(skb, hsk, rpc);
> +			break;
> +			goto discard;
                    ^^^^^^^^^^^^

This goto is unreachable because it follows the break statement above.
More critically, the switch has no default case. Looking at the validation
in homa_softirq (which checks 'h->type >= DATA && h->type <= MAX_OP'),
the header_lengths array (defined in homa_plumbing.c) has zero entries
for types 0x11, 0x15, and 0x16. Packets with these type values pass
validation since 'skb->len < 0' is always false.

When such a packet reaches homa_dispatch_pkts, no case matches, execution
falls through to 'continue', and the SKB is never freed. Can this leak
memory?

[ ... ]

> +/**
> + * homa_ack_pkt() - Handler for incoming ACK packets
> + * @skb:     Incoming packet; size already verified large enough for header.
> + *           This function now owns the packet.
> + * @hsk:     Socket on which the packet was received.
> + * @rpc:     The RPC named in the packet header, or NULL if no such
> + *           RPC exists. The RPC lock will be dead on return.
> + */
> +void homa_ack_pkt(struct sk_buff *skb, struct homa_sock *hsk,
> +		  struct homa_rpc *rpc)
> +	__must_hold(rpc->bucket->lock)
> +{
> +	const struct in6_addr saddr = skb_canonical_ipv6_saddr(skb);
> +	struct homa_ack_hdr *h = (struct homa_ack_hdr *)skb->data;
> +	int i, count;
> +
> +	if (rpc)
> +		homa_rpc_end(rpc);
> +
> +	count = ntohs(h->num_acks);
> +	if (count > 0) {
> +		if (rpc) {
> +			/* Must temporarily release rpc's lock because
> +			 * homa_rpc_acked needs to acquire RPC locks.
> +			 */
> +			homa_rpc_unlock(rpc);
> +			for (i = 0; i < count; i++)
> +				homa_rpc_acked(hsk, &saddr, &h->acks[i]);
                                                               ^^^^^^^^^^^

Is 'count' validated before accessing h->acks[i] in this loop? The
num_acks field comes from the network via ntohs() and can be any value
from 0 to 65535. The acks array in struct homa_ack_hdr is declared as
'struct homa_ack acks[HOMA_MAX_ACKS_PER_PKT]' where HOMA_MAX_ACKS_PER_PKT
is 5 in net/homa/homa_wire.h.

If a peer sends an ACK packet with num_acks > 5, this code reads beyond
the acks[] array boundary in the skb data. The out-of-bounds data is
passed to homa_rpc_acked() which dereferences it (reading ack->server_port
and ack->client_id). Can this cause protocol state corruption or a kernel
crash if the read extends beyond the skb's allocated linear data region?