* [PATCH net v4 0/2] mlx5: ptp fifo bugfixes @ 2023-02-01 12:26 Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 1/2] mlx5: fix skb leak while fifo resync and push Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free Vadim Fedorenko 0 siblings, 2 replies; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-01 12:26 UTC (permalink / raw) To: Jakub Kicinski, Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, Saeed Mahameed Cc: Vadim Fedorenko, netdev Simple FIFO implementation for PTP queue has several bugs which lead to use-after-free and skb leaks. This series fixes the issues and adds new checks for this FIFO implementation to uncover the same problems in future. v3 -> v4: Change pr_err to mlx5_core_err_rl per suggest Removed WARN_ONCE on fifo push because has_room() should catch the issue v2 -> v3: Rearrange patches order and rephrase commit messages Remove counters as Gal confirmed FW bug, use KERN_ERR message instead Provide proper budget to napi_consume_skb as Jakub suggested v1 -> v2: Update Fixes tag to proper commit. Change debug line to avoid double print of function name Vadim Fedorenko (2): mlx5: fix skb leak while fifo resync and push mlx5: fix possible ptp queue fifo use-after-free .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 25 ++++++++++++++----- .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 6 +++-- 2 files changed, 23 insertions(+), 8 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v4 1/2] mlx5: fix skb leak while fifo resync and push 2023-02-01 12:26 [PATCH net v4 0/2] mlx5: ptp fifo bugfixes Vadim Fedorenko @ 2023-02-01 12:26 ` Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free Vadim Fedorenko 1 sibling, 0 replies; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-01 12:26 UTC (permalink / raw) To: Jakub Kicinski, Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, Saeed Mahameed Cc: Vadim Fedorenko, netdev, Tariq Toukan During ptp resync operation SKBs were poped from the fifo but were never freed neither by napi_consume nor by dev_kfree_skb_any. Add call to napi_consume_skb to properly free SKBs. Another leak was happening because mlx5e_skb_fifo_has_room() had an error in the check. Comparing free running counters works well unless C promotes the types to something wider than the counter. In this case counters are u16 but the result of the substraction is promouted to int and it causes wrong result (negative value) of the check when producer have already overlapped but consumer haven't yet. Explicit cast to u16 fixes the issue. Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port timestamp") Reviewed-by: Gal Pressman <gal@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Signed-off-by: Vadim Fedorenko <vadfed@meta.com> --- drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c | 6 ++++-- drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c index 8469e9c38670..b72de2b520ec 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c @@ -86,7 +86,8 @@ static bool mlx5e_ptp_ts_cqe_drop(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); } -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id) +static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, + u16 skb_id, int budget) { struct skb_shared_hwtstamps hwts = {}; struct sk_buff *skb; @@ -98,6 +99,7 @@ static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_ hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; skb_tstamp_tx(skb, &hwts); ptpsq->cq_stats->resync_cqe++; + napi_consume_skb(skb, budget); skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); } } @@ -119,7 +121,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, } if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) - mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id); + mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget); skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo); hwtstamp = mlx5e_cqe_ts_to_ns(sq->ptp_cyc2time, sq->clock, get_cqe_ts(cqe)); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h index c10c6ab2e7bc..d5afad368a69 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h @@ -86,7 +86,7 @@ void mlx5e_free_txqsq_descs(struct mlx5e_txqsq *sq); static inline bool mlx5e_skb_fifo_has_room(struct mlx5e_skb_fifo *fifo) { - return (*fifo->pc - *fifo->cc) < fifo->mask; + return (u16)(*fifo->pc - *fifo->cc) < fifo->mask; } static inline bool -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 12:26 [PATCH net v4 0/2] mlx5: ptp fifo bugfixes Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 1/2] mlx5: fix skb leak while fifo resync and push Vadim Fedorenko @ 2023-02-01 12:26 ` Vadim Fedorenko 2023-02-01 18:19 ` Saeed Mahameed 2023-02-02 3:08 ` Jakub Kicinski 1 sibling, 2 replies; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-01 12:26 UTC (permalink / raw) To: Jakub Kicinski, Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, Saeed Mahameed Cc: Vadim Fedorenko, netdev Fifo indexes were not checked during pop operations and it leads to potential use-after-free when poping from empty queue. Such case was possible during re-sync action. There were out-of-order cqe spotted which lead to drain of the queue and use-after-free because of lack of fifo pointers check. Special check is added to avoid resync operation if SKB could not exist in the fifo because of OOO cqe (skb_id must be between consumer and producer index). Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port timestamp") Signed-off-by: Vadim Fedorenko <vadfed@meta.com> --- .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 23 ++++++++++++++----- .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 4 +++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c index b72de2b520ec..5df726185192 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c @@ -86,7 +86,7 @@ static bool mlx5e_ptp_ts_cqe_drop(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); } -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, +static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb_id, int budget) { struct skb_shared_hwtstamps hwts = {}; @@ -94,14 +94,23 @@ static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 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) { + mlx5_core_err_rl(ptpsq->txqsq.mdev, "out-of-order ptp cqe\n"); + return false; + } + + while (skb_cc != skb_id && (skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo))) { hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; skb_tstamp_tx(skb, &hwts); ptpsq->cq_stats->resync_cqe++; napi_consume_skb(skb, budget); skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); } + + if (!skb) + return false; + + return true; } static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, @@ -111,7 +120,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, u16 skb_id = PTP_WQE_CTR2IDX(be16_to_cpu(cqe->wqe_counter)); u16 skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); struct mlx5e_txqsq *sq = &ptpsq->txqsq; - struct sk_buff *skb; + struct sk_buff *skb = NULL; ktime_t hwtstamp; if (unlikely(MLX5E_RX_ERR_CQE(cqe))) { @@ -120,8 +129,10 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, goto out; } - if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) - mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget); + if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id) && + !mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget)) { + goto out; + } skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo); hwtstamp = mlx5e_cqe_ts_to_ns(sq->ptp_cyc2time, sq->clock, get_cqe_ts(cqe)); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h index d5afad368a69..e599b86d94b5 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h @@ -295,13 +295,15 @@ static inline void mlx5e_skb_fifo_push(struct mlx5e_skb_fifo *fifo, struct sk_buff *skb) { struct sk_buff **skb_item = mlx5e_skb_fifo_get(fifo, (*fifo->pc)++); - *skb_item = skb; } static inline struct sk_buff *mlx5e_skb_fifo_pop(struct mlx5e_skb_fifo *fifo) { + if (*fifo->pc == *fifo->cc) + return NULL; + return *mlx5e_skb_fifo_get(fifo, (*fifo->cc)++); } -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 12:26 ` [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free Vadim Fedorenko @ 2023-02-01 18:19 ` Saeed Mahameed 2023-02-01 21:36 ` Vadim Fedorenko 2023-02-02 3:08 ` Jakub Kicinski 1 sibling, 1 reply; 10+ messages in thread From: Saeed Mahameed @ 2023-02-01 18:19 UTC (permalink / raw) To: Vadim Fedorenko Cc: Jakub Kicinski, Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, netdev On 01 Feb 04:26, Vadim Fedorenko wrote: >Fifo indexes were not checked during pop operations and it leads to >potential use-after-free when poping from empty queue. Such case was >possible during re-sync action. > >There were out-of-order cqe spotted which lead to drain of the queue and >use-after-free because of lack of fifo pointers check. Special check >is added to avoid resync operation if SKB could not exist in the fifo >because of OOO cqe (skb_id must be between consumer and producer index). > >Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port timestamp") >Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >--- > .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 23 ++++++++++++++----- > .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 4 +++- > 2 files changed, 20 insertions(+), 7 deletions(-) > >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >index b72de2b520ec..5df726185192 100644 >--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >@@ -86,7 +86,7 @@ static bool mlx5e_ptp_ts_cqe_drop(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb > return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); > } > >-static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, >+static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 skb_cc, > u16 skb_id, int budget) > { > struct skb_shared_hwtstamps hwts = {}; >@@ -94,14 +94,23 @@ static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 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) { To avoid returning boolean and add more functionality to this function, I prefer to put this check in mlx5e_ptp_handle_ts_cqe(), see below. >+ mlx5_core_err_rl(ptpsq->txqsq.mdev, "out-of-order ptp cqe\n"); it's better to add a counter for this, eg: ptpsq->cq_stats->ooo_cqe_drop++; >+ return false; >+ } >+ >+ while (skb_cc != skb_id && (skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo))) { > hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; > skb_tstamp_tx(skb, &hwts); > ptpsq->cq_stats->resync_cqe++; > napi_consume_skb(skb, budget); > skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); > } >+ >+ if (!skb) >+ return false; >+ >+ return true; > } > > static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, >@@ -111,7 +120,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, > u16 skb_id = PTP_WQE_CTR2IDX(be16_to_cpu(cqe->wqe_counter)); > u16 skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); > struct mlx5e_txqsq *sq = &ptpsq->txqsq; >- struct sk_buff *skb; >+ struct sk_buff *skb = NULL; > ktime_t hwtstamp; > > if (unlikely(MLX5E_RX_ERR_CQE(cqe))) { >@@ -120,8 +129,10 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, > goto out; > } > >- if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) >- mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget); you can check here: /* ignore ooo cqe as it was already handled by a previous resync */ if (ooo_cqe(cqe)) return; >+ if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id) && >+ !mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget)) { >+ goto out; >+ } > > skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo); > hwtstamp = mlx5e_cqe_ts_to_ns(sq->ptp_cyc2time, sq->clock, get_cqe_ts(cqe)); >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >index d5afad368a69..e599b86d94b5 100644 >--- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >@@ -295,13 +295,15 @@ static inline > void mlx5e_skb_fifo_push(struct mlx5e_skb_fifo *fifo, struct sk_buff *skb) > { > struct sk_buff **skb_item = mlx5e_skb_fifo_get(fifo, (*fifo->pc)++); >- redundant change. > *skb_item = skb; > } > > static inline > struct sk_buff *mlx5e_skb_fifo_pop(struct mlx5e_skb_fifo *fifo) > { >+ if (*fifo->pc == *fifo->cc) >+ return NULL; >+ I think this won't be necessary if you check for ooo early on mlx5e_ptp_handle_ts_cqe() like i suggested above. > return *mlx5e_skb_fifo_get(fifo, (*fifo->cc)++); > } > >-- >2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 18:19 ` Saeed Mahameed @ 2023-02-01 21:36 ` Vadim Fedorenko 2023-02-01 23:40 ` Saeed Mahameed 0 siblings, 1 reply; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-01 21:36 UTC (permalink / raw) To: Saeed Mahameed, Vadim Fedorenko Cc: Jakub Kicinski, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, netdev On 01/02/2023 18:19, Saeed Mahameed wrote: > On 01 Feb 04:26, Vadim Fedorenko wrote: >> Fifo indexes were not checked during pop operations and it leads to >> potential use-after-free when poping from empty queue. Such case was >> possible during re-sync action. >> >> There were out-of-order cqe spotted which lead to drain of the queue and >> use-after-free because of lack of fifo pointers check. Special check >> is added to avoid resync operation if SKB could not exist in the fifo >> because of OOO cqe (skb_id must be between consumer and producer index). >> >> Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port >> timestamp") >> Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >> --- >> .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 23 ++++++++++++++----- >> .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 4 +++- >> 2 files changed, 20 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >> b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >> index b72de2b520ec..5df726185192 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >> @@ -86,7 +86,7 @@ static bool mlx5e_ptp_ts_cqe_drop(struct mlx5e_ptpsq >> *ptpsq, u16 skb_cc, u16 skb >> return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); >> } >> >> -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >> *ptpsq, u16 skb_cc, >> +static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >> *ptpsq, u16 skb_cc, >> u16 skb_id, int budget) >> { >> struct skb_shared_hwtstamps hwts = {}; >> @@ -94,14 +94,23 @@ static void >> mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 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) { > > To avoid returning boolean and add more functionality to this function, > I prefer to put this check in mlx5e_ptp_handle_ts_cqe(), see below. > Let's discuss this point, see comments below. >> + mlx5_core_err_rl(ptpsq->txqsq.mdev, "out-of-order ptp cqe\n"); > > it's better to add a counter for this, eg: ptpsq->cq_stats->ooo_cqe_drop++; Sure, will do. > >> + return false; >> + } >> + >> + while (skb_cc != skb_id && (skb = >> mlx5e_skb_fifo_pop(&ptpsq->skb_fifo))) { >> hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; >> skb_tstamp_tx(skb, &hwts); >> ptpsq->cq_stats->resync_cqe++; >> napi_consume_skb(skb, budget); >> skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >> } >> + >> + if (!skb) >> + return false; >> + >> + return true; >> } >> >> static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, >> @@ -111,7 +120,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct >> mlx5e_ptpsq *ptpsq, >> u16 skb_id = PTP_WQE_CTR2IDX(be16_to_cpu(cqe->wqe_counter)); >> u16 skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >> struct mlx5e_txqsq *sq = &ptpsq->txqsq; >> - struct sk_buff *skb; >> + struct sk_buff *skb = NULL; >> ktime_t hwtstamp; >> >> if (unlikely(MLX5E_RX_ERR_CQE(cqe))) { >> @@ -120,8 +129,10 @@ static void mlx5e_ptp_handle_ts_cqe(struct >> mlx5e_ptpsq *ptpsq, >> goto out; >> } >> >> - if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) >> - mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget); > > you can check here: > /* ignore ooo cqe as it was already handled by a previous resync */ > if (ooo_cqe(cqe)) > return; I assume that mlx5e_ptp_skb_fifo_ts_cqe_resync() is error-recovery path and should not happen frequently. If we move this check to mlx5e_ptp_handle_ts_cqe() we will have additional if with 2 checks for every cqe coming from ptp queue - the fast path. With our load of 1+Mpps it could be countable. Another point is that mlx5e_ptp_skb_fifo_ts_cqe_resync() will assume that skb_id must always be within fifo indexes and any other (future?) code has to implement this check. Otherwise it will cause use-after-free, double-free and then crash, especially if we remove check in mlx5e_skb_fifo_pop() that you commented below. I think it's ok to have additional check in error path to prevent anything like that in the future. If you stongly against converting mlx5e_ptp_skb_fifo_ts_cqe_resync() to return bool, I can add the check to 'if mlx5e_ptp_ts_cqe_drop()' scope before calling resync(), but it will not remove problems from my second point and I just would like to see explicit 'yes, we agree to have dangerous code in our driver' from you or other maintainers in this case. >> + if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id) && >> + !mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, >> budget)) { >> + goto out; >> + } >> >> skb = mlx5e_skb_fifo_pop(&ptpsq->skb_fifo); >> hwtstamp = mlx5e_cqe_ts_to_ns(sq->ptp_cyc2time, sq->clock, >> get_cqe_ts(cqe)); >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> index d5afad368a69..e599b86d94b5 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> @@ -295,13 +295,15 @@ static inline >> void mlx5e_skb_fifo_push(struct mlx5e_skb_fifo *fifo, struct sk_buff >> *skb) >> { >> struct sk_buff **skb_item = mlx5e_skb_fifo_get(fifo, (*fifo->pc)++); >> - > > redundant change. yep, will remove this artefact. > >> *skb_item = skb; >> } >> >> static inline >> struct sk_buff *mlx5e_skb_fifo_pop(struct mlx5e_skb_fifo *fifo) >> { >> + if (*fifo->pc == *fifo->cc) >> + return NULL; >> + > > I think this won't be necessary if you check for ooo early on > mlx5e_ptp_handle_ts_cqe() like i suggested above. > And again the only concern here is that we don't have any checks whether FIFO has anything to pop before actually poping the value. That can easily bring use-after-free in the futuee, especially because the function is not ptp specific and is already used for other fifos. I can add unlikely() for this check if it helps, but I would like to have this check in the code. >> return *mlx5e_skb_fifo_get(fifo, (*fifo->cc)++); >> } >> >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 21:36 ` Vadim Fedorenko @ 2023-02-01 23:40 ` Saeed Mahameed 2023-02-02 1:34 ` Vadim Fedorenko 2023-02-02 3:01 ` Jakub Kicinski 0 siblings, 2 replies; 10+ messages in thread From: Saeed Mahameed @ 2023-02-01 23:40 UTC (permalink / raw) To: Vadim Fedorenko Cc: Vadim Fedorenko, Jakub Kicinski, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, netdev On 01 Feb 21:36, Vadim Fedorenko wrote: >On 01/02/2023 18:19, Saeed Mahameed wrote: >>On 01 Feb 04:26, Vadim Fedorenko wrote: >>>Fifo indexes were not checked during pop operations and it leads to >>>potential use-after-free when poping from empty queue. Such case was >>>possible during re-sync action. >>> >>>There were out-of-order cqe spotted which lead to drain of the queue and >>>use-after-free because of lack of fifo pointers check. Special check >>>is added to avoid resync operation if SKB could not exist in the fifo >>>because of OOO cqe (skb_id must be between consumer and producer index). >>> >>>Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port >>>timestamp") >>>Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >>>--- >>>.../net/ethernet/mellanox/mlx5/core/en/ptp.c | 23 ++++++++++++++----- >>>.../net/ethernet/mellanox/mlx5/core/en/txrx.h | 4 +++- >>>2 files changed, 20 insertions(+), 7 deletions(-) >>> >>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>index b72de2b520ec..5df726185192 100644 >>>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>@@ -86,7 +86,7 @@ static bool mlx5e_ptp_ts_cqe_drop(struct >>>mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb >>> return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); >>>} >>> >>>-static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >>>*ptpsq, u16 skb_cc, >>>+static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >>>*ptpsq, u16 skb_cc, >>> u16 skb_id, int budget) >>>{ >>> struct skb_shared_hwtstamps hwts = {}; >>>@@ -94,14 +94,23 @@ static void >>>mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 >>>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) { >> >>To avoid returning boolean and add more functionality to this function, >>I prefer to put this check in mlx5e_ptp_handle_ts_cqe(), see below. >> > >Let's discuss this point, see comments below. > >>>+ mlx5_core_err_rl(ptpsq->txqsq.mdev, "out-of-order ptp cqe\n"); >> >>it's better to add a counter for this, eg: ptpsq->cq_stats->ooo_cqe_drop++; > >Sure, will do. > >> >>>+ return false; >>>+ } >>>+ >>>+ while (skb_cc != skb_id && (skb = >>>mlx5e_skb_fifo_pop(&ptpsq->skb_fifo))) { >>> hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; >>> skb_tstamp_tx(skb, &hwts); >>> ptpsq->cq_stats->resync_cqe++; >>> napi_consume_skb(skb, budget); >>> skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >>> } >>>+ >>>+ if (!skb) >>>+ return false; >>>+ >>>+ return true; >>>} >>> >>>static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, >>>@@ -111,7 +120,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct >>>mlx5e_ptpsq *ptpsq, >>> u16 skb_id = PTP_WQE_CTR2IDX(be16_to_cpu(cqe->wqe_counter)); >>> u16 skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >>> struct mlx5e_txqsq *sq = &ptpsq->txqsq; >>>- struct sk_buff *skb; >>>+ struct sk_buff *skb = NULL; >>> ktime_t hwtstamp; >>> >>> if (unlikely(MLX5E_RX_ERR_CQE(cqe))) { >>>@@ -120,8 +129,10 @@ static void mlx5e_ptp_handle_ts_cqe(struct >>>mlx5e_ptpsq *ptpsq, >>> goto out; >>> } >>> >>>- if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) >>>- mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, budget); >> >>you can check here: >> /* ignore ooo cqe as it was already handled by a previous resync */ >> if (ooo_cqe(cqe)) >> return; > >I assume that mlx5e_ptp_skb_fifo_ts_cqe_resync() is error-recovery >path and should not happen frequently. If we move this check to >mlx5e_ptp_handle_ts_cqe() we will have additional if with 2 checks for >every cqe coming from ptp queue - the fast path. With our load of we could do: if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) { if (ooo_cqe) /* already handled by a previous resync */ return; mlx5e_ptp_skb_fifo_ts_cqe_resync(..); } >1+Mpps it could be countable. Another point is that >mlx5e_ptp_skb_fifo_ts_cqe_resync() will assume that skb_id must always >be within fifo indexes and any other (future?) code has to implement >this check. Otherwise it will cause use-after-free, double-free and >then crash, especially if we remove check in mlx5e_skb_fifo_pop() that >you commented below. I think it's ok to have additional check in error >path to prevent anything like that in the future. > >If you stongly against converting mlx5e_ptp_skb_fifo_ts_cqe_resync() >to return bool, I can add the check to 'if mlx5e_ptp_ts_cqe_drop()' >scope before calling resync(), but it will not remove problems from my >second point and I just would like to see explicit 'yes, we agree to >have dangerous code in our driver' from you or other maintainers in what do you mean ? define dangerous.. we don't willingly push dangerous code :) the code is built and designed for the current HW spec under the assumption that HW/FW is bug free, otherwise what's the point of the spec if we are going to write doubtful, inefficient, paranoid driver code.. I understand your concern but we don't design data path code to be future proof. This patch is just a temporary fix for another underlying issue that we need to continue debug. so let's keep it minimal until we find the real issue. keep in mind that the whole code is designed with fifos and only in-order queues guaranteed by both HW and Firmware, so there's no reason to be too-paranoid .. just fix the known bugs :). >>static inline >>>struct sk_buff *mlx5e_skb_fifo_pop(struct mlx5e_skb_fifo *fifo) >>>{ >>>+ if (*fifo->pc == *fifo->cc) >>>+ return NULL; >>>+ >> >>I think this won't be necessary if you check for ooo early on >>mlx5e_ptp_handle_ts_cqe() like i suggested above. >> >And again the only concern here is that we don't have any checks >whether FIFO has anything to pop before actually poping the value. >That can easily bring use-after-free in the futuee, especially because >the function is not ptp specific and is already used for other fifos. >I can add unlikely() for this check if it helps, but I would like to >have this check in the code. > you shouldn't access the fifo if by design it's guaranteed nothing is there. We don't build for a future/fool proof code, the fifo is only accessed when we know there's something there by design, this is not a general purpose fifo, it's a fifo used by mlx5 ordered cqs.. According to your logic, kfree should also check for double free.. ? :) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 23:40 ` Saeed Mahameed @ 2023-02-02 1:34 ` Vadim Fedorenko 2023-02-02 3:01 ` Jakub Kicinski 1 sibling, 0 replies; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-02 1:34 UTC (permalink / raw) To: Saeed Mahameed Cc: Vadim Fedorenko, Jakub Kicinski, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, netdev On 01/02/2023 23:40, Saeed Mahameed wrote: > On 01 Feb 21:36, Vadim Fedorenko wrote: >> On 01/02/2023 18:19, Saeed Mahameed wrote: >>> On 01 Feb 04:26, Vadim Fedorenko wrote: >>>> Fifo indexes were not checked during pop operations and it leads to >>>> potential use-after-free when poping from empty queue. Such case was >>>> possible during re-sync action. >>>> >>>> There were out-of-order cqe spotted which lead to drain of the queue >>>> and >>>> use-after-free because of lack of fifo pointers check. Special check >>>> is added to avoid resync operation if SKB could not exist in the fifo >>>> because of OOO cqe (skb_id must be between consumer and producer >>>> index). >>>> >>>> Fixes: 58a518948f60 ("net/mlx5e: Add resiliency for PTP TX port >>>> timestamp") >>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >>>> --- >>>> .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 23 ++++++++++++++----- >>>> .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 4 +++- >>>> 2 files changed, 20 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>> b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>> index b72de2b520ec..5df726185192 100644 >>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c >>>> @@ -86,7 +86,7 @@ static bool mlx5e_ptp_ts_cqe_drop(struct >>>> mlx5e_ptpsq *ptpsq, u16 skb_cc, u16 skb >>>> return (ptpsq->ts_cqe_ctr_mask && (skb_cc != skb_id)); >>>> } >>>> >>>> -static void mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >>>> *ptpsq, u16 skb_cc, >>>> +static bool mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq >>>> *ptpsq, u16 skb_cc, >>>> u16 skb_id, int budget) >>>> { >>>> struct skb_shared_hwtstamps hwts = {}; >>>> @@ -94,14 +94,23 @@ static void >>>> mlx5e_ptp_skb_fifo_ts_cqe_resync(struct mlx5e_ptpsq *ptpsq, u16 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) { >>> >>> To avoid returning boolean and add more functionality to this function, >>> I prefer to put this check in mlx5e_ptp_handle_ts_cqe(), see below. >>> >> >> Let's discuss this point, see comments below. >> >>>> + mlx5_core_err_rl(ptpsq->txqsq.mdev, "out-of-order ptp cqe\n"); >>> >>> it's better to add a counter for this, eg: >>> ptpsq->cq_stats->ooo_cqe_drop++; >> >> Sure, will do. >> >>> >>>> + return false; >>>> + } >>>> + >>>> + while (skb_cc != skb_id && (skb = >>>> mlx5e_skb_fifo_pop(&ptpsq->skb_fifo))) { >>>> hwts.hwtstamp = mlx5e_skb_cb_get_hwts(skb)->cqe_hwtstamp; >>>> skb_tstamp_tx(skb, &hwts); >>>> ptpsq->cq_stats->resync_cqe++; >>>> napi_consume_skb(skb, budget); >>>> skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >>>> } >>>> + >>>> + if (!skb) >>>> + return false; >>>> + >>>> + return true; >>>> } >>>> >>>> static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq, >>>> @@ -111,7 +120,7 @@ static void mlx5e_ptp_handle_ts_cqe(struct >>>> mlx5e_ptpsq *ptpsq, >>>> u16 skb_id = PTP_WQE_CTR2IDX(be16_to_cpu(cqe->wqe_counter)); >>>> u16 skb_cc = PTP_WQE_CTR2IDX(ptpsq->skb_fifo_cc); >>>> struct mlx5e_txqsq *sq = &ptpsq->txqsq; >>>> - struct sk_buff *skb; >>>> + struct sk_buff *skb = NULL; >>>> ktime_t hwtstamp; >>>> >>>> if (unlikely(MLX5E_RX_ERR_CQE(cqe))) { >>>> @@ -120,8 +129,10 @@ static void mlx5e_ptp_handle_ts_cqe(struct >>>> mlx5e_ptpsq *ptpsq, >>>> goto out; >>>> } >>>> >>>> - if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) >>>> - mlx5e_ptp_skb_fifo_ts_cqe_resync(ptpsq, skb_cc, skb_id, >>>> budget); >>> >>> you can check here: >>> /* ignore ooo cqe as it was already handled by a previous resync */ >>> if (ooo_cqe(cqe)) >>> return; >> >> I assume that mlx5e_ptp_skb_fifo_ts_cqe_resync() is error-recovery >> path and should not happen frequently. If we move this check to >> mlx5e_ptp_handle_ts_cqe() we will have additional if with 2 checks for >> every cqe coming from ptp queue - the fast path. With our load of > > we could do: > > if (mlx5e_ptp_ts_cqe_drop(ptpsq, skb_cc, skb_id)) > { > if (ooo_cqe) /* already handled by a previous resync */ > return; > mlx5e_ptp_skb_fifo_ts_cqe_resync(..); > } > Yep, that's one of the options I suggested. Ok, let's do it this way. >> 1+Mpps it could be countable. Another point is that >> mlx5e_ptp_skb_fifo_ts_cqe_resync() will assume that skb_id must always >> be within fifo indexes and any other (future?) code has to implement >> this check. Otherwise it will cause use-after-free, double-free and >> then crash, especially if we remove check in mlx5e_skb_fifo_pop() that >> you commented below. I think it's ok to have additional check in error >> path to prevent anything like that in the future. >> >> If you stongly against converting mlx5e_ptp_skb_fifo_ts_cqe_resync() >> to return bool, I can add the check to 'if mlx5e_ptp_ts_cqe_drop()' >> scope before calling resync(), but it will not remove problems from my >> second point and I just would like to see explicit 'yes, we agree to >> have dangerous code in our driver' from you or other maintainers in > > what do you mean ? define dangerous.. > we don't willingly push dangerous code :) the code is built and designed > for the current HW spec under the assumption that HW/FW is bug free, > otherwise what's the point of the spec if we are going to write doubtful, > inefficient, paranoid driver code.. If FW/HW is bug free - yes, I agree. But the real world is not that perfect. And I believe that's the reason why kfifo implementation has all these checks in place. > I understand your concern but we don't design data path code to be future > proof. > > This patch is just a temporary fix for another underlying issue that > we need to continue debug. so let's keep it minimal until we find the > real issue. > Yeah, with minimal changes we will definitely revisit this code once we find a root cause of the issue. > keep in mind that the whole code is designed with fifos and only in-order > queues guaranteed by both HW and Firmware, so there's no reason to be > too-paranoid .. just fix the known bugs :). > TBH, I simply don't want to spend more days debugging unclear kernel crashes if/once we hit another FW/HW bug. It easier to debug issue when kernel continues to run. But anyway, let's re-think it once we have root cause of the issue. I'll publish v5 next day, thanks for the review! >>> static inline >>>> struct sk_buff *mlx5e_skb_fifo_pop(struct mlx5e_skb_fifo *fifo) >>>> { >>>> + if (*fifo->pc == *fifo->cc) >>>> + return NULL; >>>> + >>> >>> I think this won't be necessary if you check for ooo early on >>> mlx5e_ptp_handle_ts_cqe() like i suggested above. >>> >> And again the only concern here is that we don't have any checks >> whether FIFO has anything to pop before actually poping the value. >> That can easily bring use-after-free in the futuee, especially because >> the function is not ptp specific and is already used for other fifos. >> I can add unlikely() for this check if it helps, but I would like to >> have this check in the code. >> > > you shouldn't access the fifo if by design it's guaranteed nothing is > there. > We don't build for a future/fool proof code, the fifo is only accessed > when we know there's something there by design, this is not a general > purpose fifo, it's a fifo used by mlx5 ordered cqs.. Got it. > According to your logic, kfree should also check for double free.. ? :) Such kfree will have unacceptable performance regressions, but I believe we have something like this in debug kernels :) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 23:40 ` Saeed Mahameed 2023-02-02 1:34 ` Vadim Fedorenko @ 2023-02-02 3:01 ` Jakub Kicinski 1 sibling, 0 replies; 10+ messages in thread From: Jakub Kicinski @ 2023-02-02 3:01 UTC (permalink / raw) To: Saeed Mahameed Cc: Vadim Fedorenko, Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, netdev On Wed, 1 Feb 2023 15:40:00 -0800 Saeed Mahameed wrote: > >And again the only concern here is that we don't have any checks > >whether FIFO has anything to pop before actually poping the value. > >That can easily bring use-after-free in the futuee, especially because > >the function is not ptp specific and is already used for other fifos. > >I can add unlikely() for this check if it helps, but I would like to > >have this check in the code. > > you shouldn't access the fifo if by design it's guaranteed nothing is there. > We don't build for a future/fool proof code, the fifo is only accessed > when we know there's something there by design, this is not a general > purpose fifo, it's a fifo used by mlx5 ordered cqs.. The check for fifo being empty seems 100% sane to me. You can put a WARN_ON_ONCE() on it if you believe it can never happen. But the cost of dealing with random double frees is much higher than a single conditional on not-so-fast path. > According to your logic, kfree should also check for double free.. ? :) I reckon we'd happily make kfree check for double free if there was an efficient way of doing that. Various large companies build their production kernels with KFENCE enabled, AFAIK. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-01 12:26 ` [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free Vadim Fedorenko 2023-02-01 18:19 ` Saeed Mahameed @ 2023-02-02 3:08 ` Jakub Kicinski 2023-02-02 11:48 ` Vadim Fedorenko 1 sibling, 1 reply; 10+ messages in thread From: Jakub Kicinski @ 2023-02-02 3:08 UTC (permalink / raw) To: Vadim Fedorenko Cc: Vadim Fedorenko, Rahul Rameshbabu, Tariq Toukan, Gal Pressman, Saeed Mahameed, netdev On Wed, 1 Feb 2023 04:26:05 -0800 Vadim Fedorenko wrote: > + if (skb_cc > skb_id || PTP_WQE_CTR2IDX(ptpsq->skb_fifo_pc) < skb_id) { FWIW I still can't understand why this is correct. If we lose ts for the last elem before wrap we'll see something like (assume wrap at 256 for easier math): cc: 255 pc: 2 skb_id: 0 => cc > skb_id, OOO, drop cc: 255 pc: 2 skb_id: 1 => cc > skb_id, OOO, drop cc: 255 pc: 3 // produce cc: 255 pc: 3 skb_id: 2 => cc > skb_id, OOO, drop cc: 255 pc: 4 // produce cc: 255 pc: 4 skb_id: 3 => cc > skb_id, OOO, drop No? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free 2023-02-02 3:08 ` Jakub Kicinski @ 2023-02-02 11:48 ` Vadim Fedorenko 0 siblings, 0 replies; 10+ messages in thread From: Vadim Fedorenko @ 2023-02-02 11:48 UTC (permalink / raw) To: Jakub Kicinski, Vadim Fedorenko Cc: Rahul Rameshbabu, Tariq Toukan, Gal Pressman, Saeed Mahameed, netdev On 02/02/2023 03:08, Jakub Kicinski wrote: > On Wed, 1 Feb 2023 04:26:05 -0800 Vadim Fedorenko wrote: >> + if (skb_cc > skb_id || PTP_WQE_CTR2IDX(ptpsq->skb_fifo_pc) < skb_id) { > > FWIW I still can't understand why this is correct. If we lose ts for > the last elem before wrap we'll see something like (assume wrap at 256 > for easier math): > > cc: 255 pc: 2 skb_id: 0 => cc > skb_id, OOO, drop > cc: 255 pc: 2 skb_id: 1 => cc > skb_id, OOO, drop > cc: 255 pc: 3 // produce > cc: 255 pc: 3 skb_id: 2 => cc > skb_id, OOO, drop > cc: 255 pc: 4 // produce > cc: 255 pc: 4 skb_id: 3 => cc > skb_id, OOO, drop > > No? Agreed. I'll change the check in the next version, thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-02 11:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-01 12:26 [PATCH net v4 0/2] mlx5: ptp fifo bugfixes Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 1/2] mlx5: fix skb leak while fifo resync and push Vadim Fedorenko 2023-02-01 12:26 ` [PATCH net v4 2/2] mlx5: fix possible ptp queue fifo use-after-free Vadim Fedorenko 2023-02-01 18:19 ` Saeed Mahameed 2023-02-01 21:36 ` Vadim Fedorenko 2023-02-01 23:40 ` Saeed Mahameed 2023-02-02 1:34 ` Vadim Fedorenko 2023-02-02 3:01 ` Jakub Kicinski 2023-02-02 3:08 ` Jakub Kicinski 2023-02-02 11:48 ` Vadim Fedorenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).