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

[Andrei Borzenkov]

29.11.2015 10:02, Andrei Borzenkov пишет:
> 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 <address@hidden>
>> ---
>>  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)

Should not we fall back to promiscuous in this case as the last resort?

>> +      || !(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.
> 

Actually I think it should simply build filter completely every time
from addresses (interfaces) on this card. This automatically gives us
duplicates elimination as well as correct handling of removed 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;
>>
>