Re: [PATCH v2 net-next 3/4] sfc: support TC decap rules matching on enc_ip_tos

[Simon Horman <simon.horman@corigine.com>]

On Thu, May 11, 2023 at 08:47:30PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Allow efx_tc_encap_match entries to include an ip_tos and ip_tos_mask.
> To avoid partially-overlapping Outer Rules (which can lead to undefined
>  behaviour in the hardware), store extra "pseudo" entries in our
>  encap_match hashtable, which are used to enforce that all Outer Rule
>  entries within a given <src_ip,dst_ip,udp_dport> tuple (or IPv6
>  equivalent) have the same ip_tos_mask.
> The "direct" encap_match entry takes a reference on the "pseudo",
>  allowing it to be destroyed when all "direct" entries using it are
>  removed.
> efx_tc_em_pseudo_type is an enum rather than just a bool because in
>  future an additional pseudo-type will be added to support Conntrack
>  offload.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

...

> @@ -425,12 +469,56 @@ static int efx_tc_flower_record_encap_match(struct efx_nic *efx,
>  #endif
>  	encap->udp_dport = match->value.enc_dport;
>  	encap->tun_type = type;
> +	encap->ip_tos = match->value.enc_ip_tos;
> +	encap->ip_tos_mask = match->mask.enc_ip_tos;
> +	encap->child_ip_tos_mask = child_ip_tos_mask;
> +	encap->type = em_type;
> +	encap->pseudo = pseudo;
>  	old = rhashtable_lookup_get_insert_fast(&efx->tc->encap_match_ht,
>  						&encap->linkage,
>  						efx_tc_encap_match_ht_params);
>  	if (old) {
>  		/* don't need our new entry */
>  		kfree(encap);

Hi Ed,

encap is freed here.

> +		if (pseudo) /* don't need our new pseudo either */
> +			efx_tc_flower_release_encap_match(efx, pseudo);
> +		/* check old and new em_types are compatible */
> +		switch (old->type) {
> +		case EFX_TC_EM_DIRECT:
> +			/* old EM is in hardware, so mustn't overlap with a
> +			 * pseudo, but may be shared with another direct EM
> +			 */
> +			if (em_type == EFX_TC_EM_DIRECT)
> +				break;
> +			NL_SET_ERR_MSG_MOD(extack, "Pseudo encap match conflicts with existing direct entry");
> +			return -EEXIST;
> +		case EFX_TC_EM_PSEUDO_MASK:
> +			/* old EM is protecting a ToS-qualified filter, so may
> +			 * only be shared with another pseudo for the same
> +			 * ToS mask.
> +			 */
> +			if (em_type != EFX_TC_EM_PSEUDO_MASK) {
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "%s encap match conflicts with existing pseudo(MASK) entry",
> +						       encap->type ? "Pseudo" : "Direct");

But dereferenced here.

> +				return -EEXIST;
> +			}
> +			if (child_ip_tos_mask != old->child_ip_tos_mask) {
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "Pseudo encap match for TOS mask %#04x conflicts with existing pseudo(MASK) entry for TOS mask %#04x",
> +						       child_ip_tos_mask,
> +						       old->child_ip_tos_mask);
> +				return -EEXIST;
> +			}
> +			break;
> +		default: /* Unrecognised pseudo-type.  Just say no */
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "%s encap match conflicts with existing pseudo(%d) entry",
> +					       encap->type ? "Pseudo" : "Direct",

And here.

> +					       old->type);
> +			return -EEXIST;
> +		}
> +		/* check old and new tun_types are compatible */
>  		if (old->tun_type != type) {
>  			NL_SET_ERR_MSG_FMT_MOD(extack,
>  					       "Egress encap match with conflicting tun_type %u != %u",

...