netdev - Re: [PATCH net-next v3 02/12] net: homa: define Homa packet formats

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: <CAGXJAmwvkFF4Xfmf64GZA0vUUcpZhj7SsUR+0ifoxXTcYrLgXA@mail.gmail.com>
Date: Fri, 13 Dec 2024 10:32:19 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 02/12] net: homa: define Homa packet formats

On Thu, Dec 12, 2024 at 4:29 AM Edward Cree <ecree.xilinx@...il.com> wrote:
>
> On 09/12/2024 17:51, John Ousterhout wrote:
> > Signed-off-by: John Ousterhout <ouster@...stanford.edu>
> ...> +/**
> > + * define ETHERNET_MAX_PAYLOAD - Maximum length of an Ethernet packet,
> > + * excluding preamble, frame delimeter, VLAN header, CRC, and interpacket gap;
> > + * i.e. all of this space is available for Homa.
> > + */
> > +#define ETHERNET_MAX_PAYLOAD 1500
>
> This would seem to bake in an assumption about Ethernet MTU, which is
>  bad as this can vary on different Ethernet networks (e.g. jumbo frames
>  on the one hand, or VxLAN on the other which reduces MTU to 1460 iirc).
> In any case this define doesn't seem to be used anywhere in this patch
>  series (nor in the version on Github), so I'd say just dike it out.

I have deleted it (should have been done a long time ago).

> > +/**
> > + * struct common_header - Wire format for the first bytes in every Homa
> > + * packet. This must (mostly) match the format of a TCP header to enable
> > + * Homa packets to actually be transmitted as TCP packets (and thereby
> > + * take advantage of TSO and other features).
> > + */
> > +struct common_header {
>
> Arguably names like this should be namespaced (i.e. call it "struct
>  homa_common_header"), as otherwise some other kernel .h file declaring
>  a different "struct common_header" type for unrelated uses would cause
>  compilation errors if both are included in the same unit.
> Some of your typenames are namespaced and some aren't, and it's not clear
>  how that division has been made.

I have now changed all of those names to have "homa_" prefixes.

> > +     __u8 dummy1;
>
> This is the flags byte in TCP, right?  Seems like you'd need to specify
>  something about what goes here (or at least make it reserved) so as not
>  to confuse TSO implementations.  I see HomaModule defines it as filled
>  in with SYN|RST, at least the commit message here should say something
>  about why you've not done the same in the kernel.

I'll change the name to "reserved1". By the time Homa is fully
upstreamed this will be named "flags" and will be used in TCP
hijacking. I have removed TCP hijacking from this first patch series
in order to reduce its length.

> > +     __be16 dummy2;
>
> Again, specify something here, even if it's only "reserved, must be 0"
>  to allow for future extension.

I'll rename this to "reserved" also (it doesn't have to be zero right
now, since TCP hijacking isn't included).

> > + *    No hijacking:
>
> Assuming you don't intend to try to upstream TCP hijacking, this line
>  probably doesn't want to be here as without context it'll just confuse
>  the reader.

This was accidentally left behind when I removed hijacking. I'll
remove this line.

Thanks for the comments.

-John-