[PATCH] bootp: use the ipv* version given by the dhcp server

[PATCH] bootp: use the ipv* version given by the dhcp server

[Josef Bacik]

We were setting DNS_OPTION_PREVER_IPV* based on which version of the dhcp
request we go back.  However this would mean that we would mistakenly fall back
on ipv4 in an ipv6 only system, or vice versa.  For the sake of simplicity lets
only use the ip version for the dns server we got in our dhcp response.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/bootp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 345ad7a..cde818c 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -95,7 +95,7 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
 		struct grub_net_network_level_address s;
 		s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
 		s.ipv4 = grub_get_unaligned32 (ptr);
-		s.option = DNS_OPTION_PREFER_IPV4;
+		s.option = DNS_OPTION_IPV4;
 		grub_net_add_dns_server (&s);
 		ptr += 4;
 	      }
@@ -492,7 +492,7 @@ get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet,
       (la + i)->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
       (la + i)->ipv6[0] = grub_get_unaligned64 (pa);
       (la + i)->ipv6[1] = grub_get_unaligned64 (pa + 8);
-      (la + i)->option = DNS_OPTION_PREFER_IPV6;
+      (la + i)->option = DNS_OPTION_IPV6;
     }
 
   *addr = la;
-- 
2.5.0

Re: [PATCH] bootp: use the ipv* version given by the dhcp server

[Andrei Borzenkov]

24.02.2016 22:10, Josef Bacik пишет:
> We were setting DNS_OPTION_PREVER_IPV* based on which version of the dhcp
> request we go back.  However this would mean that we would mistakenly fall back
> on ipv4 in an ipv6 only system, or vice versa.  For the sake of simplicity lets
> only use the ip version for the dns server we got in our dhcp response.  Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/bootp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index 345ad7a..cde818c 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -95,7 +95,7 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
>  		struct grub_net_network_level_address s;
>  		s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
>  		s.ipv4 = grub_get_unaligned32 (ptr);
> -		s.option = DNS_OPTION_PREFER_IPV4;
> +		s.option = DNS_OPTION_IPV4;
>  		grub_net_add_dns_server (&s);
>  		ptr += 4;
>  	      }
> @@ -492,7 +492,7 @@ get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet,
>        (la + i)->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>        (la + i)->ipv6[0] = grub_get_unaligned64 (pa);
>        (la + i)->ipv6[1] = grub_get_unaligned64 (pa + 8);
> -      (la + i)->option = DNS_OPTION_PREFER_IPV6;
> +      (la + i)->option = DNS_OPTION_IPV6;
>      }
>  
>    *addr = la;
> 

that's pretty radical behavior change. It may break existing
configuration. As the only justification is side stepping issues in
current DNS implementation I think we should rather fix implementation.

And even if it does not break things knowing that host has "other" IP
and additional interface may be needed is IMHO more useful than getting
confusing "host does not exist".

Re: [PATCH] bootp: use the ipv* version given by the dhcp server

[Josef Bacik]

On 02/27/2016 01:05 PM, Andrei Borzenkov wrote:
> 24.02.2016 22:10, Josef Bacik пишет:
>> We were setting DNS_OPTION_PREVER_IPV* based on which version of the dhcp
>> request we go back.  However this would mean that we would mistakenly fall back
>> on ipv4 in an ipv6 only system, or vice versa.  For the sake of simplicity lets
>> only use the ip version for the dns server we got in our dhcp response.  Thanks,
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>>   grub-core/net/bootp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>> index 345ad7a..cde818c 100644
>> --- a/grub-core/net/bootp.c
>> +++ b/grub-core/net/bootp.c
>> @@ -95,7 +95,7 @@ parse_dhcp_vendor (const char *name, const void *vend, int limit, int *mask)
>>   		struct grub_net_network_level_address s;
>>   		s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
>>   		s.ipv4 = grub_get_unaligned32 (ptr);
>> -		s.option = DNS_OPTION_PREFER_IPV4;
>> +		s.option = DNS_OPTION_IPV4;
>>   		grub_net_add_dns_server (&s);
>>   		ptr += 4;
>>   	      }
>> @@ -492,7 +492,7 @@ get_dhcpv6_dns_address (const struct grub_net_dhcpv6_packet *packet,
>>         (la + i)->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>>         (la + i)->ipv6[0] = grub_get_unaligned64 (pa);
>>         (la + i)->ipv6[1] = grub_get_unaligned64 (pa + 8);
>> -      (la + i)->option = DNS_OPTION_PREFER_IPV6;
>> +      (la + i)->option = DNS_OPTION_IPV6;
>>       }
>>
>>     *addr = la;
>>
>
> that's pretty radical behavior change. It may break existing
> configuration. As the only justification is side stepping issues in
> current DNS implementation I think we should rather fix implementation.
>

