Message-ID: <CAK3XEhOn+k7U88sMF2tXGSyhYvAL9u17sw6qvomL=oYESRMQkw@mail.gmail.com>
Date: Sat, 23 Dec 2023 13:35:26 +0530
From: Ani Sinha <anisinha@...hat.com>
To: Shradha Gupta <shradhagupta@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org, 
	"K. Y. Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, 
	Dexuan Cui <decui@...rosoft.com>, Long Li <longli@...rosoft.com>, Olaf Hering <olaf@...fle.de>, 
	Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection profile

On Sat, Dec 23, 2023 at 12:43 PM Ani Sinha <anisinha@...hat.com> wrote:
>
> On Fri, Oct 13, 2023 at 3:06 PM Ani Sinha <anisinha@...hat.com> wrote:
> >
> >
> >
> > > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <shradhagupta@...ux.microsoft.com> wrote:
> > >
> > > Ifcfg config file support in NetworkManger is deprecated. This patch
> > > provides support for the new keyfile config format for connection
> > > profiles in NetworkManager. The patch modifies the hv_kvp_daemon code
> > > to generate the new network configuration in keyfile
> > > format(.ini-style format) along with a ifcfg format configuration.
> > > The ifcfg format configuration is also retained to support easy
> > > backward compatibility for distro vendors. These configurations are
> > > stored in temp files which are further translated using the
> > > hv_set_ifconfig.sh script. This script is implemented by individual
> > > distros based on the network management commands supported.
> > > For example, RHEL's implementation could be found here:
> > > https://gitlab.com/redhat/centos-stream/src/hyperv-daemons/-/blob/c9s/hv_set_ifconfig.sh
> > > Debian's implementation could be found here:
> > > https://github.com/endlessm/linux/blob/master/debian/cloud-tools/hv_set_ifconfig
> > >
> > > The next part of this support is to let the Distro vendors consume
> > > these modified implementations to the new configuration format.
> > >
> > > Tested-on: Rhel9(Hyper-V, Azure)(nm and ifcfg files verified)
> >
> > Was this patch tested with ipv6? We are seeing a mix of ipv6 and ipv4 addresses in ipv6 section:
>
> There is also another issue which is kind of a design problem that
> existed from the get go but now is exposed since keyfile support was
> added.
> Imagine we configure both ipv6 and ipv4 and some interfaces have ipv4
> addresses and some have ipv6.
> getifaddres() call in kvp_get_ip_info() will return a linked list per
> interface. The code sets ip_buffer->addr_family based on the address
> family of the address set for the interface. We use this to determine
> which section in the keyfile to use, ipv6 or ipv4. However, once we
> make this decision, we are locked in. The issue here is that
> kvp_process_ip_address() that extracts the IP addresses concatenate
> the addresses in a single buffer separating the IPs with ";". Thus
> across interfaces, the buffer can contain both ipv4 and ipv6 addresses
> separated by ";" if both v4 and v6 are configured. This is problematic
> as the addr_family can be either ipv4 or ipv6 but not both.
> Essentially, we can have a situation that for a single addr_family in
> hv_kvp_ipaddr_value struct, the ip_addr member can be a buffer
> containing both ipv6 and ipv4 addresses. Notice that
> process_ip_string() handles this by iterating through the string and
> for each ip extracted, it individually determines if the IP is a v6 or
> a v4 and adds "IPV6ADDR" or "IPADDR" to the ifcfg file accordingly.
> process_ip_string_nm() does not do that and solely makes the
> determination based on is_ipv6 values which is based on a single
> addr_family value above. Thus, it cannot possibly know whether the
> specific IP address extracted from the string is a v4 or v6. Unlike
> for ifcfg files, fir nm keyfiles, we need to add v4 and v6 addresses
> in specific sections and we cannot mix the two. So we need to make two
> passes. One for v4 and one for v6 and then add IPs in the respective
> sections.
>
> This issue needs to be looked into and unless it's resolved, we cannot
> support both ipv4 and ipv6 addresses at the same time.

In the short term, we should probably do this to avoid the mismatch :

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 318e2dad27e0..b77c8edfe663 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -1216,6 +1216,9 @@ static int process_ip_string_nm(FILE *f, char
*ip_string, char *subnet,
                                                       subnet_addr,
                                                       (MAX_IP_ADDR_SIZE *
                                                        2))) {
+               if (is_ipv6 == is_ipv4((char*) addr))
+                   continue;
+
                if (!is_ipv6)
                        plen = kvp_subnet_to_plen((char *)subnet_addr);
                else

But this is really a short term hack.

Powered by blists - more mailing lists