On Wed, Feb 24, 2021 at 6:21 PM Andreas Roeseler
<andreas.a.roese...@gmail.com> wrote:
>
> On Sun, 2021-02-21 at 23:49 -0500, Willem de Bruijn wrote:
> On Wed, Feb 17, 2021 at 1:14 PM Andreas Roeseler
> <andreas.a.roese...@gmail.com> wrote:
> >
> > Modify the icmp_rcv function to check for PROBE messages and call
> > icmp_echo if a PROBE request is detected.
> >
> > Modify the existing icmp_echo function to respond to both ping and
> > PROBE
> > requests.
> >
> > This was tested using a custom modification of the iputils package
> > and
> > wireshark. It supports IPV4 probing by name, ifindex, and probing by
> > both IPV4 and IPV6
> > addresses. It currently does not support responding to probes off the
> > proxy node
> > (See RFC 8335 Section 2).
> >
> > Signed-off-by: Andreas Roeseler <andreas.a.roese...@gmail.com>
> > ---
> > Changes since v1:
> > - Reorder variable declarations to follow coding style
> > - Switch to functions such as dev_get_by_name and ip_dev_find to
> > lookup
> > net devices
> >
> > Changes since v2:
> > Suggested by Willem de Brujin <willemdebrujin.ker...@gmail.com>
> > - Add verification of incoming messages before looking up netdev
> > Reported-by: kernel test robot <l...@intel.com>
> > Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> > - Include net/addrconf.h library for ipv6_dev_find
> > + if (!buff)
> > + return -ENOMEM;
> > + memcpy(buff, &iio->ident.name, ident_len);
> > + dev = dev_get_by_name(net, buff);
> > + kfree(buff);
> > + break;
> > + case EXT_ECHO_CTYPE_INDEX:
> > + if (ident_len != sizeof(iio->ident.ifIndex)) {
>
> this checks that length is 4B, but RFC says "If the Interface
> Identification Object identifies the probed interface by index, the
> length is equal to 8 and the payload contains the if-index"
>
> ident_len stores the value of the identifier of the interface only,
> i.e. it stores the length of the iio minus the length of the iio
> header. Therefore, we can check its size against the expected size of
> an if_Index (4 octets)
Great. Thanks for clarifying.
> > @@ -1096,7 +1200,6 @@ int icmp_rcv(struct sk_buff *skb)
> > if (icmph->type > NR_ICMP_TYPES)
> > goto error;
> >
> > -
> > /*
> > * Parse the ICMP message
> > */
> > @@ -1123,6 +1226,7 @@ int icmp_rcv(struct sk_buff *skb)
> >
> > success = icmp_pointers[icmph->type].handler(skb);
> >
> > +success_check:
> > if (success) {
> > consume_skb(skb);
> > return NET_RX_SUCCESS;
> > @@ -1136,6 +1240,13 @@ int icmp_rcv(struct sk_buff *skb)
> > error:
> > __ICMP_INC_STATS(net, ICMP_MIB_INERRORS);
> > goto drop;
> > +probe:
> > + /*
> > + * We can't use icmp_pointers[].handler() because the codes
> > for PROBE
> > + * messages are 42 or 160
> > + */
>
> ICMPv6 message 160 (ICMPV6_EXT_ECHO_REQUEST) must be handled in
> icmpv6_rcv, not icmp_rcv. Then the ICMPv4 message 42 can be handled in
> the usual way.
>
>
> You are correct that we should handle ICMPV6_EXT_ECHO_REQUEST in the
> icmpv6.c file, but shouldn't we still have a special handler for the
> ICMPv4 message? The current icmp_pointers[].handler is an array of size
> NR_ICMP_TYPES + 1 (or 19 elements), so I don't think it would be a good
> idea to extend it to 42.
Interesting. So almost all numbers between NR_ICMP_TYPES (18) and 42
are deprecated:
https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml
Eventually we might get more extensions after 42. So you can go either way.
The table is the clean approach. But I see the practical point of extending it
for one case, too. The current branch approach looks fine to me.
