Re: [PATCH 1/2] ethdev: parsing multiple representor devargs string

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 1/11/24 09:44, Harman Kalra wrote:
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>

[snip]

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index bd917a15fc..62a06b75a2 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -470,15 +470,14 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   }
>   
>   int
> -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs)
>   {
> -	struct rte_kvargs args;
> +	struct rte_eth_devargs *eth_da;
>   	struct rte_kvargs_pair *pair;
> -	unsigned int i;
> +	struct rte_kvargs args;
> +	unsigned int i, j = 0;

Please, avoid multiple variable in one line declaration with
initialization. It is too error prone. Also j is a bad name
here. It is not a loop counter or seomthing like this.
Maybe 'parsed' or 'devargs_parsed'?

>   	int result = 0;
>   
> -	memset(eth_da, 0, sizeof(*eth_da));
> -
>   	result = eth_dev_devargs_tokenise(&args, dargs);
>   	if (result < 0)
>   		goto parse_cleanup;
> @@ -486,18 +485,16 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
>   	for (i = 0; i < args.count; i++) {
>   		pair = &args.pairs[i];
>   		if (strcmp("representor", pair->key) == 0) {
> -			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
> -				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
> -					dargs);
> -				result = -1;
> -				goto parse_cleanup;
> -			}
> +			eth_da = &eth_devargs[j];
> +			memset(eth_da, 0, sizeof(*eth_da));
>   			result = rte_eth_devargs_parse_representor_ports(
>   					pair->value, eth_da);
>   			if (result < 0)
>   				goto parse_cleanup;
> +			j++;
>   		}
>   	}
> +	result = (int)j;
>   
>   parse_cleanup:
>   	free(args.str);
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index b482cd12bb..6f4f1a1606 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1800,10 +1800,10 @@ rte_eth_representor_id_get(uint16_t port_id,
>    * @param devargs
>    *  device arguments
>    * @param eth_devargs
> - *  parsed ethdev specific arguments.
> + *  contiguous memory populated with parsed ethdev specific arguments.

Caller must provide information about array size. Right now you simply
introduce out-of-bound array access.
All drivers just provide one entry and if I pass many-many devargs I
can write far beyond that location and corrupt stack.

>    *
>    * @return
> - *   Negative errno value on error, 0 on success.
> + *   Negative errno value on error, no of devargs parsed on success.
>    */
>   __rte_internal
>   int