From: Eric Dumazet <eric.dumazet@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
Eric Dumazet <eric.dumazet@gmail.com>
Cc: Network Development <netdev@vger.kernel.org>,
David Miller <davem@davemloft.net>
Subject: Re: [PATCH net-next] icmp: support rfc 4884
Date: Mon, 29 Jun 2020 16:06:58 -0700 [thread overview]
Message-ID: <f713198c-5ff7-677e-a739-c0bec4a93bd6@gmail.com> (raw)
In-Reply-To: <CA+FuTSfgz54uQbzrMr1Q0cAg2Vs1TFjyOb_+jjKUPoKAb=R-fw@mail.gmail.com>
On 6/29/20 2:30 PM, Willem de Bruijn wrote:
> On Mon, Jun 29, 2020 at 5:15 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 6/29/20 9:57 AM, Willem de Bruijn wrote:
>>> 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 :
>>> + 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.
next prev parent reply other threads:[~2020-06-29 23:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-29 16:57 [PATCH net-next] icmp: support rfc 4884 Willem de Bruijn
2020-06-29 20:34 ` Tom Herbert
2020-06-29 20:55 ` Willem de Bruijn
2020-06-29 21:14 ` Eric Dumazet
2020-06-29 21:30 ` Willem de Bruijn
2020-06-29 23:06 ` Eric Dumazet [this message]
2020-06-30 0:36 ` Tom Herbert
2020-06-30 2:18 ` Willem de Bruijn
2020-06-30 13:57 ` Willem de Bruijn
2020-06-30 16:16 ` Eric Dumazet
2020-06-30 16:41 ` Tom Herbert
2020-06-30 16:52 ` Willem de Bruijn
2020-06-30 18:57 ` Tom Herbert
2020-06-30 19:27 ` Willem de Bruijn
2020-06-30 16:41 ` Willem de Bruijn
2020-06-30 0:30 ` Tom Herbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f713198c-5ff7-677e-a739-c0bec4a93bd6@gmail.com \
--to=eric.dumazet@gmail.com \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).