On Tue, Jun 30, 2020 at 12:16 PM Eric Dumazet [off-list ref] wrote:

On 6/30/20 6:57 AM, Willem de Bruijn wrote:

quoted

On Mon, Jun 29, 2020 at 10:19 PM Willem de Bruijn
[off-list ref] wrote:

quoted

On Mon, Jun 29, 2020 at 8:37 PM Tom Herbert [off-list ref] wrote:

quoted

On Mon, Jun 29, 2020 at 4:07 PM Eric Dumazet [off-list ref] wrote:

quoted

On 6/29/20 2:30 PM, Willem de Bruijn wrote:

quoted

On Mon, Jun 29, 2020 at 5:15 PM Eric Dumazet [off-list ref] wrote:

quoted

On 6/29/20 9:57 AM, Willem de Bruijn wrote:

quoted

From: Willem de Bruijn <willemb@google.com>

ICMP messages may include an extension structure after the original
datagram. RFC 4884 standardized this behavior.

It introduces an explicit original datagram length field in the ICMP
header to delineate the original datagram from the extension struct.

Return this field when reading an ICMP error from the error queue.

RFC mentions a 'length' field of 8 bits, your patch chose to export the whole
second word of icmp header.

Why is this field mapped to a prior one (icmp_hdr(skb)->un.gateway) ?

Should we add an element in the union to make this a little bit more explicit/readable ?

diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
index 5589eeb791ca580bb182e1dc38c05eab1c75adb9..427ed5a6765316a4c1e2fa06f3b6618447c01564 100644
--- a/include/uapi/linux/icmp.h
+++ b/include/uapi/linux/icmp.h

@@ -76,6 +76,7 @@ struct icmphdr {
                __be16  sequence;
        } echo;
        __be32  gateway;
+       __be32  second_word; /* RFC 4884 4.[123] : <unused:8>,<length:8>,<mtu:16> */
        struct {
                __be16  __unused;
                __be16  mtu;

Okay. How about a variant of the existing struct frag?

@@ -80,6 +80,11 @@ struct icmphdr {
                __be16  __unused;
                __be16  mtu;
        } frag;
+       struct {
+               __u8    __unused;
+               __u8    length;
+               __be16  mtu;
+       } rfc_4884;
        __u8    reserved[4];
   } un;

Sure, but my point was later in the code :

quoted

quoted

quoted

+     if (inet_sk(sk)->recverr_rfc4884)
+             info = ntohl(icmp_hdr(skb)->un.gateway);

ntohl(icmp_hdr(skb)->un.second_word);

If you leave there "info = ntohl(icmp_hdr(skb)->un.gateway)" it is a bit hard for someone
reading linux kernel code to understand why we do this.

It's also potentially problematic. The other bits are Unused, which
isn't the same thing as necessarily being zero. Userspace might assume
that info is the length without checking its bounded.

It shouldn't. The icmp type and code are passed in sock_extended_err
as ee_type and ee_code. So it can demultiplex the meaning of the rest
of the icmp header.

It just needs access to the other 32-bits, which indeed are context
sensitive. It makes more sense to me to let userspace demultiplex this
in one place, rather than demultiplex in the kernel and define a new,
likely no simpler, data structure to share with userspace.

Specific to RFC 4884, the 8-bit length field coexists with the
16-bit mtu field in case of ICMP_FRAG_NEEDED, so we cannot just pass
the first as ee_info in RFC 4884 mode. sock_extended_err additionally
has ee_data, but after that we're out of fields, too, so this approach
is not very future proof to additional ICMP extensions.

On your previous point, it might be useful to define struct rfc_4884
equivalent outside struct icmphdr, so that an application can easily
cast to that. RFC 4884 itself does not define any extension objects.
That is out of scope there, and in my opinion, here. Again, better
left to userspace. Especially because as it describes, it standardized
the behavior after observing non-compliant, but existing in the wild,
proprietary extension variants. Users may have to change how they
interpret the fields based on what they have deployed.

As this just shares the raw icmp header data, I should probably
change the name to something less specific to RFC 4884.

Since it would also help with decoding other extensions, such as
the one you mention in  draft-ietf-6man-icmp-limits-08.

Unfortunately I cannot simply reserve IP_RECVERR with integer 2.
Perhaps IP_RECVERR_EXINFO.

Perhaps let the icmp header as is, but provides the extra information
as an explicit ancillary message in ip_recv_error() ?

Really this is all about documentation and providing stable API.

Understood. Of course happy to discuss alternatives, as it does set
things in stone.

Possible alternative would be to add an union over ee_pad

Legacy applications would always get 0 for ee_pad/ee_length, while
applications enabling IP_RECVERR_RFC4884 would access the wire value.

And leave __u32 ee_data free for other uses.

I find it much more intuitive to just unconditionally pass the 32 bit
data that an application may need to be able to decode any ICMP
message (along with ee_type + ee_code), rather than start defining
ee_pad + ee_data fields in context dependent ways.

As for ICMP_FRAG_NEEDED now 24 of the 32 bits are defined, something
will inevitably find a use for the remaining 8 bits, and then we need
another kernel feature.

Also, if going down this path I will have to add the same for IPv6,
while it already exposes all this information userspace needs in
ee_info.

That said, if consensus is that the kernel should make more of an
effort to return this data in a structured form, and it is limited to
32 bits overall, the ee_pad/ee_len union for this case has my
preference. CMSG parsing adds a lot of boilerplate to each
application.