Re: [PATCH] efinet: filter multicast traffic based on addresses

[Andrei Borzenkov <arvidjaar@gmail.com>]

17.11.2015 21:35, Josef Bacik пишет:
> We have some hardware that claims to support PROMISCUOUS_MULTICAST but doesn't
> actually work.  Instead utilize the multicast filters and specifically enable
> the multicast traffic we care about.  In reality we only care about ipv6
> multicast traffic but enable ipv4 multicast as well just in case.  Whenever we
> add a new address to the card we calculate the solicited node multicast address
> to the multicast filter.  With this patch my broken hardware is still broken but
> functional.  Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/drivers/efi/efinet.c | 84 ++++++++++++++++++++++++++++++++++----
>  grub-core/net/net.c                |  2 +
>  include/grub/net.h                 | 54 ++++++++++++------------
>  3 files changed, 105 insertions(+), 35 deletions(-)
> 
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index c8f80a1..bbbadd2 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -23,6 +23,7 @@
>  #include <grub/efi/api.h>
>  #include <grub/efi/efi.h>
>  #include <grub/i18n.h>
> +#include <grub/net/ip.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -183,8 +184,9 @@ open_card (struct grub_net_card *dev)
>  	 We need unicast and broadcast and additionaly all nodes and
>  	 solicited multicast for IPv6. Solicited multicast is per-IPv6
>  	 address and we currently do not have API to do it so simply

This comment becomes wrong now after your patch