No, the justification is that if you have ipv6 only systems 
"PREFER_IPV6" isn't the appropriate behavior.  The fact is you are only 
going to get ipv6 dns servers from dhcpv6 responses and same for ipv4, 
you don't want to "prefer", you want to bind it to the response you get 
to avoid trying an incompatible ip version that you can't talk to.

> And even if it does not break things knowing that host has "other" IP
> and additional interface may be needed is IMHO more useful than getting
> confusing "host does not exist".
>

The end result is the same, you can't talk to it.  This is the same 
behavior you get when you try to ping ipv6.google.com from a non-ipv6 
system, it'll say it can't find the host, which is true, there is no way 
we can talk to it so it can't see it.  This isn't going to break 
existing setups at all, it just removes unexpected behavior in mixed and 
ipv6 only environments.  Thanks,

Josef

Re: [PATCH] bootp: use the ipv* version given by the dhcp server

[Andrei Borzenkov]

01.03.2016 19:34, Josef Bacik пишет:
> On 02/27/2016 01:05 PM, Andrei Borzenkov wrote:
>> 24.02.2016 22:10, Josef Bacik пишет:
>>> We were setting DNS_OPTION_PREVER_IPV* based on which version of the
>>> dhcp
>>> request we go back.  However this would mean that we would mistakenly
>>> fall back
>>> on ipv4 in an ipv6 only system, or vice versa.  For the sake of
>>> simplicity lets
>>> only use the ip version for the dns server we got in our dhcp
>>> response.  Thanks,
>>>
>>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>>> ---
>>>   grub-core/net/bootp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>>> index 345ad7a..cde818c 100644
>>> --- a/grub-core/net/bootp.c
>>> +++ b/grub-core/net/bootp.c
>>> @@ -95,7 +95,7 @@ parse_dhcp_vendor (const char *name, const void
>>> *vend, int limit, int *mask)
>>>           struct grub_net_network_level_address s;
>>>           s.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
>>>           s.ipv4 = grub_get_unaligned32 (ptr);
>>> -        s.option = DNS_OPTION_PREFER_IPV4;
>>> +        s.option = DNS_OPTION_IPV4;
>>>           grub_net_add_dns_server (&s);
>>>           ptr += 4;
>>>             }
>>> @@ -492,7 +492,7 @@ get_dhcpv6_dns_address (const struct
>>> grub_net_dhcpv6_packet *packet,
>>>         (la + i)->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>>>         (la + i)->ipv6[0] = grub_get_unaligned64 (pa);
>>>         (la + i)->ipv6[1] = grub_get_unaligned64 (pa + 8);
>>> -      (la + i)->option = DNS_OPTION_PREFER_IPV6;
>>> +      (la + i)->option = DNS_OPTION_IPV6;
>>>       }
>>>
>>>     *addr = la;
>>>
>>
>> that's pretty radical behavior change. It may break existing
>> configuration. As the only justification is side stepping issues in
>> current DNS implementation I think we should rather fix implementation.
>>
> 
> No, the justification is that if you have ipv6 only systems
> "PREFER_IPV6" isn't the appropriate behavior.  The fact is you are only
> going to get ipv6 dns servers from dhcpv6 responses and same for ipv4,
> you don't want to "prefer", you want to bind it to the response you get
> to avoid trying an incompatible ip version that you can't talk to.
> 

How do you know it is ipv6 only system? People are quite creative in
grub.cfg and the fact that firmware boots over IPv6 does not mean
grub.cfg cannot call net_bootp.

>> And even if it does not break things knowing that host has "other" IP
>> and additional interface may be needed is IMHO more useful than getting
>> confusing "host does not exist".
>>
> 
> The end result is the same, you can't talk to it.  This is the same
> behavior you get when you try to ping ipv6.google.com from a non-ipv6
> system, it'll say it can't find the host, which is true,

bor@bor-Latitude-E5450:~/obs/home:arvidjaar:grub2-next/grub2$ ping6
ipv6.google.com
connect: Network is unreachable

ping is IPv4 only program so it asks for A only.

> there is no way
> we can talk to it so it can't see it.  This isn't going to break
> existing setups at all,

The question is not, whether it does not break something, but what does
it fix? The bug is that we get IPv4 address when we asked for IPv6 if
available. *That* is real bug and *that* should be fixed. This patch
just hides it, sorry.

> it just removes unexpected behavior in mixed and
> ipv6 only environments.

Well ... filtering DNS replies based on which interfaces are configured
looks sensible; but that is topic for post-2.02.