Received-SPF: pass (google.com: domain of linux-kernel+bounces-100551-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <3bf8844a-3e19-4105-8cce-2b1f8f98d3bc@linux.microsoft.com>
Date: Tue, 12 Mar 2024 09:58:03 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format
Content-Language: en-US
To: Shradha Gupta <shradhagupta@linux.microsoft.com>,
 linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Cc: "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>,
 Ani Sinha <anisinha@redhat.com>, Shradha Gupta <shradhagupta@microsoft.com>
References: <1710247112-7414-1-git-send-email-shradhagupta@linux.microsoft.com>
From: Easwar Hariharan <eahariha@linux.microsoft.com>
In-Reply-To: <1710247112-7414-1-git-send-email-shradhagupta@linux.microsoft.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 3/12/2024 5:38 AM, Shradha Gupta wrote:
> If the network configuration strings are passed as a combination of IPv and

                                                                      *IPv4*

> IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
                                         *does not/doesn't*
> 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.
> 
> Built-on: Rhel9
> Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)

As mentioned by Jakub[1], what value does this information provide?
Please follow Haiyang's suggestion [2] and put SKU and test information, or just
skip it.

[1] https://lore.kernel.org/all/20240307072923.6cc8a2ba@kernel.org/
[2] https://lore.kernel.org/all/DM6PR21MB14817597567C638DEF020FE3CA202@DM6PR21MB1481.namprd21.prod.outlook.com/

> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> ---
>  Changes in v2
>  * Use calloc to avoid initialization later
>  * Return standard error codes
>  * Free the output_str pointer on completion
>  * Add out-of bound checks while writing to buffers
> ---
>  tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
>  1 file changed, 132 insertions(+), 41 deletions(-)
> 
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
>  
>  #define MAX_FILE_NAME 100
>  #define ENTRIES_PER_BLOCK 50
> +#define MAX_IP_ENTRIES 64

Is this a limitation defined by hv_kvp? If so, is it possible it may change in a later
version? A comment would help here

>  
>  struct kvp_record {
>  	char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> @@ -1171,6 +1178,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;

You can skip the else here...

> +	else
> +		return -EINVAL;

..and you can skip the else here as well and just return -EINVAL

> +}
> +
>  /*
>   * Only IPv4 subnet strings needs to be converted to plen
>   * For IPv6 the subnet is already privided in plen format
> @@ -1197,14 +1216,71 @@ 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;
> +
> +	output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> +				    sizeof(char));
> +
> +	if (!output_str)
> +		return -ENOMEM;
> +
> +	memset(addr, 0, sizeof(addr));
> +
> +	if (type == DNS) {
> +		param_name = "dns";
> +	} else if (type == GATEWAY) {
> +		param_name = "gateway";
> +	} else {
> +		error = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> +				   (MAX_IP_ADDR_SIZE * 2))) {
> +		ip_ver = ip_version_check(addr);
> +		if (ip_ver < 0)
> +			continue;
> +
> +		if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> +		    (ip_ver == IPV6 && ip_sec == IPV6)) {
> +			if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> +			    (strlen(addr))) {
> +				strcat(output_str, addr);
> +				strcat(output_str, ",");

Prefer strncat() here

> +			}
> +			memset(addr, 0, sizeof(addr));
> +
> +		} else {
> +			memset(addr, 0, sizeof(addr));
> +			continue;
> +		}
> +	}
> +
> +	if (strlen(output_str)) {
> +		output_str[strlen(output_str) - 1] = '\0';
> +		error = fprintf(f, "%s=%s\n", param_name, output_str);
> +		if (error <  0)
> +			goto cleanup;
> +	}
> +
> +cleanup:
> +	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 +1292,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 +1320,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_type;

nit: Can we keep ip_ver through all the functions for consistency

<snip>

Thanks,
Easwar