2023-03-13 15:26:14

by Jakob Koschel

Subject: [PATCH net-next] net/mlx5e: avoid usage of list iterator after loop

If potentially no valid element is found, 'priv_rx' would contain an
invalid pointer past the iterator loop. To ensure 'priv_rx' is always
valid, we only set it if the correct element was found. That allows
adding a WARN_ON() in case the code works incorrectly, exposing
currently undetectable potential bugs.

Additionally, Linus proposed to avoid any use of the list iterator
variable after the loop, in the attempt to move the list iterator
variable declaration into the macro to avoid any potential misuse after
the loop [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
index 4be770443b0c..8aad500e622d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c
@@ -718,7 +718,7 @@ void mlx5e_ktls_del_rx(struct net_device *netdev, struct tls_context *tls_ctx)

bool mlx5e_ktls_rx_handle_resync_list(struct mlx5e_channel *c, int budget)
{
- struct mlx5e_ktls_offload_context_rx *priv_rx, *tmp;
+ struct mlx5e_ktls_offload_context_rx *priv_rx = NULL, *iter, *tmp;
struct mlx5e_ktls_resync_resp *ktls_resync;
struct mlx5_wqe_ctrl_seg *db_cseg;
struct mlx5e_icosq *sq;
@@ -735,10 +735,12 @@ bool mlx5e_ktls_rx_handle_resync_list(struct mlx5e_channel *c, int budget)
i = 0;

spin_lock(&ktls_resync->lock);
- list_for_each_entry_safe(priv_rx, tmp, &ktls_resync->list, list) {
- list_move(&priv_rx->list, &local_list);
- if (++i == budget)
+ list_for_each_entry_safe(iter, tmp, &ktls_resync->list, list) {
+ list_move(&iter->list, &local_list);
+ if (++i == budget) {
+ priv_rx = iter;
break;
+ }
}
if (list_empty(&ktls_resync->list))
clear_bit(MLX5E_SQ_STATE_PENDING_TLS_RX_RESYNC, &sq->state);
@@ -765,6 +767,7 @@ bool mlx5e_ktls_rx_handle_resync_list(struct mlx5e_channel *c, int budget)
mlx5e_notify_hw(&sq->wq, sq->pc, sq->uar_map, db_cseg);
spin_unlock(&c->async_icosq_lock);

+ WARN_ON(!priv_rx);
priv_rx->rq_stats->tls_resync_res_ok += j;

if (!list_empty(&local_list)) {

---
base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
change-id: 20230301-net-mlx5e-avoid-iter-after-loop-dcc215275a96

Best regards,
--
Jakob Koschel <[email protected]>

2023-03-14 13:30:33

by Leon Romanovsky

Subject: Re: [PATCH net-next] net/mlx5e: avoid usage of list iterator after loop

On Mon, Mar 13, 2023 at 04:26:02PM +0100, Jakob Koschel wrote:
> If potentially no valid element is found, 'priv_rx' would contain an
> invalid pointer past the iterator loop. To ensure 'priv_rx' is always
> valid, we only set it if the correct element was found. That allows
> adding a WARN_ON() in case the code works incorrectly, exposing
> currently undetectable potential bugs.
>
> Additionally, Linus proposed to avoid any use of the list iterator
> variable after the loop, in the attempt to move the list iterator
> variable declaration into the macro to avoid any potential misuse after
> the loop [1].
>
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_rx.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <[email protected]>

2023-03-21 20:49:45

by Saeed Mahameed

Subject: Re: [PATCH net-next] net/mlx5e: avoid usage of list iterator after loop

On 13 Mar 16:26, Jakob Koschel wrote:
>If potentially no valid element is found, 'priv_rx' would contain an
>invalid pointer past the iterator loop. To ensure 'priv_rx' is always
>valid, we only set it if the correct element was found. That allows
>adding a WARN_ON() in case the code works incorrectly, exposing
>currently undetectable potential bugs.
>
>Additionally, Linus proposed to avoid any use of the list iterator
>variable after the loop, in the attempt to move the list iterator
>variable declaration into the macro to avoid any potential misuse after
>the loop [1].
>
>Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>Signed-off-by: Jakob Koschel <[email protected]>

Applied to net-next-mlx5.

Thanks.