Received-SPF: pass (google.com: domain of linux-kernel+bounces-84570-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 27 Feb 2024 22:43:39 -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: <20240228064339.GA10916@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>
 <20240105054153.GA18258@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
 <CAK3XEhNRuFq=x22q011uthNdrzWzTeFzzEoNX0ZjWfcivfhyCw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAK3XEhNRuFq=x22q011uthNdrzWzTeFzzEoNX0ZjWfcivfhyCw@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

Hi Ani,

This patch should be out for review by mid next week.

Thanks and Regards,
Shradha.
On Sat, Feb 24, 2024 at 09:48:17PM +0530, Ani Sinha wrote:
> On Fri, 5 Jan, 2024, 11:12 am Shradha Gupta, <
> shradhagupta@linux.microsoft.com> wrote:
> 
> > 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)
> >
> 
> Has there been any progress on this front?
> 
> 
> >