Received-SPF: pass (google.com: domain of linux-kernel+bounces-17488-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 4 Jan 2024 21:41:53 -0800
From: Shradha Gupta <shradhagupta@linux.microsoft.com>
To: Ani Sinha <anisinha@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
	Long Li <longli@microsoft.com>, Olaf Hering <olaf@aepfle.de>,
	Shradha Gupta <shradhagupta@microsoft.com>
Subject: Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection
 profile
Message-ID: <20240105054153.GA18258@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
References: <1696847920-31125-1-git-send-email-shradhagupta@linux.microsoft.com>
 <A8905E07-2D01-46D6-A40D-C9F7461393EB@redhat.com>
 <CAK3XEhMzLvxTbP+sFjD7btfdjw5uxyAccdT09d4hcw5hkCKXHQ@mail.gmail.com>
 <CAK3XEhOn+k7U88sMF2tXGSyhYvAL9u17sw6qvomL=oYESRMQkw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAK3XEhOn+k7U88sMF2tXGSyhYvAL9u17sw6qvomL=oYESRMQkw@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

On Sat, Dec 23, 2023 at 01:35:26PM +0530, Ani Sinha wrote:
> On Sat, Dec 23, 2023 at 12:43???PM Ani Sinha <anisinha@redhat.com> wrote:
> >
> > On Fri, Oct 13, 2023 at 3:06???PM Ani Sinha <anisinha@redhat.com> wrote:
> > >
> > >
> > >
> > > > On 09-Oct-2023, at 4:08 PM, Shradha Gupta <shradhagupta@linux.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.
Thanks for bringing this up Ani. I will try to fix this in a new patch
(would get the new testcases added in our suite as well)