RE: [PATCH v2 8/9] net/mlx5: fix non full word sample fields in flex item

[Dariusz Sosnowski <dsosnowski@nvidia.com>]

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Wednesday, September 18, 2024 15:46
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Dariusz Sosnowski
> <dsosnowski@nvidia.com>; stable@dpdk.org
> Subject: [PATCH v2 8/9] net/mlx5: fix non full word sample fields in flex item
> 
> If the sample field in flex item did not cover the entire 32-bit word (width was not
> verified 32 bits) or was not aligned on the byte boundary the match on this
> sample in flows happened to be ignored or wrongly missed. The field mask "def"
> was build in wrong endianness, and non-byte aligned shifts were wrongly
> performed for the pattern masks and values.
> 
> Fixes: 6dac7d7ff2bf ("net/mlx5: translate flex item pattern into matcher")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  drivers/net/mlx5/hws/mlx5dr_definer.c |  4 +--
>  drivers/net/mlx5/mlx5.h               |  5 ++-
>  drivers/net/mlx5/mlx5_flow_dv.c       |  5 ++-
>  drivers/net/mlx5/mlx5_flow_flex.c     | 47 +++++++++++++--------------
>  4 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
> b/drivers/net/mlx5/hws/mlx5dr_definer.c
> index 2dfcc5eba6..10b986d66b 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_definer.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
> @@ -574,7 +574,7 @@ mlx5dr_definer_flex_parser_set(struct
> mlx5dr_definer_fc *fc,
>  	idx = fc->fname - MLX5DR_DEFINER_FNAME_FLEX_PARSER_0;
>  	byte_off -= idx * sizeof(uint32_t);
>  	ret = mlx5_flex_get_parser_value_per_byte_off(flex, flex->handle,
> byte_off,
> -						      false, is_inner, &val);
> +						      is_inner, &val);
>  	if (ret == -1 || !val)
>  		return;
> 
> @@ -2825,7 +2825,7 @@ mlx5dr_definer_conv_item_flex_parser(struct
> mlx5dr_definer_conv_data *cd,
>  	for (i = 0; i < MLX5_GRAPH_NODE_SAMPLE_NUM; i++) {
>  		byte_off = base_off - i * sizeof(uint32_t);
>  		ret = mlx5_flex_get_parser_value_per_byte_off(m, v->handle,
> byte_off,
> -							      true, is_inner,
> &mask);
> +							      is_inner,
> &mask);
>  		if (ret == -1) {
>  			rte_errno = EINVAL;
>  			return rte_errno;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> b1423b6868..0fb18f7fb1 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -2600,11 +2600,10 @@ void mlx5_flex_flow_translate_item(struct
> rte_eth_dev *dev, void *matcher,
>  				   void *key, const struct rte_flow_item *item,
>  				   bool is_inner);
>  int mlx5_flex_get_sample_id(const struct mlx5_flex_item *tp,
> -			    uint32_t idx, uint32_t *pos,
> -			    bool is_inner, uint32_t *def);
> +			    uint32_t idx, uint32_t *pos, bool is_inner);
>  int mlx5_flex_get_parser_value_per_byte_off(const struct rte_flow_item_flex
> *item,
>  					    void *flex, uint32_t byte_off,
> -					    bool is_mask, bool tunnel,
> uint32_t *value);
> +					    bool tunnel, uint32_t *value);
>  int mlx5_flex_get_tunnel_mode(const struct rte_flow_item *item,
>  			      enum rte_flow_item_flex_tunnel_mode
> *tunnel_mode);  int mlx5_flex_acquire_index(struct rte_eth_dev *dev, diff --git
> a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index
> b18bb430d7..d2a3f829d5 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1526,7 +1526,6 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
>  	const struct mlx5_flex_pattern_field *map;
>  	uint32_t offset = data->offset;
>  	uint32_t width_left = width;
> -	uint32_t def;
>  	uint32_t cur_width = 0;
>  	uint32_t tmp_ofs;
>  	uint32_t idx = 0;
> @@ -1551,7 +1550,7 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
>  	tmp_ofs = pos < data->offset ? data->offset - pos : 0;
>  	for (j = i; i < flex->mapnum && width_left > 0; ) {
>  		map = flex->map + i;
> -		id = mlx5_flex_get_sample_id(flex, i, &pos, false, &def);
> +		id = mlx5_flex_get_sample_id(flex, i, &pos, false);
>  		if (id == -1) {
>  			i++;
>  			/* All left length is dummy */
> @@ -1570,7 +1569,7 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
>  			 * 2. Width has been covered.
>  			 */
>  			for (j = i + 1; j < flex->mapnum; j++) {
> -				tmp_id = mlx5_flex_get_sample_id(flex, j,
> &pos, false, &def);
> +				tmp_id = mlx5_flex_get_sample_id(flex, j,
> &pos, false);
>  				if (tmp_id == -1) {
>  					i = j;
>  					pos -= flex->map[j].width;
> diff --git a/drivers/net/mlx5/mlx5_flow_flex.c
> b/drivers/net/mlx5/mlx5_flow_flex.c
> index 0c41b956b0..bf38643a23 100644
> --- a/drivers/net/mlx5/mlx5_flow_flex.c
> +++ b/drivers/net/mlx5/mlx5_flow_flex.c
> @@ -118,28 +118,32 @@ mlx5_flex_get_bitfield(const struct
> rte_flow_item_flex *item,
>  		       uint32_t pos, uint32_t width, uint32_t shift)  {
>  	const uint8_t *ptr = item->pattern + pos / CHAR_BIT;
> -	uint32_t val, vbits;
> +	uint32_t val, vbits, skip = pos % CHAR_BIT;
> 
>  	/* Proceed the bitfield start byte. */
>  	MLX5_ASSERT(width <= sizeof(uint32_t) * CHAR_BIT && width);
>  	MLX5_ASSERT(width + shift <= sizeof(uint32_t) * CHAR_BIT);
>  	if (item->length <= pos / CHAR_BIT)
>  		return 0;
> -	val = *ptr++ >> (pos % CHAR_BIT);
> +	/* Bits are enumerated in byte in network order: 01234567 */
> +	val = *ptr++;
>  	vbits = CHAR_BIT - pos % CHAR_BIT;
> -	pos = (pos + vbits) / CHAR_BIT;
> +	pos = RTE_ALIGN_CEIL(pos, CHAR_BIT) / CHAR_BIT;
>  	vbits = RTE_MIN(vbits, width);
> -	val &= RTE_BIT32(vbits) - 1;
> +	/* Load bytes to cover the field width, checking pattern boundary */
>  	while (vbits < width && pos < item->length) {
>  		uint32_t part = RTE_MIN(width - vbits, (uint32_t)CHAR_BIT);
>  		uint32_t tmp = *ptr++;
> 
> -		pos++;
> -		tmp &= RTE_BIT32(part) - 1;
> -		val |= tmp << vbits;
> +		val |= tmp << RTE_ALIGN_CEIL(vbits, CHAR_BIT);
>  		vbits += part;
> +		pos++;
>  	}
> -	return rte_bswap32(val <<= shift);
> +	val = rte_cpu_to_be_32(val);
> +	val <<= skip;
> +	val >>= shift;
> +	val &= (RTE_BIT64(width) - 1) << (sizeof(uint32_t) * CHAR_BIT - shift -
> width);
> +	return val;
>  }
> 
>  #define SET_FP_MATCH_SAMPLE_ID(x, def, msk, val, sid) \ @@ -211,21 +215,17
> @@ mlx5_flex_set_match_sample(void *misc4_m, void *misc4_v,
>   *   Where to search the value and mask.
>   * @param[in] is_inner
>   *   For inner matching or not.
> - * @param[in, def] def
> - *   Mask generated by mapping shift and width.
>   *
>   * @return
>   *   0 on success, -1 to ignore.
>   */
>  int
>  mlx5_flex_get_sample_id(const struct mlx5_flex_item *tp,
> -			uint32_t idx, uint32_t *pos,
> -			bool is_inner, uint32_t *def)
> +			uint32_t idx, uint32_t *pos, bool is_inner)
>  {
>  	const struct mlx5_flex_pattern_field *map = tp->map + idx;
>  	uint32_t id = map->reg_id;
> 
> -	*def = (RTE_BIT64(map->width) - 1) << map->shift;
>  	/* Skip placeholders for DUMMY fields. */
>  	if (id == MLX5_INVALID_SAMPLE_REG_ID) {
>  		*pos += map->width;
> @@ -252,8 +252,6 @@ mlx5_flex_get_sample_id(const struct mlx5_flex_item
> *tp,
>   *   Mlx5 flex item sample mapping handle.
>   * @param[in] byte_off
>   *   Mlx5 flex item format_select_dw.
> - * @param[in] is_mask
> - *   Spec or mask.
>   * @param[in] tunnel
>   *   Tunnel mode or not.
>   * @param[in, def] value
> @@ -265,25 +263,23 @@ mlx5_flex_get_sample_id(const struct mlx5_flex_item
> *tp,  int  mlx5_flex_get_parser_value_per_byte_off(const struct
> rte_flow_item_flex *item,
>  					void *flex, uint32_t byte_off,
> -					bool is_mask, bool tunnel, uint32_t
> *value)
> +					bool tunnel, uint32_t *value)
>  {
>  	struct mlx5_flex_pattern_field *map;
>  	struct mlx5_flex_item *tp = flex;
> -	uint32_t def, i, pos, val;
> +	uint32_t i, pos, val;
>  	int id;
> 
>  	*value = 0;
>  	for (i = 0, pos = 0; i < tp->mapnum && pos < item->length * CHAR_BIT;
> i++) {
>  		map = tp->map + i;
> -		id = mlx5_flex_get_sample_id(tp, i, &pos, tunnel, &def);
> +		id = mlx5_flex_get_sample_id(tp, i, &pos, tunnel);
>  		if (id == -1)
>  			continue;
>  		if (id >= (int)tp->devx_fp->num_samples || id >=
> MLX5_GRAPH_NODE_SAMPLE_NUM)
>  			return -1;
>  		if (byte_off == tp->devx_fp->sample_info[id].sample_dw_data *
> sizeof(uint32_t)) {
>  			val = mlx5_flex_get_bitfield(item, pos, map->width,
> map->shift);
> -			if (is_mask)
> -				val &= RTE_BE32(def);
>  			*value |= val;
>  		}
>  		pos += map->width;
> @@ -355,10 +351,10 @@ mlx5_flex_flow_translate_item(struct rte_eth_dev
> *dev,
>  	spec = item->spec;
>  	mask = item->mask;
>  	tp = (struct mlx5_flex_item *)spec->handle;
> -	for (i = 0; i < tp->mapnum; i++) {
> +	for (i = 0; i < tp->mapnum && pos < (spec->length * CHAR_BIT); i++) {
>  		struct mlx5_flex_pattern_field *map = tp->map + i;
>  		uint32_t val, msk, def;
> -		int id = mlx5_flex_get_sample_id(tp, i, &pos, is_inner, &def);
> +		int id = mlx5_flex_get_sample_id(tp, i, &pos, is_inner);
> 
>  		if (id == -1)
>  			continue;
> @@ -366,11 +362,14 @@ mlx5_flex_flow_translate_item(struct rte_eth_dev
> *dev,
>  		if (id >= (int)tp->devx_fp->num_samples ||
>  		    id >= MLX5_GRAPH_NODE_SAMPLE_NUM)
>  			return;
> +		def = (uint32_t)(RTE_BIT64(map->width) - 1);
> +		def <<= (sizeof(uint32_t) * CHAR_BIT - map->shift - map-
> >width);
>  		val = mlx5_flex_get_bitfield(spec, pos, map->width, map-
> >shift);
> -		msk = mlx5_flex_get_bitfield(mask, pos, map->width, map-
> >shift);
> +		msk = pos < (mask->length * CHAR_BIT) ?
> +		      mlx5_flex_get_bitfield(mask, pos, map->width, map->shift) :
> +def;
>  		sample_id = tp->devx_fp->sample_ids[id];
>  		mlx5_flex_set_match_sample(misc4_m, misc4_v,
> -					   def, msk & def, val & msk & def,
> +					   def, msk, val & msk,
>  					   sample_id, id);
>  		pos += map->width;
>  	}
> --
> 2.34.1

Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>

Resending the Ack for each patch separately, because patchwork assigned my Ack for the series to v1, not v2.

Best regards,
Dariusz Sosnowski