From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1a34gO-0004aO-2c for mharc-grub-devel@gnu.org; Sun, 29 Nov 2015 11:18:36 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a34gK-0004Yp-Lz for grub-devel@gnu.org; Sun, 29 Nov 2015 11:18:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a34gG-0000rj-KP for grub-devel@gnu.org; Sun, 29 Nov 2015 11:18:32 -0500 Received: from mail-lf0-x22b.google.com ([2a00:1450:4010:c07::22b]:35205) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a34gG-0000rK-9U for grub-devel@gnu.org; Sun, 29 Nov 2015 11:18:28 -0500 Received: by lfdl133 with SMTP id l133so170452619lfd.2 for ; Sun, 29 Nov 2015 08:18:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type:content-transfer-encoding; bh=3opqiNmBU3+NxGZs0YZel56defzSWtC9htDeT5vVCI4=; b=Bhxh7cB/zNQAxiVf3cMpf6EY66xIOruddDGgQ0XKqAHUc3hHUvlWsq/D3asM3l82ba tWD7XXrDQVcSt9sspnBV+YTKj7JrnxM++GcEdm7iX1VisY5QkLtaIow5b1PyR8fa1vEI thgKWgnHVgtNRlclOL85JRvXPJsqlZaibaV41DvdLdcaSH0UbRXzZo27Ij2GcMTwniBt 9csI68n5nr/d3KuTdka9UviEZAYi+slAAJL8CB9fbTjsc1iIids6NEJ6Ul/oRp3prTA4 eml9rQ+YPwF1NrH99qXdZBEjnCJsnf+ns4E1XsVdMji85IhMQbiX1jgoISFypenQNu+Z +rxw== X-Received: by 10.112.200.163 with SMTP id jt3mr5489639lbc.68.1448813907175; Sun, 29 Nov 2015 08:18:27 -0800 (PST) Received: from [192.168.1.41] (ppp91-76-25-247.pppoe.mtu-net.ru. [91.76.25.247]) by smtp.gmail.com with ESMTPSA id m64sm526405lfd.45.2015.11.29.08.18.25 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 Nov 2015 08:18:26 -0800 (PST) Subject: Re: [PATCH] efinet: filter multicast traffic based on addresses To: The development of GNU GRUB , kernel-team@fb.com References: <1447785313-1383440-1-git-send-email-jbacik@fb.com> <565AA2ED.1010007@gmail.com> From: Andrei Borzenkov Message-ID: <565B2550.1020303@gmail.com> Date: Sun, 29 Nov 2015 19:18:24 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <565AA2ED.1010007@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c07::22b Cc: Josef Bacik X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 29 Nov 2015 16:18:34 -0000 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 >> --- >> 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 >> #include >> #include >> +#include >> >> 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; >> >