Re: [PATCH] efinet: get bootstrap info from proxy offer packet

[chen zhihui]

On 04/21/2016 09:02 PM, Andrei Borzenkov wrote:
> On Thu, Apr 21, 2016 at 11:36 AM, chen zhihui <address@hidden> wrote:
>> From: chenzhihui <address@hidden>
>>
>> Bootstrap server ip address and boot file name maybe come from
>> a separate proxy DHCP server, check the proxy_offer packet if
>> failed with dhcp_ack packet.
>>
> Yes, this came up before. Thank you for a patch. This is likely
> post-2.02 material though.
Thanks for your comments!
>
>> Signed-off-by: chenzhihui <address@hidden>
>> Tested-by: Jerome Forissier <address@hidden>
>> ---
>>   grub-core/net/bootp.c              | 170 
>> ++++++++++++++++++++++++++++++++++++-
>>   grub-core/net/drivers/efi/efinet.c |  23 ++++-
>>   include/grub/net.h                 |  10 +++
>>   3 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>> index 4fdeac3..52f4051 100644
>> --- a/grub-core/net/bootp.c
>> +++ b/grub-core/net/bootp.c
>> @@ -186,7 +186,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>>       }
>>   #endif
>>
>> -  if (size > OFFSET_OF (boot_file, bp))
>> +  if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0])
>>       grub_env_set_net_property (name, "boot_file", bp->boot_file,
>>                                  sizeof (bp->boot_file));
>>     if (is_def)
>> @@ -233,7 +233,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
>>          }
>>       }
>>
>> -  if (size > OFFSET_OF (boot_file, bp) && path)
>> +  if (size > OFFSET_OF (boot_file, bp) && bp->boot_file[0] && path)
>>       {
>>         *path = grub_strndup (bp->boot_file, sizeof (bp->boot_file));
>>         grub_print_error ();
>> @@ -263,6 +263,172 @@ grub_net_configure_by_dhcp_ack (const char *name,
>>     return inter;
>>   }
>>
>> +struct dhcp4_packet_option {
>> +       grub_uint8_t code;
>> +       grub_uint8_t length;
>> +       grub_uint8_t data[0];
>> +};
>> +
>> +/*
>> + * Get specified option from DHCP extension data
>> + *
>> + * from PxeBcDhcp.c of UEFI
>> + *
>> + */
>> +static struct dhcp4_packet_option *
>> +dhcp_proxy_extension_option (const grub_uint8_t *buf,
>> +               grub_size_t size,
>> +               grub_uint8_t code)
>> +{
>> +       struct dhcp4_packet_option *option = (struct dhcp4_packet_option 
>> *)buf;
>> +       grub_size_t offset = 0;
>> +
>> +       while (offset < size && option->code != GRUB_NET_BOOTP_END) {
> OK, could you please rebase it on top of
> http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00280.html. Of
> course comments to this patch are welcome. Without comments it will
> land here at some point and it implements DHCP options processing
> already. If you find it easier, I can create branch with this patch.
No thanks, maybe I'll send v2 patch when your patch is accepted by upstream.
> ...
>> +void
>> +grub_net_configure_by_proxy_offer (const struct grub_net_bootp_packet *bp,
>> +                               grub_size_t size,
>> +                               char **device,
>> +                               char **path)
>> +{
>> +       const grub_uint8_t *buf = bp->vendor + sizeof (grub_uint32_t);
>> +       grub_uint32_t option_size =
>> +               size - OFFSET_OF(vendor, bp) - sizeof (grub_uint32_t);
>> +       struct dhcp4_packet_option *option;
>> +
>> +       if (device == NULL)
>> +               return;
>> +
>> +       if (!proxy_offer_is_valid(bp, size))
>> +               return;
>> +
>> +       if (!*device && bp->server_ip)
>> +       {
>> +               *device = grub_xasprintf ("tftp,%d.%d.%d.%d",
>> +                               ((grub_uint8_t *) &bp->server_ip)[0],
>> +                               ((grub_uint8_t *) &bp->server_ip)[1],
>> +                               ((grub_uint8_t *) &bp->server_ip)[2],
>> +                               ((grub_uint8_t *) &bp->server_ip)[3]);
>> +               grub_print_error ();
>> +       }
>> +
>> +       option = dhcp_proxy_extension_option(buf,
>> +                       option_size, GRUB_NET_DHCP_OVERLOAD);
>> +
>> +       if ((option == NULL || option->data[0] == 1) && !*device && 
>> bp->server_name[0])
>> +       {
>> +               *device = grub_xasprintf ("tftp,%s", bp->server_name);
>> +               grub_print_error ();
>> +       }
>> +
>> +       if (!*device)
>> +       {
>> +               option = dhcp_proxy_extension_option(buf,
>> +                               option_size, GRUB_NET_DHCP_SERVER_ID);
>> +
>> +               if (option) {
>> +                       *device = grub_xasprintf("tftp,%d.%d.%d.%d",
>> +                                       option->data[0],
>> +                                       option->data[1],
>> +                                       option->data[2],
>> +                                       option->data[3]);
> DHCP_SERVER_ID (option 54) is defined for DHCP client/server
> communication only. There is no implied usage of this option as next
> server (i.e. boot server) so I do not think this is correct.
Yes, I'll remove this in v2.
>
> ...
>> diff --git a/grub-core/net/drivers/efi/efinet.c 
>> b/grub-core/net/drivers/efi/efinet.c
>> index 5388f95..ef0ccd9 100644
>> --- a/grub-core/net/drivers/efi/efinet.c
>> +++ b/grub-core/net/drivers/efi/efinet.c
>> @@ -338,6 +338,7 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char 
>> **device,
>>     FOR_NET_CARDS (card)
>>     {
>>       grub_efi_device_path_t *cdp;
>> +       struct grub_net_network_level_interface *inter;
>>       struct grub_efi_pxe *pxe;
>>       struct grub_efi_pxe_mode *pxe_mode;
>>       if (card->driver != &efidriver)
>> @@ -378,11 +379,31 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char 
>> **device,
>>       if (! pxe)
>>         continue;
>>       pxe_mode = pxe->mode;
>> -    grub_net_configure_by_dhcp_ack (card->name, card, 0,
>> +    inter = grub_net_configure_by_dhcp_ack (card->name, card, 0,
>>                                      (struct grub_net_bootp_packet *)
>>                                      &pxe_mode->dhcp_ack,
>>                                      sizeof (pxe_mode->dhcp_ack),
>>                                      1, device, path);
>> +
>> +       /*
>> +        * Bootstrap server ip address and file name maybe
>> +        * come from a separate proxy DHCP server,
>> +        * so check the proxy_offer DHCP packet
>> +        *
>> +        */
>> +       if (inter && *path == NULL) {
>> +               if (*device) {
>> +                       grub_free(*device);
>> +                       *device = NULL;
>> +               }
>> +
>> +               grub_net_configure_by_proxy_offer(
>> +                               (struct grub_net_bootp_packet 
>> *)&pxe_mode->proxy_offer,
> Please check ProxyOfferReceived as required by UEFI. I know we do not
> do it currently but let's do it right at least in new code.
You're right, I'll change in v2
>
>> +                               sizeof (pxe_mode->proxy_offer),
>> +                               device,
>> +                               path);
>> +       }
>> +
> Well, this opens up the question about precedence of data from
> different packets. With my patch (that abstracts away options
> processing) we can actually simply pass both packets at once and fetch
> IP information from (original) DHCPACK and other information from
> proxy DHCPOFFER as required (falling back to DHCPACK if proxy is
> NULL).
Great! If you can do that in your v3 patch, I'll has nothing to do.
>>       return;
>>     }
>>   }
>> diff --git a/include/grub/net.h b/include/grub/net.h
>> index b62643a..f107a23 100644
>> --- a/include/grub/net.h
>> +++ b/include/grub/net.h
>> @@ -433,6 +433,10 @@ enum
>>       GRUB_NET_BOOTP_DOMAIN = 0x0f,
>>       GRUB_NET_BOOTP_ROOT_PATH = 0x11,
>>       GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
>> +       GRUB_NET_DHCP_OVERLOAD = 0x34,
>> +       GRUB_NET_DHCP_SERVER_ID = 0x36,
>> +       GRUB_NET_DHCP_CLASS_ID = 0x3c,
>> +       GRUB_NET_DHCP_BOOTFILE = 0x43,
> Please use decimal option numbers for any new option you add. They are
> defined as decimal in RFC and it makes it easier to cross-reference
> with RFC later.

OK.

-- 
==========================================
Best Regards

陈志辉(Chenzhihui)

华为海思图灵软件架构组
Huawei Hisilicon Turing Software Architecture Team
http://www.hisilicon.com/