Received-SPF: pass (google.com: domain of linux-kernel+bounces-10331-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <1696847920-31125-1-git-send-email-shradhagupta@linux.microsoft.com>
 <A8905E07-2D01-46D6-A40D-C9F7461393EB@redhat.com>
In-Reply-To: <A8905E07-2D01-46D6-A40D-C9F7461393EB@redhat.com>
From: Ani Sinha <anisinha@redhat.com>
Date: Sat, 23 Dec 2023 12:43:28 +0530
Message-ID: <CAK3XEhMzLvxTbP+sFjD7btfdjw5uxyAccdT09d4hcw5hkCKXHQ@mail.gmail.com>
Subject: Re: [PATCH v8] hv/hv_kvp_daemon:Support for keyfile based connection profile
To: Shradha Gupta <shradhagupta@linux.microsoft.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>, 
	Michael Kelley <mikelley@microsoft.com>, Olaf Hering <olaf@aepfle.de>, 
	Shradha Gupta <shradhagupta@microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Oct 13, 2023 at 3:06=E2=80=AFPM Ani Sinha <anisinha@redhat.com> wro=
te:
>
>
>
> > 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/h=
v_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 add=
resses 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.

Thoughts?