Message-ID: <CAGXJAmztO1SdjyMc6jdHf7Zz=WGnboR5w74kbmy4n-ZjJHNHQw@mail.gmail.com>
Date: Mon, 1 Sep 2025 15:53:35 -0700
From: John Ousterhout <ouster@...stanford.edu>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, horms@...nel.org, 
	kuba@...nel.org
Subject: Re: [PATCH net-next v15 14/15] net: homa: create homa_plumbing.c

On Tue, Aug 26, 2025 at 9:17 AM Paolo Abeni <pabeni@...hat.com> wrote:

> > +     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.

I initially implemented this with different goto labels, but there
were so many different labels that the code became unmanageable (very
difficult to figure out what to change when adding or removing
initializers). The current approach is *way* cleaner and more obvious,
so I hope I can keep it. The label approach works best when there is
only one label that collects all errors.

> > +/**
> > + * 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()...

This mechanism relates to GRO, not GSO. I suggest we hold off on this
discussion until I submit the GRO patch; I'm pretty sure there will be
a *lot* of discussion about that :-)

> > +      */
> > +     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.

As with the comment above, let's defer until you see the GRO mechanism
(a preview: Homa aggregates out-of-order packets in GRO, or even
packets from different RPCs, so it has to retain header information in
the aggregated data).

> > +              */
> > +             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

I think it's OK: homa_make_header_avl pulls the min of HOMA_MAX_HEADER
and the packet length. This may pull some bytes byte that aren't in
the header, but is that a problem? (this approach seemed
simpler/faster than trying to compute the header length on a
packet-by-packet basis; for example, )

> > +             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.

Long messages consist of multiple packets, so it is fine if
data_hdr->message_length > skb->len. That said, Homa does not fragment
a message into multiple packets unless necessary, so if the condition
above is met, then the message is contained in a single packet (if for
some reason a sender fragments a short message, that won't cause
problems).

The message length is validated in homa_message_in_init, invoked via
homa_softirq -> homa_dispatch_pkts -> homa_data_pkt ->
homa_message_in_init.

For comments that I haven't responded to explicitly here, I have
implemented your suggested fix.

-John-

Powered by blists - more mailing lists