On 12/3/20 8:17 PM, Andreas Roeseler wrote:
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 005faea415a4..313061b60387 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -984,20 +984,121 @@ static bool icmp_redirect(struct sk_buff *skb)
> static bool icmp_echo(struct sk_buff *skb)
> {
> struct net *net;
> + struct icmp_bxm icmp_param;
> + struct net_device *dev;
> + struct net_device *target_dev;
> + struct in_ifaddr *ifaddr;
> + struct inet6_ifaddr *inet6_ifaddr;
> + struct list_head *position;
> + struct icmp_extobj_hdr *extobj_hdr;
> + struct icmp_ext_ctype3_hdr *ctype3_hdr;
> + __u8 status;
networking coding style is reverse xmas tree — i.e., longest to shortest.
>
> net = dev_net(skb_dst(skb)->dev);
> - if (!net->ipv4.sysctl_icmp_echo_ignore_all) {
> - struct icmp_bxm icmp_param;
> + /* should there be an ICMP stat for ignored echos? */
> + if (net->ipv4.sysctl_icmp_echo_ignore_all)
> + return true;
> +
> + icmp_param.data.icmph = *icmp_hdr(skb);
> + icmp_param.skb = skb;
> + icmp_param.offset = 0;
> + icmp_param.data_len = skb->len;
> + icmp_param.head_len = sizeof(struct icmphdr);
>
> - icmp_param.data.icmph = *icmp_hdr(skb);
> + if (icmp_param.data.icmph.type == ICMP_ECHO) {
> icmp_param.data.icmph.type = ICMP_ECHOREPLY;
> - icmp_param.skb = skb;
> - icmp_param.offset = 0;
> - icmp_param.data_len = skb->len;
> - icmp_param.head_len = sizeof(struct icmphdr);
> - icmp_reply(&icmp_param, skb);
> + goto send_reply;
> }
> - /* should there be an ICMP stat for ignored echos? */
> + if (!net->ipv4.sysctl_icmp_echo_enable_probe)
> + return true;
> + /* We currently do not support probing off the proxy node */
> + if ((ntohs(icmp_param.data.icmph.un.echo.sequence) & 1) == 0)
> + return true;
> +
> + icmp_param.data.icmph.type = ICMP_EXT_ECHOREPLY;
> + icmp_param.data.icmph.un.echo.sequence &= htons(0xFF00);
> + extobj_hdr = (struct icmp_extobj_hdr *)(skb->data + sizeof(struct
> icmp_ext_hdr));
> + ctype3_hdr = (struct icmp_ext_ctype3_hdr *)(extobj_hdr + 1);
> + status = 0;
> + target_dev = NULL;
> + read_lock(&dev_base_lock);
> + for_each_netdev(net, dev) {
for_each_netdev needs to be replaced by an appropriate lookup.
> + switch (extobj_hdr->class_type) {
> + case CTYPE_NAME:
> + if (strcmp(dev->name, (char *)(extobj_hdr + 1)) == 0)
> + goto found_matching_interface;
> + break;
> + case CTYPE_INDEX:
> + if (ntohl(*((uint32_t *)(extobj_hdr + 1))) ==
> + dev->ifindex)
> + goto found_matching_interface;
> + break;
> + case CTYPE_ADDR:
1. In general, a name lookup is done by __dev_get_by_name /
dev_get_by_name_rcu / dev_get_by_name based on locking. rtnl is not held
in the datapath. Depending on need, you can hold the rcu lock
(rcu_read_lock) and use dev_get_by_name_rcu but you need to make sure
all references to the dev are used before calling rcu_read_unlock.
2. Similarly, lookup by index is done using __dev_get_by_index /
dev_get_by_index_rcu / dev_get_by_index.
3. Address to device lookup is done using something like __ip_dev_find
(IPv4) or ipv6_dev_find (IPv6) - again check the locking needs.
> + switch (ntohs(ctype3_hdr->afi)) {
> + /* IPV4 address */
> + case 1:
> + ifaddr = dev->ip_ptr->ifa_list;
> + while (ifaddr) {
> + if (memcmp(&ifaddr->ifa_address,
> + (ctype3_hdr + 1),
> + sizeof(ifaddr->ifa_address))
> == 0)
> + goto found_matching_interface;
> + ifaddr = ifaddr->ifa_next;
> + }
> + break;
> + /* IPV6 address */
> + case 2:
No magic numbers - if AFI enums do not exist, add them.
