mlx5 fixes 2023-11-13
From: Saeed Mahameed <[email protected]>
V1->v2:
- Toss two patches to comply with max of 15 patches rule.
This series provides bug fixes to mlx5 driver. Please pull and let me know if there is any problem.
Thanks,
Saeed.
The following changes since commit 4b7b492615cf3017190f55444f7016812b66611d:
af_unix: fix use-after-free in unix_stream_read_actor() (2023-11-14 10:51:13 +0100)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-fixes-2023-11-14
for you to fetch changes up to 67e715ed3c91d6704aa0e26664f3f01abaad699f:
net/mlx5e: Check return value of snprintf writing to fw_version buffer for representors (2023-11-14 13:56:18 -0800)
----------------------------------------------------------------
mlx5-fixes-2023-11-14
----------------------------------------------------------------
Dust Li (1):
net/mlx5e: fix double free of encap_header
Erez Shitrit (1):
net/mlx5: DR, Allow old devices to use multi destination FTE
Gavin Li (1):
net/mlx5e: fix double free of encap_header in update funcs
Itamar Gozlan (1):
Revert "net/mlx5: DR, Supporting inline WQE when possible"
Jianbo Liu (1):
net/mlx5e: Don't modify the peer sent-to-vport rules for IPSec offload
Maher Sanalla (1):
net/mlx5: Free used cpus mask when an IRQ is released
Rahul Rameshbabu (7):
net/mlx5: Decouple PHC .adjtime and .adjphase implementations
net/mlx5e: Avoid referencing skb after free-ing in drop path of mlx5e_sq_xmit_wqe net/mlx5e: Track xmit submission to PTP WQ after populating metadata map net/mlx5e: Update doorbell for port timestamping CQ before the software counter net/mlx5: Increase size of irq name buffer
net/mlx5e: Check return value of snprintf writing to fw_version buffer net/mlx5e: Check return value of snprintf writing to fw_version buffer for representors
Saeed Mahameed (1):
net/mlx5e: Reduce the size of icosq_str
Vlad Buslov (1):
net/mlx5e: Fix pedit endianness
drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c | 20 +++- .../ethernet/mellanox/mlx5/core/en/reporter_rx.c | 4 +- .../net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 30 +++--- .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 13 ++- drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 12 ++- drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 60 ++++++----- drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 4 +- drivers/net/ethernet/mellanox/mlx5/core/eq.c | 25 +++-- .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 3 +- .../net/ethernet/mellanox/mlx5/core/irq_affinity.c | 42 -------- .../net/ethernet/mellanox/mlx5/core/lib/clock.c | 7 +- drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c | 6 +- drivers/net/ethernet/mellanox/mlx5/core/pci_irq.h | 3 + .../mellanox/mlx5/core/steering/dr_action.c | 3 +- .../ethernet/mellanox/mlx5/core/steering/dr_send.c | 115 +++------------------ 15 files changed, 131 insertions(+), 216 deletions(-)
PATCH v2 1/15 Revert "net/mlx5: DR, Supporting inline WQE when possible"
From: Itamar Gozlan <[email protected]>
This reverts commit 95c337cce0e11d06a715da73e6796ade9216637f. The revert is required due to the suspicion it cause some tests fail and will be moved to further investigation.
--- .../mellanox/mlx5/core/steering/dr_send.c | 115 ++---------------- 1 file changed, 13 insertions(+), 102 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/steering/dr_send.c +13 -102
@@ -52,7 +52,6 @@ struct dr_qp_init_attr {u32 cqn;u32 pdn;u32 max_send_wr; -u32 max_send_sge;struct mlx5_uars_page *uar; u8 isolate_vl_tc:1; };
@@ -247,37 +246,6 @@ static int dr_poll_cq(struct mlx5dr_cq *dr_cq, int ne)return err ==CQ_POLL_ERR ? err : npolled; } -static int dr_qp_get_args_update_send_wqe_size (struct dr_qp_init_attr *attr) -{ -return roundup_pow_of_two (sizeof (struct mlx5_wqe_ctrl_seg ) + -sizeof (struct mlx5_wqe_flow_update_ctrl_seg ) + -sizeof (struct mlx5_wqe_header_modify_argument_update_seg )); -} - -/* We calculate for specific RC QP with the required functionality */ -static int dr_qp_calc_rc_send_wqe (struct dr_qp_init_attr *attr) -{ -int update_arg_size; -int inl_size =0 ; -int tot_size; -int size; - - update_arg_size =dr_qp_get_args_update_send_wqe_size (attr); - - size =sizeof (struct mlx5_wqe_ctrl_seg ) + -sizeof (struct mlx5_wqe_raddr_seg ); - inl_size = size +ALIGN (sizeof (struct mlx5_wqe_inline_seg ) + -DR_STE_SIZE ,16 ); - - size += attr->max_send_sge *sizeof (struct mlx5_wqe_data_seg ); - - size =max (size, update_arg_size); - - tot_size =max (size, inl_size); - -return ALIGN (tot_size,MLX5_SEND_WQE_BB ); -} -static struct mlx5dr_qp *dr_create_rc_qp (struct mlx5_core_dev *mdev,struct dr_qp_init_attr *attr) {
@@ -285,7 +253,6 @@ static struct mlx5dr_qp *dr_create_rc_qp(struct mlx5_core_dev *mdev,u32 temp_qpc[MLX5_ST_SZ_DW (qpc)] = {};struct mlx5_wq_param wqp;struct mlx5dr_qp *dr_qp; -int wqe_size;int inlen;void *qpc;void *in;
@@ -365,15 +332,6 @@ static struct mlx5dr_qp *dr_create_rc_qp(struct mlx5_core_dev *mdev,if (err) goto err_in; dr_qp->uar = attr->uar; - wqe_size =dr_qp_calc_rc_send_wqe (attr); - dr_qp->max_inline_data =min (wqe_size - - (sizeof (struct mlx5_wqe_ctrl_seg ) + -sizeof (struct mlx5_wqe_raddr_seg ) + -sizeof (struct mlx5_wqe_inline_seg )), - (2 *MLX5_SEND_WQE_BB - - (sizeof (struct mlx5_wqe_ctrl_seg ) + -sizeof (struct mlx5_wqe_raddr_seg ) + -sizeof (struct mlx5_wqe_inline_seg ))));return dr_qp;
@@ -437,48 +395,8 @@ dr_rdma_handle_flow_access_arg_segments(struct mlx5_wqe_ctrl_seg *wq_ctrl,MLX5_SEND_WQE_DS ; } -static int dr_set_data_inl_seg (struct mlx5dr_qp *dr_qp, -struct dr_data_seg *data_seg,void *wqe) -{ -int inline_header_size =sizeof (struct mlx5_wqe_ctrl_seg ) + -sizeof (struct mlx5_wqe_raddr_seg ) + -sizeof (struct mlx5_wqe_inline_seg ); -struct mlx5_wqe_inline_seg *seg; -int left_space; -int inl =0 ; -void *addr; -int len; -int idx; - - seg = wqe; - wqe +=sizeof (*seg); - addr = (void *)(unsigned long )(data_seg->addr); - len = data_seg->length; - inl += len; - left_space =MLX5_SEND_WQE_BB - inline_header_size; - -if (likely (len > left_space)) { -memcpy (wqe, addr, left_space); - len -= left_space; - addr += left_space; - idx = (dr_qp->sq.pc +1 ) & (dr_qp->sq.wqe_cnt -1 ); - wqe =mlx5_wq_cyc_get_wqe (&dr_qp->wq.sq, idx); - } - -memcpy (wqe, addr, len); - -if (likely (inl)) { - seg->byte_count =cpu_to_be32 (inl |MLX5_INLINE_SEG ); -return DIV_ROUND_UP (inl +sizeof (seg->byte_count), -MLX5_SEND_WQE_DS ); - }else { -return 0 ; - } -} - static void -dr_rdma_handle_icm_write_segments(struct mlx5dr_qp *dr_qp, - struct mlx5_wqe_ctrl_seg *wq_ctrl, +dr_rdma_handle_icm_write_segments(struct mlx5_wqe_ctrl_seg *wq_ctrl, u64 remote_addr, u32 rkey, struct dr_data_seg *data_seg,
@@ -494,17 +412,15 @@ dr_rdma_handle_icm_write_segments(struct mlx5dr_qp *dr_qp, wq_raddr->reserved =0 ; wq_dseg = (void *)(wq_raddr +1 ); -/* WQE ctrl segment + WQE remote addr segment */ - *size = (sizeof (*wq_ctrl) +sizeof (*wq_raddr)) /MLX5_SEND_WQE_DS ; -if (data_seg->send_flags &IB_SEND_INLINE ) { - *size +=dr_set_data_inl_seg (dr_qp, data_seg, wq_dseg); - }else { - wq_dseg->byte_count =cpu_to_be32 (data_seg->length); - wq_dseg->lkey =cpu_to_be32 (data_seg->lkey); - wq_dseg->addr =cpu_to_be64 (data_seg->addr); - *size +=sizeof (*wq_dseg) /MLX5_SEND_WQE_DS ;/* WQE data segment */ - } + wq_dseg->byte_count =cpu_to_be32 (data_seg->length); + wq_dseg->lkey =cpu_to_be32 (data_seg->lkey); + wq_dseg->addr =cpu_to_be64 (data_seg->addr); + + *size = (sizeof (*wq_ctrl) +/* WQE ctrl segment */ +sizeof (*wq_dseg) +/* WQE data segment */ +sizeof (*wq_raddr)) //* WQE remote addr segment */ +MLX5_SEND_WQE_DS ; } static void dr_set_ctrl_seg(struct mlx5_wqe_ctrl_seg *wq_ctrl,
@@ -535,7 +451,7 @@ static void dr_rdma_segments(struct mlx5dr_qp *dr_qp, u64 remote_addr,switch (opcode) {case MLX5_OPCODE_RDMA_READ :case MLX5_OPCODE_RDMA_WRITE : -dr_rdma_handle_icm_write_segments (dr_qp, wq_ctrl, remote_addr, +dr_rdma_handle_icm_write_segments (wq_ctrl, remote_addr, rkey, data_seg, &size);break ;case MLX5_OPCODE_FLOW_TBL_ACCESS :
@@ -656,7 +572,7 @@ static void dr_fill_write_args_segs(struct mlx5dr_send_ring *send_ring,if (send_ring->pending_wqe % send_ring->signal_th ==0 ) send_info->write.send_flags |=IB_SEND_SIGNALED ;else - send_info->write.send_flags &= ~IB_SEND_SIGNALED ; + send_info->write.send_flags =0 ; } static void dr_fill_write_icm_segs(struct mlx5dr_domain *dmn,
@@ -680,13 +596,9 @@ static void dr_fill_write_icm_segs(struct mlx5dr_domain *dmn, } send_ring->pending_wqe++; -if (!send_info->write.lkey) - send_info->write.send_flags |=IB_SEND_INLINE ;if (send_ring->pending_wqe % send_ring->signal_th ==0 ) send_info->write.send_flags |=IB_SEND_SIGNALED ; -else - send_info->write.send_flags &= ~IB_SEND_SIGNALED ; send_ring->pending_wqe++; send_info->read.length = send_info->write.length;
@@ -696,9 +608,9 @@ static void dr_fill_write_icm_segs(struct mlx5dr_domain *dmn, send_info->read.lkey = send_ring->sync_mr->mkey;if (send_ring->pending_wqe % send_ring->signal_th ==0 ) - send_info->read.send_flags |=IB_SEND_SIGNALED ; + send_info->read.send_flags =IB_SEND_SIGNALED ;else - send_info->read.send_flags &= ~IB_SEND_SIGNALED ; + send_info->read.send_flags =0 ; } static void dr_fill_data_segs(struct mlx5dr_domain *dmn,
@@ -1345,7 +1257,6 @@ int mlx5dr_send_ring_alloc(struct mlx5dr_domain *dmn) dmn->send_ring->cq->qp = dmn->send_ring->qp; dmn->info.max_send_wr =QUEUE_SIZE ; - init_attr.max_send_sge =1 ; dmn->info.max_inline_size =min (dmn->send_ring->qp->max_inline_data,DR_STE_SIZE );
Signature · 1 line
2.41.0
Re: [net V2 01/15] Revert "net/mlx5: DR, Supporting inline WQE when possible"
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 11 lines · On Tue, 14 Nov 2023 13:58:32 -0800 you wrote:
From: Itamar Gozlan <[email protected]>
This reverts commit
95c337cce0e11d06a715da73e6796ade9216637f. The revert is required due to the suspicion it cause some tests fail and will be moved to further investigation.Fixes 95c337cce0e1 ("net/mlx5: DR, Supporting inline WQE when possible")Signed-off-by Itamar Gozlan <[email protected]>Signed-off-by Saeed Mahameed <[email protected]>[...]
Here is the summary with links:
- [net,V2,01/15] Revert "net/mlx5: DR, Supporting inline WQE when possible"
https://git.kernel.org/netdev/net/c/df3aafe50185 - [net,V2,02/15] net/mlx5: Free used cpus mask when an IRQ is released https://git.kernel.org/netdev/net/c/7d2f74d1d438 - [net,V2,03/15] net/mlx5: DR, Allow old devices to use multi destination FTE https://git.kernel.org/netdev/net/c/ad4d82c3eacd - [net,V2,04/15] net/mlx5: Decouple PHC .adjtime and .adjphase implementations https://git.kernel.org/netdev/net/c/fd64fd13c49a - [net,V2,05/15] net/mlx5e: fix double free of encap_header https://git.kernel.org/netdev/net/c/6f9b1a073166 - [net,V2,06/15] net/mlx5e: fix double free of encap_header in update funcs https://git.kernel.org/netdev/net/c/3a4aa3cb8356 - [net,V2,07/15] net/mlx5e: Fix pedit endianness https://git.kernel.org/netdev/net/c/0c101a23ca7e - [net,V2,08/15] net/mlx5e: Don't modify the peer sent-to-vport rules for IPSec offload https://git.kernel.org/netdev/net/c/bdf788cf224f - [net,V2,09/15] net/mlx5e: Avoid referencing skb after free-ing in drop path of mlx5e_sq_xmit_wqe https://git.kernel.org/netdev/net/c/64f14d16eef1 - [net,V2,10/15] net/mlx5e: Track xmit submission to PTP WQ after populating metadata map https://git.kernel.org/netdev/net/c/7e3f3ba97e6c - [net,V2,11/15] net/mlx5e: Update doorbell for port timestamping CQ before the software counter https://git.kernel.org/netdev/net/c/92214be5979c - [net,V2,12/15] net/mlx5: Increase size of irq name buffer https://git.kernel.org/netdev/net/c/3338bebfc26a - [net,V2,13/15] net/mlx5e: Reduce the size of icosq_str https://git.kernel.org/netdev/net/c/dce94142842e - [net,V2,14/15] net/mlx5e: Check return value of snprintf writing to fw_version buffer https://git.kernel.org/netdev/net/c/41e63c2baa11 - [net,V2,15/15] net/mlx5e: Check return value of snprintf writing to fw_version buffer for representors https://git.kernel.org/netdev/net/c/1b2bd0c0264f
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
PATCH v2 2/15 net/mlx5: Free used cpus mask when an IRQ is released
In reply to Saeed MahameedFrom: Maher Sanalla <[email protected]>
Each EQ table maintains a cpumask of the already used CPUs that are mapped to IRQs to ensure that each IRQ gets mapped to a unique CPU.
However, on IRQ release, the said cpumask is not updated by clearing the CPU from the mask to allow future IRQ request, causing the following error when a SF is reloaded after it has utilized all CPUs for its IRQs:
mlx5_irq_affinity_request:135:(pid 306010): Didn't find a matching IRQ. err = -28
Thus, when releasing an IRQ, clear its mapped CPU from the used CPUs mask, to prevent the case described above.
While at it, move the used cpumask update to the EQ layer as it is more fitting and preserves symmetricity of the IRQ request/release API.
--- drivers/net/ethernet/mellanox/mlx5/core/eq.c | 25 ++++++++--- .../mellanox/mlx5/core/irq_affinity.c | 42 ------------------- 2 files changed, 19 insertions(+), 48 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/eq.c +19 -6
@@ -885,11 +885,14 @@ static void comp_irq_release_sf(struct mlx5_core_dev *dev, u16 vecidx) {struct mlx5_eq_table *table = dev->priv.eq_table;struct mlx5_irq *irq; +int cpu; irq =xa_load (&table->comp_irqs, vecidx);if (!irq)return ; + cpu =cpumask_first (mlx5_irq_get_affinity_mask (irq)); +cpumask_clear_cpu (cpu, &table->used_cpus);xa_erase (&table->comp_irqs, vecidx);mlx5_irq_affinity_irq_release (dev, irq); }
@@ -897,16 +900,26 @@ static void comp_irq_release_sf(struct mlx5_core_dev *dev, u16 vecidx)static int comp_irq_request_sf (struct mlx5_core_dev *dev,u16 vecidx) {struct mlx5_eq_table *table = dev->priv.eq_table; +struct mlx5_irq_pool *pool =mlx5_irq_pool_get (dev); +struct irq_affinity_desc af_desc = {};struct mlx5_irq *irq; - irq =mlx5_irq_affinity_irq_request_auto (dev, &table->used_cpus, vecidx); -if (IS_ERR (irq)) { -/* In case SF irq pool does not exist, fallback to the PF irqs*/ -if (PTR_ERR (irq) == -ENOENT ) -return comp_irq_request_pci (dev, vecidx); +/* In case SF irq pool does not exist, fallback to the PF irqs*/ +if (!mlx5_irq_pool_is_sf_pool (pool)) +return comp_irq_request_pci (dev, vecidx); + af_desc.is_managed =1 ; +cpumask_copy (&af_desc.mask, cpu_online_mask); +cpumask_andnot (&af_desc.mask, &af_desc.mask, &table->used_cpus); + irq =mlx5_irq_affinity_request (pool, &af_desc); +if (IS_ERR (irq))return PTR_ERR (irq); - } + +cpumask_or (&table->used_cpus, &table->used_cpus,mlx5_irq_get_affinity_mask (irq)); +mlx5_core_dbg (pool->dev,"IRQ %u mapped to cpu %*pbl, %u EQs on this irq\n" , +pci_irq_vector (dev->pdev,mlx5_irq_get_index (irq)), +cpumask_pr_args (mlx5_irq_get_affinity_mask (irq)), +mlx5_irq_read_locked (irq) /MLX5_EQ_REFS_PER_IRQ );return xa_err (xa_store (&table->comp_irqs, vecidx, irq,GFP_KERNEL )); }
drivers/net/ethernet/mellanox/mlx5/core/irq_affinity.c +0 -42
45 lines not on this page · raw message
Signature · 1 line
2.41.0
PATCH v2 3/15 net/mlx5: DR, Allow old devices to use multi destination FTE
In reply to Saeed MahameedFrom: Erez Shitrit <[email protected]>
The current check isn't aware of old devices that don't have the relevant FW capability. This patch allows multi destination FTE in old cards, as it was before this check.
--- drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/steering/dr_action.c +2 -1
@@ -57,7 +57,8 @@ static const char *dr_action_id_to_str(enum mlx5dr_action_type action_id)static bool mlx5dr_action_supp_fwd_fdb_multi_ft (struct mlx5_core_dev *dev) { -return (MLX5_CAP_ESW_FLOWTABLE (dev, fdb_multi_path_any_table_limit_regc) || +return (MLX5_CAP_GEN (dev, steering_format_version) <MLX5_STEERING_FORMAT_CONNECTX_6DX || +MLX5_CAP_ESW_FLOWTABLE (dev, fdb_multi_path_any_table_limit_regc) ||MLX5_CAP_ESW_FLOWTABLE (dev, fdb_multi_path_any_table)); }
Signature · 1 line
2.41.0
PATCH v2 4/15 net/mlx5: Decouple PHC .adjtime and .adjphase implementations
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
When running a phase adjustment operation, the free running clock should not be modified at all. The phase control keyword is intended to trigger an internal servo on the device that will converge to the provided delta. A free running counter cannot implement phase adjustment.
--- drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c +6 -1
@@ -384,7 +384,12 @@ static int mlx5_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)static int mlx5_ptp_adjphase (struct ptp_clock_info *ptp,s32 delta) { -return mlx5_ptp_adjtime (ptp, delta); + struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info); +struct mlx5_core_dev *mdev; + + mdev = container_of(clock, struct mlx5_core_dev, clock); + +return mlx5_ptp_adjtime_real_time (mdev, delta); }static int mlx5_ptp_freq_adj_real_time (struct mlx5_core_dev *mdev,long scaled_ppm)
Signature · 1 line
2.41.0
PATCH v2 5/15 net/mlx5e: fix double free of encap_header
In reply to Saeed MahameedFrom: Dust Li <[email protected]>
When mlx5_packet_reformat_alloc() fails, the encap_header allocated in mlx5e_tc_tun_create_header_ipv4{6} will be released within it. However, e->encap_header is already set to the previously freed encap_header before mlx5_packet_reformat_alloc(). As a result, the later mlx5e_encap_put() will free e->encap_header again, causing a double free issue.
mlx5e_encap_put()
--> mlx5e_encap_dealloc()
--> kfree(e->encap_header)This happens when cmd: MLX5_CMD_OP_ALLOC_PACKET_REFORMAT_CONTEXT fail.
This patch fix it by not setting e->encap_header until mlx5_packet_reformat_alloc() success.
--- drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c +4 -6
@@ -300,9 +300,6 @@ int mlx5e_tc_tun_create_header_ipv4(struct mlx5e_priv *priv,if (err) goto destroy_neigh_entry; - e->encap_size = ipv4_encap_size; - e->encap_header = encap_header; -if (!(nud_state &NUD_VALID )) {neigh_event_send (attr.n,NULL );/* the encap entry will be made valid on neigh update event
@@ -322,6 +319,8 @@ int mlx5e_tc_tun_create_header_ipv4(struct mlx5e_priv *priv, goto destroy_neigh_entry; } + e->encap_size = ipv4_encap_size; + e->encap_header = encap_header; e->flags |=MLX5_ENCAP_ENTRY_VALID ;mlx5e_rep_queue_neigh_stats_work (netdev_priv (attr.out_dev));mlx5e_route_lookup_ipv4_put (&attr);
@@ -568,9 +567,6 @@ int mlx5e_tc_tun_create_header_ipv6(struct mlx5e_priv *priv,if (err) goto destroy_neigh_entry; - e->encap_size = ipv6_encap_size; - e->encap_header = encap_header; -if (!(nud_state &NUD_VALID )) {neigh_event_send (attr.n,NULL );/* the encap entry will be made valid on neigh update event
@@ -590,6 +586,8 @@ int mlx5e_tc_tun_create_header_ipv6(struct mlx5e_priv *priv, goto destroy_neigh_entry; } + e->encap_size = ipv6_encap_size; + e->encap_header = encap_header; e->flags |=MLX5_ENCAP_ENTRY_VALID ;mlx5e_rep_queue_neigh_stats_work (netdev_priv (attr.out_dev));mlx5e_route_lookup_ipv6_put (&attr);
Signature · 1 line
2.41.0
Jakub Kicinski
maintainerRe: [net V2 05/15] net/mlx5e: fix double free of encap_header
Quoted 1 line · On Tue, 14 Nov 2023 13:58:36 -0800 Saeed Mahameed wrote:
Fixes d589e785baf5e("net/mlx5e: Allow concurrent creation of encap entries")
Applied from the list 'cause I had to fix this tag.
PATCH v2 6/15 net/mlx5e: fix double free of encap_header in update funcs
In reply to Saeed MahameedFrom: Gavin Li <[email protected]>
Follow up to the previous patch to fix the same issue for mlx5e_tc_tun_update_header_ipv4{6} when mlx5_packet_reformat_alloc() fails.
When mlx5_packet_reformat_alloc() fails, the encap_header allocated in mlx5e_tc_tun_update_header_ipv4{6} will be released within it. However, e->encap_header is already set to the previously freed encap_header before mlx5_packet_reformat_alloc(). As a result, the later mlx5e_encap_put() will free e->encap_header again, causing a double free issue.
mlx5e_encap_put()
--> mlx5e_encap_dealloc()
--> kfree(e->encap_header)This patch fix it by not setting e->encap_header until mlx5_packet_reformat_alloc() success.
--- .../ethernet/mellanox/mlx5/core/en/tc_tun.c | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c +10 -10
@@ -403,16 +403,12 @@ int mlx5e_tc_tun_update_header_ipv4(struct mlx5e_priv *priv,if (err) goto free_encap; - e->encap_size = ipv4_encap_size; -kfree (e->encap_header); - e->encap_header = encap_header; -if (!(nud_state &NUD_VALID )) {neigh_event_send (attr.n,NULL );/* the encap entry will be made valid on neigh update event * and not used before that. */ - goto release_neigh; + goto free_encap; }memset (&reformat_params,0 ,sizeof (reformat_params));
@@ -426,6 +422,10 @@ int mlx5e_tc_tun_update_header_ipv4(struct mlx5e_priv *priv, goto free_encap; } + e->encap_size = ipv4_encap_size; +kfree (e->encap_header); + e->encap_header = encap_header; + e->flags |=MLX5_ENCAP_ENTRY_VALID ;mlx5e_rep_queue_neigh_stats_work (netdev_priv (attr.out_dev));mlx5e_route_lookup_ipv4_put (&attr);
@@ -669,16 +669,12 @@ int mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv,if (err) goto free_encap; - e->encap_size = ipv6_encap_size; -kfree (e->encap_header); - e->encap_header = encap_header; -if (!(nud_state &NUD_VALID )) {neigh_event_send (attr.n,NULL );/* the encap entry will be made valid on neigh update event * and not used before that. */ - goto release_neigh; + goto free_encap; }memset (&reformat_params,0 ,sizeof (reformat_params));
@@ -692,6 +688,10 @@ int mlx5e_tc_tun_update_header_ipv6(struct mlx5e_priv *priv, goto free_encap; } + e->encap_size = ipv6_encap_size; +kfree (e->encap_header); + e->encap_header = encap_header; + e->flags |=MLX5_ENCAP_ENTRY_VALID ;mlx5e_rep_queue_neigh_stats_work (netdev_priv (attr.out_dev));mlx5e_route_lookup_ipv6_put (&attr);
Signature · 1 line
2.41.0
PATCH v2 7/15 net/mlx5e: Fix pedit endianness
In reply to Saeed MahameedFrom: Vlad Buslov <[email protected]>
Referenced commit addressed endianness issue in mlx5 pedit implementation in ad hoc manner instead of systematically treating integer values according to their types which left pedit fields of sizes not equal to 4 and where the bytes being modified are not least significant ones broken on big endian machines since wrong bits will be consumed during parsing which leads to following example error when applying pedit to source and destination MAC addresses:
[Wed Oct 18 12:52:42 2023] mlx5_core 0001:00:00.1 p1v3_r: attempt to offload an unsupported field (cmd 0) [Wed Oct 18 12:52:42 2023] mask: 00000000330c5b68: 00 00 00 00 ff ff 00 00 00 00 ff ff 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 0000000017d22fd9: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 000000008186d717: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 0000000029eb6149: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 000000007ed103e4: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 00000000db8101a6: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [Wed Oct 18 12:52:42 2023] mask: 00000000ec3c08a9: 00 00 00 00 00 00 00 00 00 00 00 00 ............
Treat masks and values of pedit and filter match as network byte order, refactor pointers to them to void pointers instead of confusing u32 pointers and only cast to pointer-to-integer when reading a value from them. Treat pedit mlx5_fields->field_mask as host byte order according to its type u32, change the constants in fields array accordingly.
--- .../net/ethernet/mellanox/mlx5/core/en_tc.c | 60 ++++++++++--------- 1 file changed, 32 insertions(+), 28 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +32 -28
@@ -3147,7 +3147,7 @@ static struct mlx5_fields fields[] = {OFFLOAD (DIPV6_31_0 ,32 ,U32_MAX , ip6.daddr.s6_addr32[3 ],0 , dst_ipv4_dst_ipv6.ipv6_layout.ipv6[12 ]),OFFLOAD (IPV6_HOPLIMIT ,8 ,U8_MAX , ip6.hop_limit,0 , ttl_hoplimit), -OFFLOAD (IP_DSCP ,16 ,0xc00f , ip6,0 , ip_dscp), +OFFLOAD (IP_DSCP ,16 ,0x0fc0 , ip6,0 , ip_dscp),OFFLOAD (TCP_SPORT ,16 ,U16_MAX , tcp.source,0 , tcp_sport), OFFLOAD(TCP_DPORT, 16, U16_MAX, tcp.dest, 0, tcp_dport),
@@ -3158,21 +3158,31 @@ static struct mlx5_fields fields[] = { OFFLOAD(UDP_DPORT, 16, U16_MAX, udp.dest, 0, udp_dport), }; -static unsigned long mask_to_le (unsigned long mask,int size) +static u32 mask_field_get (void *mask,struct mlx5_fields *f) { -__be32 mask_be32; -__be16 mask_be16; - -if (size ==32 ) { - mask_be32 = (__force __be32)(mask); - mask = (__force unsigned long )cpu_to_le32 (be32_to_cpu (mask_be32)); - }else if (size ==16 ) { - mask_be32 = (__force __be32)(mask); - mask_be16 = *(__be16 *)&mask_be32; - mask = (__force unsigned long )cpu_to_le16 (be16_to_cpu (mask_be16)); +switch (f->field_bsize) { +case 32 : +return be32_to_cpu (*(__be32 *)mask) & f->field_mask; +case 16 : +return be16_to_cpu (*(__be16 *)mask) & (u16 )f->field_mask; +default : +return *(u8 *)mask & (u8 )f->field_mask; } +} -return mask; +static void mask_field_clear (void *mask,struct mlx5_fields *f) +{ +switch (f->field_bsize) { +case 32 : + *(__be32 *)mask &= ~cpu_to_be32 (f->field_mask); +break ; +case 16 : + *(__be16 *)mask &= ~cpu_to_be16 ((u16 )f->field_mask); +break ; +default : + *(u8 *)mask &= ~(u8 )f->field_mask; +break ; + } } static int offload_pedit_fields(struct mlx5e_priv *priv,
@@ -3184,11 +3194,12 @@ static int offload_pedit_fields(struct mlx5e_priv *priv,struct pedit_headers *set_masks, *add_masks, *set_vals, *add_vals;struct pedit_headers_action *hdrs = parse_attr->hdrs;void *headers_c, *headers_v, *action, *vals_p; -u32 *s_masks_p, *a_masks_p, s_mask, a_mask;struct mlx5e_tc_mod_hdr_acts *mod_acts; -unsigned long mask, field_mask; +void *s_masks_p, *a_masks_p;int i, first, last, next_z;struct mlx5_fields *f; +unsigned long mask; +u32 s_mask, a_mask;u8 cmd; mod_acts = &parse_attr->mod_hdr_acts;
@@ -3204,15 +3215,11 @@ static int offload_pedit_fields(struct mlx5e_priv *priv,bool skip; f = &fields[i]; -/* avoid seeing bits set from previous iterations */ - s_mask =0 ; - a_mask =0 ; - s_masks_p = (void *)set_masks + f->offset; a_masks_p = (void *)add_masks + f->offset; - s_mask = *s_masks_p & f->field_mask; - a_mask = *a_masks_p & f->field_mask; + s_mask =mask_field_get (s_masks_p, f); + a_mask =mask_field_get (a_masks_p, f);if (!s_mask && !a_mask)/* nothing to offload here */ continue ;
@@ -3239,22 +3246,20 @@ static int offload_pedit_fields(struct mlx5e_priv *priv, match_mask, f->field_bsize)) skip = true;/* clear to denote we consumed this field */ - *s_masks_p &= ~f->field_mask; +mask_field_clear (s_masks_p, f); }else { cmd =MLX5_ACTION_TYPE_ADD ; mask = a_mask; vals_p = (void *)add_vals + f->offset;/* add 0 is no change */ -if ((*(u32 *)vals_p & f->field_mask) ==0 ) +if (!mask_field_get (vals_p, f)) skip = true;/* clear to denote we consumed this field */ - *a_masks_p &= ~f->field_mask; +mask_field_clear (a_masks_p, f); }if (skip)continue ; - mask =mask_to_le (mask, f->field_bsize); - first =find_first_bit (&mask, f->field_bsize); next_z =find_next_zero_bit (&mask, f->field_bsize, first); last =find_last_bit (&mask, f->field_bsize);
@@ -3281,10 +3286,9 @@ static int offload_pedit_fields(struct mlx5e_priv *priv,MLX5_SET (set_action_in, action, field, f->field);if (cmd ==MLX5_ACTION_TYPE_SET ) { +unsigned long field_mask = f->field_mask;int start; - field_mask =mask_to_le (f->field_mask, f->field_bsize); -/* if field is bit sized it can start not from first bit */ start =find_first_bit (&field_mask, f->field_bsize);
Signature · 1 line
2.41.0
PATCH v2 8/15 net/mlx5e: Don't modify the peer sent-to-vport rules for IPSec offload
In reply to Saeed MahameedFrom: Jianbo Liu <[email protected]>
As IPSec packet offload in switchdev mode is not supported with LAG, it's unnecessary to modify those sent-to-vport rules to the peer eswitch.
--- drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +2 -1
@@ -984,7 +984,8 @@ mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *on_esw, dest.vport.flags |=MLX5_FLOW_DEST_VPORT_VHCA_ID ; flow_act.action =MLX5_FLOW_CONTEXT_ACTION_FWD_DEST ; -if (rep->vport ==MLX5_VPORT_UPLINK && on_esw->offloads.ft_ipsec_tx_pol) { +if (rep->vport ==MLX5_VPORT_UPLINK && + on_esw == from_esw && on_esw->offloads.ft_ipsec_tx_pol) { dest.ft = on_esw->offloads.ft_ipsec_tx_pol; flow_act.flags =FLOW_ACT_IGNORE_FLOW_LEVEL ; dest.type =MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE ;
Signature · 1 line
2.41.0
PATCH v2 9/15 net/mlx5e: Avoid referencing skb after free-ing in drop path of mlx5e_sq_xmit_wqe
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
When SQ is a port timestamping SQ for PTP, do not access tx flags of skb after free-ing the skb. Free the skb only after all references that depend on it have been handled in the dropped WQE path.
--- drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c +1 -1
@@ -494,10 +494,10 @@ mlx5e_sq_xmit_wqe(struct mlx5e_txqsq *sq, struct sk_buff *skb, err_drop: stats->dropped++; -dev_kfree_skb_any (skb);if (unlikely (sq->ptpsq && (skb_shinfo (skb)->tx_flags &SKBTX_HW_TSTAMP )))mlx5e_ptp_metadata_fifo_push (&sq->ptpsq->metadata_freelist,be32_to_cpu (eseg->flow_table_metadata)); +dev_kfree_skb_any (skb);mlx5e_tx_flush (sq); }
Signature · 1 line
2.41.0
PATCH v2 10/15 net/mlx5e: Track xmit submission to PTP WQ after populating metadata map
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
Ensure the skb is available in metadata mapping to skbs before tracking the metadata index for detecting undelivered CQEs. If the metadata index is put in the tracking list before putting the skb in the map, the metadata index might be used for detecting undelivered CQEs before the relevant skb is available in the map, which can lead to a null-ptr-deref.
Log:
general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
CPU: 0 PID: 1243 Comm: kworker/0:2 Not tainted 6.6.0-rc4+ #108
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
Workqueue: events mlx5e_rx_dim_work [mlx5_core]
RIP: 0010:mlx5e_ptp_napi_poll+0x9a4/0x2290 [mlx5_core]
Code: 8c 24 38 cc ff ff 4c 8d 3c c1 4c 89 f9 48 c1 e9 03 42 80 3c 31 00 0f 85 97 0f 00 00 4d 8b 3f 49 8d 7f 28 48 89 f9 48 c1 e9 03 <42> 80 3c 31 00 0f 85 8b 0f 00 00 49 8b 47 28 48 85 c0 0f 84 05 07
RSP: 0018:ffff8884d3c09c88 EFLAGS: 00010206
RAX: 0000000000000069 RBX: ffff8881160349d8 RCX: 0000000000000005
RDX: ffffed10218f48cf RSI: 0000000000000004 RDI: 0000000000000028
RBP: ffff888122707700 R08: 0000000000000001 R09: ffffed109a781383
R10: 0000000000000003 R11: 0000000000000003 R12: ffff88810c7a7a40
R13: ffff888122707700 R14: dffffc0000000000 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8884d3c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f4f878dd6e0 CR3: 000000014d108002 CR4: 0000000000370eb0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<IRQ>
? die_addr+0x3c/0xa0
? exc_general_protection+0x144/0x210
? asm_exc_general_protection+0x22/0x30
? mlx5e_ptp_napi_poll+0x9a4/0x2290 [mlx5_core]
? mlx5e_ptp_napi_poll+0x8f6/0x2290 [mlx5_core]
__napi_poll.constprop.0+0xa4/0x580
net_rx_action+0x460/0xb80
? _raw_spin_unlock_irqrestore+0x32/0x60
? __napi_poll.constprop.0+0x580/0x580
? tasklet_action_common.isra.0+0x2ef/0x760
__do_softirq+0x26c/0x827
irq_exit_rcu+0xc2/0x100
common_interrupt+0x7f/0xa0
</IRQ>
<TASK>
asm_common_interrupt+0x22/0x40
RIP: 0010:__kmem_cache_alloc_node+0xb/0x330
Code: 41 5d 41 5e 41 5f c3 8b 44 24 14 8b 4c 24 10 09 c8 eb d5 e8 b7 43 ca 01 0f 1f 80 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 57 <41> 56 41 89 d6 41 55 41 89 f5 41 54 49 89 fc 53 48 83 e4 f0 48 83
RSP: 0018:ffff88812c4079c0 EFLAGS: 00000246
RAX: 1ffffffff083c7fe RBX: ffff888100042dc0 RCX: 0000000000000218
RDX: 00000000ffffffff RSI: 0000000000000dc0 RDI: ffff888100042dc0
RBP: ffff88812c4079c8 R08: ffffffffa0289f96 R09: ffffed1025880ea9
R10: ffff888138839f80 R11: 0000000000000002 R12: 0000000000000dc0
R13: 0000000000000100 R14: 000000000000008c R15: ffff8881271fc450
? cmd_exec+0x796/0x2200 [mlx5_core]
kmalloc_trace+0x26/0xc0
cmd_exec+0x796/0x2200 [mlx5_core]
mlx5_cmd_do+0x22/0xc0 [mlx5_core]
mlx5_cmd_exec+0x17/0x30 [mlx5_core]
mlx5_core_modify_cq_moderation+0x139/0x1b0 [mlx5_core]
? mlx5_add_cq_to_tasklet+0x280/0x280 [mlx5_core]
? lockdep_set_lock_cmp_fn+0x190/0x190
? process_one_work+0x659/0x1220
mlx5e_rx_dim_work+0x9d/0x100 [mlx5_core]
process_one_work+0x730/0x1220
? lockdep_hardirqs_on_prepare+0x400/0x400
? max_active_store+0xf0/0xf0
? assign_work+0x168/0x240
worker_thread+0x70f/0x12d0
? __kthread_parkme+0xd1/0x1d0
? process_one_work+0x1220/0x1220
kthread+0x2d9/0x3b0
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x2d/0x70
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Modules linked in: xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat br_netfilter rpcsec_gss_krb5 auth_rpcgss oid_registry overlay mlx5_ib ib_uverbs ib_core zram zsmalloc mlx5_core fuse
---[ end trace 0000000000000000 ]------ drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c +1 -1
@@ -399,9 +399,9 @@ mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb,u8 metadata_index =be32_to_cpu (eseg->flow_table_metadata);mlx5e_skb_cb_hwtstamp_init (skb); -mlx5e_ptpsq_track_metadata (sq->ptpsq, metadata_index);mlx5e_ptp_metadata_map_put (&sq->ptpsq->metadata_map, skb, metadata_index); +mlx5e_ptpsq_track_metadata (sq->ptpsq, metadata_index);if (!netif_tx_queue_stopped (sq->txq) &&mlx5e_ptpsq_metadata_freelist_empty (sq->ptpsq)) {netif_tx_stop_queue (sq->txq);
Signature · 1 line
2.41.0
PATCH v2 11/15 net/mlx5e: Update doorbell for port timestamping CQ before the software counter
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
Previously, mlx5e_ptp_poll_ts_cq would update the device doorbell with the incremented consumer index after the relevant software counters in the kernel were updated. In the mlx5e_sq_xmit_wqe context, this would lead to either overrunning the device CQ or exceeding the expected software buffer size in the device CQ if the device CQ size was greater than the software buffer size. Update the relevant software counter only after updating the device CQ consumer index in the port timestamping napi_poll context.
Log:
mlx5_core 0000:08:00.0: cq_err_event_notifier:517:(pid 0): CQ error on CQN 0x487, syndrome 0x1
mlx5_core 0000:08:00.0 eth2: mlx5e_cq_error_event: cqn=0x000487 event=0x04--- .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c +16 -4
@@ -177,6 +177,8 @@ static void mlx5e_ptpsq_mark_ts_cqes_undelivered(struct mlx5e_ptpsq *ptpsq,static void mlx5e_ptp_handle_ts_cqe (struct mlx5e_ptpsq *ptpsq,struct mlx5_cqe64 *cqe, +u8 *md_buff, +u8 *md_buff_sz,int budget) {struct mlx5e_ptp_port_ts_cqe_list *pending_cqe_list = ptpsq->ts_cqe_pending_list;
@@ -211,19 +213,24 @@ static void mlx5e_ptp_handle_ts_cqe(struct mlx5e_ptpsq *ptpsq,mlx5e_ptpsq_mark_ts_cqes_undelivered (ptpsq, hwtstamp); out:napi_consume_skb (skb, budget); -mlx5e_ptp_metadata_fifo_push (&ptpsq->metadata_freelist, metadata_id); + md_buff[*md_buff_sz++] = metadata_id;if (unlikely (mlx5e_ptp_metadata_map_unhealthy (&ptpsq->metadata_map)) && !test_and_set_bit (MLX5E_SQ_STATE_RECOVERING , &sq->state))queue_work (ptpsq->txqsq.priv->wq, &ptpsq->report_unhealthy_work); } -static bool mlx5e_ptp_poll_ts_cq (struct mlx5e_cq *cq,int budget) +static bool mlx5e_ptp_poll_ts_cq (struct mlx5e_cq *cq,int napi_budget) { struct mlx5e_ptpsq *ptpsq = container_of(cq, struct mlx5e_ptpsq, ts_cq); -struct mlx5_cqwq *cqwq = &cq->wq; +int budget =min (napi_budget,MLX5E_TX_CQ_POLL_BUDGET ); +u8 metadata_buff[MLX5E_TX_CQ_POLL_BUDGET ]; +u8 metadata_buff_sz =0 ; +struct mlx5_cqwq *cqwq;struct mlx5_cqe64 *cqe;int work_done =0 ; + cqwq = &cq->wq; +if (unlikely (!test_bit (MLX5E_SQ_STATE_ENABLED , &ptpsq->txqsq.state)))return false;
@@ -234,7 +241,8 @@ static bool mlx5e_ptp_poll_ts_cq(struct mlx5e_cq *cq, int budget)do {mlx5_cqwq_pop (cqwq); -mlx5e_ptp_handle_ts_cqe (ptpsq, cqe, budget); +mlx5e_ptp_handle_ts_cqe (ptpsq, cqe, + metadata_buff, &metadata_buff_sz, napi_budget); }while ((++work_done < budget) && (cqe =mlx5_cqwq_get_cqe (cqwq)));mlx5_cqwq_update_db_record (cqwq);
@@ -242,6 +250,10 @@ static bool mlx5e_ptp_poll_ts_cq(struct mlx5e_cq *cq, int budget)/* ensure cq space is freed before enabling more cqes */ wmb (); +while (metadata_buff_sz >0 ) +mlx5e_ptp_metadata_fifo_push (&ptpsq->metadata_freelist, + metadata_buff[--metadata_buff_sz]); +mlx5e_txqsq_wake (&ptpsq->txqsq);return work_done == budget;
Signature · 1 line
2.41.0
PATCH v2 12/15 net/mlx5: Increase size of irq name buffer
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
Without increased buffer size, will trigger -Wformat-truncation with W=1 for the snprintf operation writing to the buffer.
drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c: In function 'mlx5_irq_alloc':
drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c:296:7: error: '@pci:' directive output may be truncated writing 5 bytes into a region of size between 1 and 32 [-Werror=format-truncation=]
296 | "%s@pci:%s", name, pci_name(dev->pdev));
| ^~~~~
drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c:295:2: note: 'snprintf' output 6 or more bytes (assuming 37) into a destination of size 32
295 | snprintf(irq->name, MLX5_MAX_IRQ_NAME,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
296 | "%s@pci:%s", name, pci_name(dev->pdev));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--- drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c | 6 +++--- drivers/net/ethernet/mellanox/mlx5/core/pci_irq.h | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c +3 -3
@@ -28,7 +28,7 @@struct mlx5_irq {struct atomic_notifier_head nh;cpumask_var_t mask; -char name[MLX5_MAX_IRQ_NAME ]; +char name[MLX5_MAX_IRQ_FORMATTED_NAME ];struct mlx5_irq_pool *pool;int refcount;struct msi_map map;
@@ -292,8 +292,8 @@ struct mlx5_irq *mlx5_irq_alloc(struct mlx5_irq_pool *pool, int i,else irq_sf_set_name (pool, name, i);ATOMIC_INIT_NOTIFIER_HEAD (&irq->nh); -snprintf (irq->name,MLX5_MAX_IRQ_NAME , -"%s@pci:%s" , name,pci_name (dev->pdev)); +snprintf (irq->name,MLX5_MAX_IRQ_FORMATTED_NAME , +MLX5_IRQ_NAME_FORMAT_STR , name,pci_name (dev->pdev)); err =request_irq (irq->map.virq, irq_int_handler,0 , irq->name, &irq->nh);if (err) {
drivers/net/ethernet/mellanox/mlx5/core/pci_irq.h +3 -0
@@ -7,6 +7,9 @@#include <linux/mlx5/driver.h> #define MLX5_MAX_IRQ_NAME (32) +#define MLX5_IRQ_NAME_FORMAT_STR ("%s@pci:%s") +#define MLX5_MAX_IRQ_FORMATTED_NAME \ + (MLX5_MAX_IRQ_NAME + sizeof(MLX5_IRQ_NAME_FORMAT_STR))/* max irq_index is 2047, so four chars */ #define MLX5_MAX_IRQ_IDX_CHARS (4)#define MLX5_EQ_REFS_PER_IRQ (2)
Signature · 1 line
2.41.0
PATCH v2 13/15 net/mlx5e: Reduce the size of icosq_str
In reply to Saeed MahameedFrom: Saeed Mahameed <[email protected]>
icosq_str size is unnecessarily too long, and it causes a build warning -Wformat-truncation with W=1. Looking closely, It doesn't need to be 255B, hence this patch reduces the size to 32B which should be more than enough to host the string: "ICOSQ: 0x%x, ".
While here, add a missing space in the formatted string.
This fixes the following build warning:
$ KCFLAGS='-Wall -Werror' $ make O=/tmp/kbuild/linux W=1 -s -j12 drivers/net/ethernet/mellanox/mlx5/core/
drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c: In function 'mlx5e_reporter_rx_timeout':drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c:718:56:
error: ', CQ: 0x' directive output may be truncated writing 8 bytes into a region of size between 0 and 255 [-Werror=format-truncation=]
718 | "RX timeout on channel: %d, %sRQ: 0x%x, CQ: 0x%x",
| ^~~~~~~~drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c:717:9: note: 'snprintf' output between 43 and 322 bytes into a destination of size 288
717 | snprintf(err_str, sizeof(err_str),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
718 | "RX timeout on channel: %d, %sRQ: 0x%x, CQ: 0x%x",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
719 | rq->ix, icosq_str, rq->rqn, rq->cq.mcq.cqn);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--- drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c +2 -2
@@ -492,11 +492,11 @@ static int mlx5e_rx_reporter_dump(struct devlink_health_reporter *reporter,void mlx5e_reporter_rx_timeout (struct mlx5e_rq *rq) { -char icosq_str[MLX5E_REPORTER_PER_Q_MAX_LEN ] = {};char err_str[MLX5E_REPORTER_PER_Q_MAX_LEN ];struct mlx5e_icosq *icosq = rq->icosq;struct mlx5e_priv *priv = rq->priv;struct mlx5e_err_ctx err_ctx = {}; +char icosq_str[32 ] = {}; err_ctx.ctx = rq; err_ctx.recover = mlx5e_rx_reporter_timeout_recover;
@@ -505,7 +505,7 @@ void mlx5e_reporter_rx_timeout(struct mlx5e_rq *rq)if (icosq)snprintf (icosq_str,sizeof (icosq_str),"ICOSQ: 0x%x, " , icosq->sqn);snprintf (err_str,sizeof (err_str), -"RX timeout on channel: %d, %sRQ: 0x%x, CQ: 0x%x" , +"RX timeout on channel: %d, %s RQ: 0x%x, CQ: 0x%x" , rq->ix, icosq_str, rq->rqn, rq->cq.mcq.cqn);mlx5e_health_report (priv, priv->rx_reporter, err_str, &err_ctx);
Signature · 1 line
2.41.0
PATCH v2 14/15 net/mlx5e: Check return value of snprintf writing to fw_version buffer
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
Treat the operation as an error case when the return value is equivalent to the size of the name buffer. Failed to write null terminator to the name buffer, making the string malformed and should not be used. Provide a string with only the firmware version when forming the string with the board id fails.
Without check, will trigger -Wformat-truncation with W=1.
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: In function 'mlx5e_ethtool_get_drvinfo':
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:49:31: warning: '%.16s' directive output may be truncated writing up to 16 bytes into a region of size between 13 and 22 [-Wformat-truncation=]
49 | "%d.%d.%04d (%.16s)",
| ^~~~~
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:48:9: note: 'snprintf' output between 12 and 37 bytes into a destination of size 32
48 | snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
49 | "%d.%d.%04d (%.16s)",
| ~~~~~~~~~~~~~~~~~~~~~
50 | fw_rev_maj(mdev), fw_rev_min(mdev), fw_rev_sub(mdev),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
51 | mdev->board_id);
| ~~~~~~~~~~~~~~~--- .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +9 -4
@@ -43,12 +43,17 @@ void mlx5e_ethtool_get_drvinfo(struct mlx5e_priv *priv, struct ethtool_drvinfo *drvinfo) {struct mlx5_core_dev *mdev = priv->mdev; +int count;strscpy (drvinfo->driver,KBUILD_MODNAME ,sizeof (drvinfo->driver)); -snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), -"%d.%d.%04d (%.16s)" , -fw_rev_maj (mdev),fw_rev_min (mdev),fw_rev_sub (mdev), - mdev->board_id); + count =snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d (%.16s)" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev), mdev->board_id); +if (count ==sizeof (drvinfo->fw_version)) +snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev)); +strscpy (drvinfo->bus_info,dev_name (mdev->device),sizeof (drvinfo->bus_info)); }
Signature · 1 line
2.41.0
David Laight
NAKRE: [net V2 14/15] net/mlx5e: Check return value of snprintf writing to fw_version buffer
From: Saeed Mahameed
Quoted 7 lines
Sent: 14 November 2023 21:59
Treat the operation as an error case when the return value is equivalent to the size of the name buffer. Failed to write null terminator to the name buffer, making the string malformed and should not be used. Provide a string with only the firmware version when forming the string with the board id fails.
Nak.
RTFM snprintf().
Quoted 58 lines
Without check, will trigger -Wformat-truncation with W=1.
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: In function 'mlx5e_ethtool_get_drvinfo':
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:49:31: warning: '%.16s' directive output may
be truncated writing up to 16 bytes into a region of size between 13 and 22 [-Wformat-truncation=]
49 | "%d.%d.%04d (%.16s)",
| ^~~~~
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:48:9: note: 'snprintf' output between 12 and
37 bytes into a destination of size 3248 | snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 49 | "%d.%d.%04d (%.16s)", | ~~~~~~~~~~~~~~~~~~~~~ 50 | fw_rev_maj(mdev), fw_rev_min(mdev), fw_rev_sub(mdev), | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 51 | mdev->board_id); | ~~~~~~~~~~~~~~~Fixes 84e11edb71de ("net/mlx5e: Show board id in ethtool driver information")Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6d4ab2e97dcfbcd748ae71761a9d8e5e41cc732cSigned-off-by Rahul Rameshbabu <[email protected]>Reviewed-by Dragos Tatulea <[email protected]>Signed-off-by Saeed Mahameed <[email protected]>--- .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +9
-5@@ -43,12 +43,17 @@ void mlx5e_ethtool_get_drvinfo(struct mlx5e_priv *priv, struct ethtool_drvinfo *drvinfo) {struct mlx5_core_dev *mdev = priv->mdev; +int count;strscpy (drvinfo->driver,KBUILD_MODNAME ,sizeof (drvinfo->driver)); -snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), -"%d.%d.%04d (%.16s)" , -fw_rev_maj (mdev),fw_rev_min (mdev),fw_rev_sub (mdev), - mdev->board_id); + count =snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d (%.16s)" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev), mdev->board_id); +if (count ==sizeof (drvinfo->fw_version)) +snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev)); +strscpy (drvinfo->bus_info,dev_name (mdev->device),sizeof (drvinfo->bus_info)); } --
Quoted 2 lines
2.41.0
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [net V2 14/15] net/mlx5e: Check return value of snprintf writing to fw_version buffer
Quoted 54 lines · On Sun, 19 Nov, 2023 10:46:57 +0000 David Laight <[email protected]>…
From: Saeed Mahameed
Quoted 7 lines
Sent: 14 November 2023 21:59
Treat the operation as an error case when the return value is equivalent to the size of the name buffer. Failed to write null terminator to the name buffer, making the string malformed and should not be used. Provide a string with only the firmware version when forming the string with the board id fails.
Nak.
RTFM
snprintf().Quoted 41 lines
Without check, will trigger -Wformat-truncation with W=1.
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: In function 'mlx5e_ethtool_get_drvinfo':
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:49:31: warning: '%.16s' directive output may
be truncated writing up to 16 bytes into a region of size between 13 and 22 [-Wformat-truncation=]
49 | "%d.%d.%04d (%.16s)",
| ^~~~~
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:48:9: note: 'snprintf' output between 12 and
37 bytes into a destination of size 3248 | snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 49 | "%d.%d.%04d (%.16s)", | ~~~~~~~~~~~~~~~~~~~~~ 50 | fw_rev_maj(mdev), fw_rev_min(mdev), fw_rev_sub(mdev), | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 51 | mdev->board_id); | ~~~~~~~~~~~~~~~Fixes 84e11edb71de ("net/mlx5e: Show board id in ethtool driver information")Link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6d4ab2e97dcfbcd748ae71761a9d8e5e41cc732cSigned-off-by Rahul Rameshbabu <[email protected]>Reviewed-by Dragos Tatulea <[email protected]>Signed-off-by Saeed Mahameed <[email protected]>--- .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +1
-0@@ -43,12 +43,17 @@ void mlx5e_ethtool_get_drvinfo(struct mlx5e_priv *priv, struct ethtool_drvinfo *drvinfo) {struct mlx5_core_dev *mdev = priv->mdev; +int count;
Quoted 9 lines
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c void mlx5e_ethtool_get_drvinfo(struct mlx5e_priv *priv, · line 52
Quoted 9 lines
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +4
-4strscpy (drvinfo->driver,KBUILD_MODNAME ,sizeof (drvinfo->driver)); -snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), -"%d.%d.%04d (%.16s)" , -fw_rev_maj (mdev),fw_rev_min (mdev),fw_rev_sub (mdev), - mdev->board_id); + count =snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d (%.16s)" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev), mdev->board_id); +if (count ==sizeof (drvinfo->fw_version))
This should be >= now that I think about it.
From the kernel docs: If the return is greater than or equal to size, the resulting string is truncated.
The return value *can* be greater than the size parameter expressing what the length would have been.
https://docs.kernel.org/core-api/kernel-api.html#c.snprintf
Quoted 10 lines
Quoted 10 lines
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +4
-1+snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev)); +strscpy (drvinfo->bus_info,dev_name (mdev->device),sizeof (drvinfo->bus_info)); } --2.41.0
--
Thanks,
Rahul Rameshbabu
PATCH v2 15/15 net/mlx5e: Check return value of snprintf writing to fw_version buffer for representors
In reply to Saeed MahameedFrom: Rahul Rameshbabu <[email protected]>
Treat the operation as an error case when the return value is equivalent to the size of the name buffer. Failed to write null terminator to the name buffer, making the string malformed and should not be used. Provide a string with only the firmware version when forming the string with the board id fails. This logic for representors is identical to normal flow with ethtool.
Without check, will trigger -Wformat-truncation with W=1.
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c: In function 'mlx5e_rep_get_drvinfo':
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:78:31: warning: '%.16s' directive output may be truncated writing up to 16 bytes into a region of size between 13 and 22 [-Wformat-truncation=]
78 | "%d.%d.%04d (%.16s)",
| ^~~~~
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:77:9: note: 'snprintf' output between 12 and 37 bytes into a destination of size 32
77 | snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
78 | "%d.%d.%04d (%.16s)",
| ~~~~~~~~~~~~~~~~~~~~~
79 | fw_rev_maj(mdev), fw_rev_min(mdev),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
80 | fw_rev_sub(mdev), mdev->board_id);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~--- drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c +8 -4
@@ -71,13 +71,17 @@ static void mlx5e_rep_get_drvinfo(struct net_device *dev, {struct mlx5e_priv *priv =netdev_priv (dev);struct mlx5_core_dev *mdev = priv->mdev; +int count;strscpy (drvinfo->driver, mlx5e_rep_driver_name,sizeof (drvinfo->driver)); -snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), -"%d.%d.%04d (%.16s)" , -fw_rev_maj (mdev),fw_rev_min (mdev), -fw_rev_sub (mdev), mdev->board_id); + count =snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d (%.16s)" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev), mdev->board_id); +if (count ==sizeof (drvinfo->fw_version)) +snprintf (drvinfo->fw_version,sizeof (drvinfo->fw_version), +"%d.%d.%04d" ,fw_rev_maj (mdev), +fw_rev_min (mdev),fw_rev_sub (mdev)); }static const struct counter_desc sw_rep_stats_desc[] = {
Signature · 1 line
2.41.0