Message-Id: <9879EB6C-FEC4-42AD-8B40-90457455F0DD@redhat.com>
Date: Thu, 21 Mar 2024 09:25:40 +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 v4] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format



> On 20 Mar 2024, at 16:47, Shradha Gupta <shradhagupta@...ux.microsoft.com> wrote:
> 
> If the network configuration strings are passed as a combination of IPv4
> and IPv6 addresses, the current KVP daemon does not handle processing 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.
> 
> Testcases ran:Rhel 9, Hyper-V VMs
>              (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> Signed-off-by: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> ---
> Changes in v4
> * Removed the unnecessary memset for addr in the start
> * Added a comment to describe how we erase the last comma character
> * Fixed some typos in the commit description
> * While using strncat, skip copying the '\0' character.
> ---
> tools/hv/hv_kvp_daemon.c | 181 ++++++++++++++++++++++++++++++---------
> 1 file changed, 140 insertions(+), 41 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..d64d548a802f 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -76,6 +76,12 @@ enum {
> DNS
> };
> 
> +enum {
> + IPV4 = 1,
> + IPV6,
> + IP_TYPE_MAX
> +};
> +
> static int in_hand_shake;
> 
> static char *os_name = "";
> @@ -102,6 +108,11 @@ static struct utsname uts_buf;
> 
> #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)
> 
> 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;
> }
> 
> +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,75 @@ static int kvp_subnet_to_plen(char *subnet_addr_str)
> return plen;
> }
> 
> +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 = 0, error = 0, ip_ver;
> + char *param_name;
> +
> + if (type == DNS)
> + param_name = "dns";
> + else if (type == GATEWAY)
> + param_name = "gateway";
> + else
> + return -EINVAL;
> +
> + output_str = (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 = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> +    (ip_ver == IPV6 && ip_sec == 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) - 1);
> + strncat(output_str, ",",
> + OUTSTR_BUF_SIZE -
> + strlen(output_str) - 1);
> + }
> + } else {
> + continue;
> + }
> + }
> +
> + if (strlen(output_str)) {
> + /*
> + * This is to get rid of that extra comma character
> + * in the end of the string
> + */
> + output_str[strlen(output_str) - 1] = '\0';
> + error = fprintf(f, "%s=%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 = 0;
> int ip_offset = 0, subnet_offset = 0;
> - int plen;
> + int plen, ip_ver;
> 
> memset(addr, 0, sizeof(addr));
> memset(subnet_addr, 0, sizeof(subnet_addr));
> @@ -1216,10 +1300,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 = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if (ip_ver == IPV4 && ip_sec == IPV4)
> plen = kvp_subnet_to_plen((char *)subnet_addr);
> - else
> + else if (ip_ver == IPV6 && ip_sec == IPV6)
> plen = atoi(subnet_addr);
> + else
> + continue;
> 
> if (plen < 0)
> return plen;
> @@ -1238,12 +1328,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> 
> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> {
> - int error = 0;
> + int error = 0, ip_ver;
> char if_filename[PATH_MAX];
> char nm_filename[PATH_MAX];
> FILE *ifcfg_file, *nmfile;
> char cmd[PATH_MAX];
> - int is_ipv6 = 0;
> char *mac_addr;
> int str_len;
> 
> @@ -1421,52 +1510,62 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> if (error)
> goto setval_error;
> 
> - if (new_val->addr_family & ADDR_FAMILY_IPV6) {
> - error = fprintf(nmfile, "\n[ipv6]\n");
> - if (error < 0)
> - goto setval_error;
> - is_ipv6 = 1;
> - } else {
> - error = 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 = IPV4;
> + do {
> + if (ip_ver == IPV4) {
> + error = fprintf(nmfile, "\n[ipv4]\n");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = fprintf(nmfile, "\n[ipv6]\n");
> + if (error < 0)
> + goto setval_error;
> + }
> 
> - if (new_val->dhcp_enabled) {
> - error = kvp_write_file(nmfile, "method", "", "auto");
> - if (error < 0)
> - goto setval_error;
> - } else {
> - error = kvp_write_file(nmfile, "method", "", "manual");
> + /*
> + * Now we populate the keyfile format
> + */
> +
> + if (new_val->dhcp_enabled) {
> + error = kvp_write_file(nmfile, "method", "", "auto");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = kvp_write_file(nmfile, "method", "", "manual");
> + if (error < 0)
> + goto setval_error;

There is a problem with this code. dhcp_enabled is only valid for ipv4. From looking at ifcfg files that were generated before, I do not see IPV6_AUTOCONF related settings. So maybe we should set method only for ipv4 and not for ipv6.

If the user configures only ipv6, then we do not want to have a section with
method = manual for ipv4. method = manual without an IP address does not work. So I suggest that we set method = manual only after checking that ipv4 addresses were added. So maybe move this section a little below after call to process_ip_string_nm(). This function can return a specific value to indicate that address were indeed written to the if cfg/kefile. I am not sure what happens when dhcp_enabled is True and the user provides specific IP addresses.


> + }
> +
> + /*
> + * Write the configuration for ipaddress, netmask, gateway and
> + * name services
> + */
> + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> +     (char *)new_val->sub_net,
> +     ip_ver);
> if (error < 0)
> goto setval_error;
> - }
> 
> - /*
> - * Write the configuration for ipaddress, netmask, gateway and
> - * name services
> - */
> - error = 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 != is_ipv4((char *)new_val->gate_way)) {
> - error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
> + error = process_dns_gateway_nm(nmfile,
> +       (char *)new_val->gate_way,
> +       GATEWAY, ip_ver);
> if (error < 0)
> goto setval_error;
> - }
> 
> - if (is_ipv6 != is_ipv4((char *)new_val->dns_addr)) {
> - error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
> + error = 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);
> 
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists