Re: [PATCH net v2 1/2] mlx5: fix possible ptp queue fifo overflow

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 24 Jan 2023 03:08:35 +0300 Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@meta.com>
> 
> Fifo pointers are not checked for overflow and this could potentially
> lead to overflow and double free under heavy PTP traffic.
> 
> Also there were accidental OOO cqe which lead to absolutely broken fifo.
> Add checks to workaround OOO cqe and add counters to show the amount of
> such events.

May be worth adding a mention of the brokenness of the empty() check.
Comparing free running counters works well unless C promotes the types
to something wider than the counters themselves. So unsigned or u32
works, but comparing two u16s or u8s needs a explicit cast.

> -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id)
> +static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id)
>  {
>  	struct skb_shared_hwtstamps hwts = {};
>  	struct sk_buff *skb;
>  
>  	ptpsq->cq_stats->resync_event++;
>  
> -	while (skb_cc != skb_id) {
> -		skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo);
> +	if (skb_cc > skb_id || PTP_WQE_CTR2IDX(ptpsq->skb_fifo_pc) < skb_id) {

Are you sure this works for all cases?
Directly comparing indexes of a ring buffer seems dangerous.
We'd need to compare like this:

	(s16)(skb_cc - skb_id) < 0

> +		ptpsq->cq_stats->ooo_cqe++;
> +		return false;
> +	}

> @@ -128,7 +141,8 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq,
>  	ptpsq->cq_stats->cqe++;
>  
>  out:
> -	napi_consume_skb(skb, budget);
> +	if (skb)
> +		napi_consume_skb(skb, budget);

I think napi_consume_skb() takes NULLs.