Received-SPF: pass (google.com: domain of linux-kernel+bounces-105698-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.400.31\))
Subject: Re: [PATCH v3] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format
From: Ani Sinha <anisinha@redhat.com>
In-Reply-To: <85394C37-16E1-4AA1-82D4-DEB3D58920A7@redhat.com>
Date: Mon, 18 Mar 2024 10:26:49 +0530
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <56EA1869-5EFA-4515-AC39-965241C373BF@redhat.com>
References: <1710729951-2695-1-git-send-email-shradhagupta@linux.microsoft.com>
 <85394C37-16E1-4AA1-82D4-DEB3D58920A7@redhat.com>
To: Shradha Gupta <shradhagupta@linux.microsoft.com>


> On 18-Mar-2024, at 09:51, Ani Sinha <anisinha@redhat.com> wrote:
>=20
>=20
>=20
>> On 18-Mar-2024, at 08:15, Shradha Gupta =
<shradhagupta@linux.microsoft.com> wrote:
>>=20
>> If the network configuration strings are passed as a combination of =
IPv and
>> IPv6 addresses, the current KVP daemon doesnot handle it for the =
keyfile
>> configuration format.
>> With these changes, the keyfile config generation logic scans through =
the
>> list twice to generate IPv4 and IPv6 sections for the configuration =
files
>> to handle this support.
>>=20
>> Testcases ran:Rhel 9, Hyper-V VMs
>>             (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
>> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
>> ---
>> Changes in v3
>> * Introduced a macro for the output string size
>> * Added cound checks and used strncpy instead of strncpy
>> * Rearranged code to reduce total lines of code
>> ---
>> tools/hv/hv_kvp_daemon.c | 177 =
++++++++++++++++++++++++++++++---------
>> 1 file changed, 136 insertions(+), 41 deletions(-)
>>=20
>> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
>> index 318e2dad27e0..156cef99d361 100644
>> --- a/tools/hv/hv_kvp_daemon.c
>> +++ b/tools/hv/hv_kvp_daemon.c
>> @@ -76,6 +76,12 @@ enum {
>> DNS
>> };
>>=20
>> +enum {
>> + IPV4 =3D 1,
>> + IPV6,
>> + IP_TYPE_MAX
>> +};
>> +
>> static int in_hand_shake;
>>=20
>> static char *os_name =3D "";
>> @@ -102,6 +108,11 @@ static struct utsname uts_buf;
>>=20
>> #define MAX_FILE_NAME 100
>> #define ENTRIES_PER_BLOCK 50
>> +/*
>> + * Change this entry if the number of addresses increases in future
>> + */
>> +#define MAX_IP_ENTRIES 64
>> +#define OUTSTR_BUF_SIZE ((INET6_ADDRSTRLEN + 1) * MAX_IP_ENTRIES)
>>=20
>> struct kvp_record {
>> char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
>> @@ -1171,6 +1182,18 @@ static int process_ip_string(FILE *f, char =
*ip_string, int type)
>> return 0;
>> }
>>=20
>> +int ip_version_check(const char *input_addr)
>> +{
>> + struct in6_addr addr;
>> +
>> + if (inet_pton(AF_INET, input_addr, &addr))
>> + return IPV4;
>> + else if (inet_pton(AF_INET6, input_addr, &addr))
>> + return IPV6;
>> +
>> + return -EINVAL;
>> +}
>> +
>> /*
>> * Only IPv4 subnet strings needs to be converted to plen
>> * For IPv6 the subnet is already privided in plen format
>> @@ -1197,14 +1220,71 @@ static int kvp_subnet_to_plen(char =
*subnet_addr_str)
>> return plen;
>> }
>>=20
>> +static int process_dns_gateway_nm(FILE *f, char *ip_string, int =
type,
>> +  int ip_sec)
>> +{
>> + char addr[INET6_ADDRSTRLEN], *output_str;
>> + int ip_offset =3D 0, error =3D 0, ip_ver;
>> + char *param_name;
>> +
>> + memset(addr, 0, sizeof(addr));
>=20
> ^^^^^^^^^^^^^^^^^^^^^^^^
> Is this still needed?
>=20
>> +
>> + if (type =3D=3D DNS)
>> + param_name =3D "dns";
>> + else if (type =3D=3D GATEWAY)
>> + param_name =3D "gateway";
>> + else
>> + return -EINVAL;
>> +
>> + output_str =3D (char *)calloc(OUTSTR_BUF_SIZE, sizeof(char));
>> + if (!output_str)
>> + return -ENOMEM;
>> +
>> + while (1) {
>> + memset(addr, 0, sizeof(addr));
>> +
>> + if (!parse_ip_val_buffer(ip_string, &ip_offset, addr,
>> + (MAX_IP_ADDR_SIZE * 2)))
>> + break;
>> +
>> + ip_ver =3D ip_version_check(addr);
>> + if (ip_ver < 0)
>> + continue;
>> +
>> + if ((ip_ver =3D=3D IPV4 && ip_sec =3D=3D IPV4) ||
>> +    (ip_ver =3D=3D IPV6 && ip_sec =3D=3D IPV6)) {
>> + /*
>> + * do a bound check to avoid out-of bound writes
>> + */
>> + if ((OUTSTR_BUF_SIZE - strlen(output_str)) >
>> +    (strlen(addr) + 1)) {
>> + strncat(output_str, addr,
>> + OUTSTR_BUF_SIZE - strlen(output_str));
>> + strncat(output_str, ",",
>=20
> Please see man page for strncat(). Why is the third parameter simply =
not strlen(addr)?=20

I see what you are trying to do. You are doing a bound check here. In =
that case, should this be
OUTSTR_BUF_SIZE - strlen(output_str) - 1 to accommodate the terminating =
NULL? Man page says

> it will use at most n bytes from src

So n bytes from addr and one extra at the end for NULL termination since =
strncat () will add a terminating NULL byte in the end.

>=20
>> + OUTSTR_BUF_SIZE - strlen(output_str));
>=20
>=20
>=20
>> + }
>> + } else {
>> + continue;
>> + }
>> + }
>> +
>> + if (strlen(output_str)) {
>=20
> Please add the comment I mentioned in the previous version as to why =
you are doing this.
>=20
>> + output_str[strlen(output_str) - 1] =3D '\0';
>> + error =3D fprintf(f, "%s=3D%s\n", param_name, output_str);
>> + }
>> +
>> + free(output_str);
>> + return error;
>> +}
>> +
>> static int process_ip_string_nm(FILE *f, char *ip_string, char =
*subnet,
>> - int is_ipv6)
>> + int ip_sec)
>> {
>> char addr[INET6_ADDRSTRLEN];
>> char subnet_addr[INET6_ADDRSTRLEN];
>> int error, i =3D 0;
>> int ip_offset =3D 0, subnet_offset =3D 0;
>> - int plen;
>> + int plen, ip_ver;
>>=20
>> memset(addr, 0, sizeof(addr));
>> memset(subnet_addr, 0, sizeof(subnet_addr));
>> @@ -1216,10 +1296,16 @@ static int process_ip_string_nm(FILE *f, char =
*ip_string, char *subnet,
>>      subnet_addr,
>>      (MAX_IP_ADDR_SIZE *
>> 2))) {
>> - if (!is_ipv6)
>> + ip_ver =3D ip_version_check(addr);
>> + if (ip_ver < 0)
>> + continue;
>> +
>> + if (ip_ver =3D=3D IPV4 && ip_sec =3D=3D IPV4)
>> plen =3D kvp_subnet_to_plen((char *)subnet_addr);
>> - else
>> + else if (ip_ver =3D=3D IPV6 && ip_sec =3D=3D IPV6)
>> plen =3D atoi(subnet_addr);
>> + else
>> + continue;
>>=20
>> if (plen < 0)
>> return plen;
>> @@ -1238,12 +1324,11 @@ static int process_ip_string_nm(FILE *f, char =
*ip_string, char *subnet,
>>=20
>> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value =
*new_val)
>> {
>> - int error =3D 0;
>> + int error =3D 0, ip_ver;
>> char if_filename[PATH_MAX];
>> char nm_filename[PATH_MAX];
>> FILE *ifcfg_file, *nmfile;
>> char cmd[PATH_MAX];
>> - int is_ipv6 =3D 0;
>> char *mac_addr;
>> int str_len;
>>=20
>> @@ -1421,52 +1506,62 @@ static int kvp_set_ip_info(char *if_name, =
struct hv_kvp_ipaddr_value *new_val)
>> if (error)
>> goto setval_error;
>>=20
>> - if (new_val->addr_family & ADDR_FAMILY_IPV6) {
>> - error =3D fprintf(nmfile, "\n[ipv6]\n");
>> - if (error < 0)
>> - goto setval_error;
>> - is_ipv6 =3D 1;
>> - } else {
>> - error =3D fprintf(nmfile, "\n[ipv4]\n");
>> - if (error < 0)
>> - goto setval_error;
>> - }
>> -
>> /*
>> - * Now we populate the keyfile format
>> + * The keyfile format expects the IPv6 and IPv4 configuration in
>> + * different sections. Therefore we iterate through the list twice,
>> + * once to populate the IPv4 section and the next time for IPv6
>> */
>> + ip_ver =3D IPV4;
>> + do {
>> + if (ip_ver =3D=3D IPV4) {
>> + error =3D fprintf(nmfile, "\n[ipv4]\n");
>> + if (error < 0)
>> + goto setval_error;
>> + } else {
>> + error =3D fprintf(nmfile, "\n[ipv6]\n");
>> + if (error < 0)
>> + goto setval_error;
>> + }
>>=20
>> - if (new_val->dhcp_enabled) {
>> - error =3D kvp_write_file(nmfile, "method", "", "auto");
>> - if (error < 0)
>> - goto setval_error;
>> - } else {
>> - error =3D kvp_write_file(nmfile, "method", "", "manual");
>> + /*
>> + * Now we populate the keyfile format
>> + */
>> +
>> + if (new_val->dhcp_enabled) {
>> + error =3D kvp_write_file(nmfile, "method", "", "auto");
>> + if (error < 0)
>> + goto setval_error;
>> + } else {
>> + error =3D kvp_write_file(nmfile, "method", "", "manual");
>> + if (error < 0)
>> + goto setval_error;
>> + }
>> +
>> + /*
>> + * Write the configuration for ipaddress, netmask, gateway and
>> + * name services
>> + */
>> + error =3D process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
>> +     (char *)new_val->sub_net,
>> +     ip_ver);
>> if (error < 0)
>> goto setval_error;
>> - }
>>=20
>> - /*
>> - * Write the configuration for ipaddress, netmask, gateway and
>> - * name services
>> - */
>> - error =3D process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
>> -     (char *)new_val->sub_net, is_ipv6);
>> - if (error < 0)
>> - goto setval_error;
>> -
>> - /* we do not want ipv4 addresses in ipv6 section and vice versa */
>> - if (is_ipv6 !=3D is_ipv4((char *)new_val->gate_way)) {
>> - error =3D fprintf(nmfile, "gateway=3D%s\n", (char =
*)new_val->gate_way);
>> + error =3D process_dns_gateway_nm(nmfile,
>> +       (char *)new_val->gate_way,
>> +       GATEWAY, ip_ver);
>> if (error < 0)
>> goto setval_error;
>> - }
>>=20
>> - if (is_ipv6 !=3D is_ipv4((char *)new_val->dns_addr)) {
>> - error =3D fprintf(nmfile, "dns=3D%s\n", (char *)new_val->dns_addr);
>> + error =3D process_dns_gateway_nm(nmfile,
>> +       (char *)new_val->dns_addr, DNS,
>> +       ip_ver);
>> if (error < 0)
>> goto setval_error;
>> - }
>> +
>> + ip_ver++;
>> + } while (ip_ver < IP_TYPE_MAX);
>> +
>> fclose(nmfile);
>> fclose(ifcfg_file);
>>=20
>> --=20
>> 2.34.1