> -	 try to enable receive of all multicast packets or evertyhing in
> -	 the worst case (i386 PXE driver always enables promiscuous too).
> +	 enable the all node addresses and the link local address.  We do this
> +	 because some firmware has been found to not do promiscuous multicast
> +	 mode properly.
>  
>  	 This does trust firmware to do what it claims to do.
>         */
> @@ -192,14 +194,25 @@ open_card (struct grub_net_card *dev)
>  	{
>  	  grub_uint32_t filters = GRUB_EFI_SIMPLE_NETWORK_RECEIVE_UNICAST   |
>  				  GRUB_EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
> -				  GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
> +				  GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST;
> +	  grub_efi_status_t st;
> +	  grub_efi_mac_address_t mac_filter[2] = {
> +		  { 0x1, 0, 0x5e, 0, 0, 1, },

Why this one? Where is it defined?

> +		  { 0x33, 0x33, 0, 0, 0, 1, },};
>  
>  	  filters &= net->mode->receive_filter_mask;
> -	  if (!(filters & GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST))
> -	    filters |= (net->mode->receive_filter_mask &
> -			GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
> -
> -	  efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
> +	  if (net->mode->max_mcast_filter_count < 2)
> +	    filters &= ~GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST;
> +
> +	  if (filters & GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST)
> +	    st = efi_call_6 (net->receive_filters, net, filters, 0, 0, 2,
> +			     mac_filter);
> +	  else
> +	    st = efi_call_6 (net->receive_filters, net, filters, 0, 0, 0,
> +			     NULL);

I think we still should attempt to fallback to promiscuous in this case.
It is better than giving up completely. Also grub_dprintf with current
filter mask would be good.

> +	  if (st != GRUB_EFI_SUCCESS)
> +	    grub_dprintf("efinet", "failed to set receive filters %u, %u\n",
> +			 (unsigned)filters, (unsigned)st);
>  	}
>  
>        efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> @@ -212,6 +225,58 @@ open_card (struct grub_net_card *dev)
>    return GRUB_ERR_NONE;
>  }
>  
> +/* We only need the lower 24 bits of the address, so just take the bottom part
> +   of the address and convert it over.
> + */
> +static void
> +solicited_node_mcast_addr_to_mac (grub_uint64_t addr,
> +				  grub_efi_mac_address_t mac)
> +{
> +  grub_uint64_t cpu_addr = grub_be_to_cpu64(addr);

Why cannot you simply copy last 3 bytes?

> +  int i, c = 0;
> +
> +  /* The format is 33:33:xx:xx:xx:xx, where xx is the last 32 bits of the
> +     multicast address.
> +
> +     The solicited node mcast addr is in the format ff02:0:0:0:0:1:ffxx:xxxx,
> +     where xx is the last 24 bits of the ipv6 address.
> +   */
> +  mac[0] = 0x33;
> +  mac[1] = 0x33;
> +  mac[2] = 0xff;
> +  for (i = 3; i < 6; i++, c++)
> +    mac[i] = (grub_uint8_t)((cpu_addr >> (16 - 8 * c)) & 0xff);
> +}
> +
> +static void
> +add_addr (struct grub_net_card *dev,
> +	  const grub_net_network_level_address_t *address)
> +{
> +  grub_efi_simple_network_t *net = dev->efi_net;
> +  grub_efi_mac_address_t mac_filters[16];
> +  grub_efi_status_t st;
> +  unsigned slot = net->mode->mcast_filter_count;
> +
> +  /* We don't need to add anything for ipv4 addresses. */
> +  if (address->type != GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6)
> +    return;
> +
> +  if ((slot >= net->mode->max_mcast_filter_count)
> +      || !(GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST &
> +	   net->mode->receive_filter_mask))
> +    return;
> +
> +  grub_memcpy(mac_filters, net->mode->mcast_filter,
> +	      sizeof (grub_efi_mac_address_t) * slot);
> +  solicited_node_mcast_addr_to_mac (address->ipv6[1], mac_filters[slot++]);
> +  st = efi_call_6 (net->receive_filters, net,
> +		   GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST, 0, 0, slot,
> +		   mac_filters);

What about overlapping addresses (see also below)?

> +  if (st != GRUB_EFI_SUCCESS)
> +    grub_dprintf("efinet", "failed to add new receive filter %u\n",
> +		 (unsigned)st);
> +}
> +
>  static void
>  close_card (struct grub_net_card *dev)
>  {
> @@ -228,7 +293,8 @@ static struct grub_net_card_driver efidriver =
>      .open = open_card,
>      .close = close_card,
>      .send = send_card_buffer,
> -    .recv = get_card_packet
> +    .recv = get_card_packet,
> +    .add_addr = add_addr,

Well ... if you add function to add filter, you should also add function
to remove filter when address is removed. And here it becomes
complicated; mapping is not 1-to-1 so we need to reference count used
multicast addresses.

>    };
>  
>  grub_efi_handle_t
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 65bea28..c4d2484 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -252,6 +252,8 @@ grub_net_add_addr_real (char *name,
>    inter->dhcp_ack = NULL;
>    inter->dhcp_acklen = 0;
>  
> +  if (card->driver->add_addr)
> +    card->driver->add_addr(card, addr);
>    grub_net_network_level_interface_register (inter);
>  
>    return inter;
> diff --git a/include/grub/net.h b/include/grub/net.h
> index a1ff519..885f0be 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -67,6 +67,32 @@ typedef enum grub_net_card_flags
>      GRUB_NET_CARD_NO_MANUAL_INTERFACES = 2
>    } grub_net_card_flags_t;
>  
> +typedef enum grub_network_level_protocol_id 
> +{
> +  GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV,
> +  GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4,
> +  GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6
> +} grub_network_level_protocol_id_t;
> +
> +typedef enum
> +{
> +  DNS_OPTION_IPV4,
> +  DNS_OPTION_IPV6,
> +  DNS_OPTION_PREFER_IPV4,
> +  DNS_OPTION_PREFER_IPV6
> +} grub_dns_option_t;
> +
> +typedef struct grub_net_network_level_address
> +{
> +  grub_network_level_protocol_id_t type;
> +  union
> +  {
> +    grub_uint32_t ipv4;
> +    grub_uint64_t ipv6[2];
> +  };
> +  grub_dns_option_t option;
> +} grub_net_network_level_address_t;
> +
>  struct grub_net_card;
>  
>  struct grub_net_card_driver
> @@ -79,6 +105,8 @@ struct grub_net_card_driver
>    grub_err_t (*send) (struct grub_net_card *dev,
>  		      struct grub_net_buff *buf);
>    struct grub_net_buff * (*recv) (struct grub_net_card *dev);
> +  void (*add_addr) (struct grub_net_card *dev,
> +		    const grub_net_network_level_address_t *address);
>  };
>  
>  typedef struct grub_net_packet
> @@ -150,32 +178,6 @@ struct grub_net_card
>  
>  struct grub_net_network_level_interface;
>  
> -typedef enum grub_network_level_protocol_id 
> -{
> -  GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV,
> -  GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4,
> -  GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6
> -} grub_network_level_protocol_id_t;
> -
> -typedef enum
> -{
> -  DNS_OPTION_IPV4,
> -  DNS_OPTION_IPV6,
> -  DNS_OPTION_PREFER_IPV4,
> -  DNS_OPTION_PREFER_IPV6
> -} grub_dns_option_t;
> -
> -typedef struct grub_net_network_level_address
> -{
> -  grub_network_level_protocol_id_t type;
> -  union
> -  {
> -    grub_uint32_t ipv4;
> -    grub_uint64_t ipv6[2];
> -  };
> -  grub_dns_option_t option;
> -} grub_net_network_level_address_t;
> -
>  typedef struct grub_net_network_level_netaddress
>  {
>    grub_network_level_protocol_id_t type;
>