linux-kernel - Re: [net-next PATCH v2] octeontx2-pf: Add support for offload tc with skbedit mark action

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240416103702.GI2320920@kernel.org>
Date: Tue, 16 Apr 2024 11:37:02 +0100
From: Simon Horman <horms@...nel.org>
To: Geetha sowjanya <gakula@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, kuba@...nel.org,
	davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
	sgoutham@...vell.com, sbhatta@...vell.com, hkelam@...vell.com
Subject: Re: [net-next PATCH v2] octeontx2-pf: Add support for offload tc
 with skbedit mark action

On Sun, Apr 14, 2024 at 11:59:57AM +0530, Geetha sowjanya wrote:
> Support offloading of skbedit mark action.
> 
> For example, to mark with 0x0008, with dest ip 60.60.60.2 on eth2
> interface:
> 
>  # tc qdisc add dev eth2 ingress
>  # tc filter add dev eth2 ingress protocol ip flower \
>       dst_ip 60.60.60.2 action skbedit mark 0x0008 skip_sw
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Geetha sowjanya <gakula@...vell.com>
> ---
> v1-v2: 
>   -Changed mark_flows data type to refcount_t 

Thanks Geetha,

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 87bdb93cb066..8b8ac179f3c3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -511,7 +511,15 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
>  			nr_police++;
>  			break;
>  		case FLOW_ACTION_MARK:
> +			if (act->mark & ~OTX2_RX_MATCH_ID_MASK) {
> +				NL_SET_ERR_MSG_MOD(extack, "Bad flow mark, only 16 bit supported");
> +				return -EOPNOTSUPP;
> +			}
>  			mark = act->mark;
> +			req->match_id = mark & 0xFFFFULL;

nit: Is the mask necessary here?
     act->mark was already checked against OTX2_RX_MATCH_ID_MASK
     to ensure that nothing is set in the upper 16 bits.

     If it is, could OTX2_RX_MATCH_ID_MASK be used instead of 0xFFFFULL ?
     Or perhaps use lower_16_bits().

     No need for this to block this patch, AFAIK.
     But perhaps a follow-up could be considered.

> +			req->op = NIX_RX_ACTION_DEFAULT;
> +			nic->flags |= OTX2_FLAG_TC_MARK_ENABLED;
> +			refcount_inc(&nic->flow_cfg->mark_flows);
>  			break;
>  
>  		case FLOW_ACTION_RX_QUEUE_MAPPING:

..

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> index a82ffca8ce1b..3f1d2655ff77 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.h
> @@ -62,6 +62,9 @@
>  #define CQ_OP_STAT_OP_ERR       63
>  #define CQ_OP_STAT_CQ_ERR       46
>  
> +/* Packet mark mask */
> +#define OTX2_RX_MATCH_ID_MASK 0x0000ffff
> +
>  struct queue_stats {
>  	u64	bytes;
>  	u64	pkts;
> -- 
> 2.25.1
> 
>