Re: [PATCH net-next 6/6] icmp: add response to RFC 8335 PROBE messages

[David Ahern <dsahern@gmail.com>]

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.