Message-ID: <20240306085830.GA18235@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Wed, 6 Mar 2024 00:58:30 -0800
From: Shradha Gupta <shradhagupta@...ux.microsoft.com>
To: Ani Sinha <anisinha@...hat.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>,
	Michael Kelley <mikelley@...rosoft.com>,
	Olaf Hering <olaf@...fle.de>,
	Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format

Thanks Ani, will send out another version soon with these fixes.

On Wed, Mar 06, 2024 at 01:54:36PM +0530, Ani Sinha wrote:
> 
> 
> > On 01-Mar-2024, at 18:26, Shradha Gupta <shradhagupta@...ux.microsoft.com> wrote:
> > 
> > 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.
> 
> We tried to test this but seems some memory corruption is happening.
> Some more feedbacks follows ...
> 
> > 
> > Built-on: Rhel9
> > Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> > Signed-off-by: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> > ---
> > tools/hv/hv_kvp_daemon.c | 152 ++++++++++++++++++++++++++++-----------
> > 1 file changed, 112 insertions(+), 40 deletions(-)
> > 
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index 318e2dad27e0..7e84e40b55fb 100644
> > --- a/tools/hv/hv_kvp_daemon.c
> > +++ b/tools/hv/hv_kvp_daemon.c
> > @@ -76,6 +76,11 @@ enum {
> > DNS
> > };
> > 
> > +enum {
> > + IPV4 = 1,
> > + IPV6
> > +};
> > +
> > static int in_hand_shake;
> > 
> > static char *os_name = "";
> > @@ -1171,6 +1176,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;
> > + else
> > + return -EINVAL;
> > +}
> > +
> > /*
> >  * Only IPv4 subnet strings needs to be converted to plen
> >  * For IPv6 the subnet is already privided in plen format
> > @@ -1197,14 +1214,56 @@ 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, ip_ver;
> > + char *param_name;
> > +
> > + output_str = malloc(strlen(ip_string));
> > +
> > + if (!output_str)
> > + return 1;
> 
> Shouldn???t you be returning -ENOMEM or some such here?
> 
> > +
> > + output_str[0] = '\0';
> > +
> > + if (type == DNS)
> > + param_name = "dns";
> > + else
> 
> Please be more explicit here and check for type == GATEWAY
> 
> > + param_name = "gateway";
> 
> Else, if neither, it should return error.
> 
> > +
> > + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> > +   (MAX_IP_ADDR_SIZE * 2))) {
> > + ip_ver = ip_version_check(addr);
> > +
> > + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> > +    (ip_ver == IPV6 && ip_sec == IPV6)) {
> > + strcat(output_str, addr);
> > + strcat(output_str, ",");
> 
> I prefer strncat() here.
> 
> > + } else {
> > + continue;
> > + }
> > + }
> > +
> > + if (strlen(output_str)) {
> > + output_str[strlen(output_str) - 1] = '\0';
> > + error = fprintf(f, "%s=%s\n", param_name, output_str);
> 
> Please do not forget to free output_str.
> 
> > + if (error <  0)
> > + return error;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > 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 +1275,13 @@ 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 == 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;
> > @@ -1242,8 +1304,8 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> > char if_filename[PATH_MAX];
> > char nm_filename[PATH_MAX];
> > FILE *ifcfg_file, *nmfile;
> > + int ip_sections_count;
> > char cmd[PATH_MAX];
> > - int is_ipv6 = 0;
> > char *mac_addr;
> > int str_len;
> > 
> > @@ -1421,52 +1483,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_sections_count = 1;
> > + do {
> > + if (ip_sections_count == 1) {
> > + 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;
> > + }
> > +
> > + /*
> > + * 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_sections_count);
> > 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_sections_count);
> > 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_sections_count);
> > if (error < 0)
> 
> But you return +ve from this function so this will never be true.
> 
> > goto setval_error;
> > - }
> > +
> > + ip_sections_count++;
> > + } while (ip_sections_count <= 2);
> > +
> > fclose(nmfile);
> > fclose(ifcfg_file);
> > 
> > -- 
> > 2.34.1
> > 

Powered by blists - more mailing lists