| Message ID | 20190723072248.6844-2-leon@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | DIM fixes for 5.3 | expand |
On Tue, 2019-07-23 at 10:22 +0300, Leon Romanovsky wrote: > From: Yamin Friedman <yaminf@mellanox.com> > > While using net_dim, a dim_sample was used without ever initializing > the > comps value. Added use of DIV_ROUND_DOWN_ULL() to prevent potential > overflow, it should not be a problem to save the final result in an > int > because after the division by epms the value should not be larger > than a > few thousand. > > [ 1040.127124] UBSAN: Undefined behaviour in lib/dim/dim.c:78:23 > [ 1040.130118] signed integer overflow: > [ 1040.131643] 134718714 * 100 cannot be represented in type 'int' > > Fixes: 398c2b05bbee ("linux/dim: Add completions count to > dim_sample") > Signed-off-by: Yamin Friedman <yaminf@mellanox.com> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/net/ethernet/broadcom/bcmsysport.c | 2 +- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +- > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +- > drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 4 ++-- > lib/dim/dim.c | 4 ++-- > 5 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c > b/drivers/net/ethernet/broadcom/bcmsysport.c > index b9c5cea8db16..9483553ce444 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -992,7 +992,7 @@ static int bcm_sysport_poll(struct napi_struct > *napi, int budget) > { > struct bcm_sysport_priv *priv = > container_of(napi, struct bcm_sysport_priv, napi); > - struct dim_sample dim_sample; > + struct dim_sample dim_sample = {}; net_dim implementation doesn't care about sample->comp_ctr, so this is unnecessary for the sake of fixing the rdma overflow issue, but it doens't hurt anyone to have this change in this patch, although Tariq already sent me a fix that i applied to my internal queues. > unsigned int work_done = 0; > > work_done = bcm_sysport_desc_rx(priv, budget); > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 7134d2c3eb1c..7070349915bc 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -2136,7 +2136,7 @@ static int bnxt_poll(struct napi_struct *napi, > int budget) > } > } > if (bp->flags & BNXT_FLAG_DIM) { > - struct dim_sample dim_sample; > + struct dim_sample dim_sample = {}; > > dim_update_sample(cpr->event_ctr, > cpr->rx_packets, > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index a2b57807453b..d3a0b614dbfa 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -1895,7 +1895,7 @@ static int bcmgenet_rx_poll(struct napi_struct > *napi, int budget) > { > struct bcmgenet_rx_ring *ring = container_of(napi, > struct bcmgenet_rx_ring, napi); > - struct dim_sample dim_sample; > + struct dim_sample dim_sample = {}; > unsigned int work_done; > > work_done = bcmgenet_desc_rx(ring, budget); > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > index c50b6f0769c8..49b06b256c92 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > @@ -49,7 +49,7 @@ static inline bool > mlx5e_channel_no_affinity_change(struct mlx5e_channel *c) > static void mlx5e_handle_tx_dim(struct mlx5e_txqsq *sq) > { > struct mlx5e_sq_stats *stats = sq->stats; > - struct dim_sample dim_sample; > + struct dim_sample dim_sample = {}; > > if (unlikely(!test_bit(MLX5E_SQ_STATE_AM, &sq->state))) > return; > @@ -61,7 +61,7 @@ static void mlx5e_handle_tx_dim(struct mlx5e_txqsq > *sq) > static void mlx5e_handle_rx_dim(struct mlx5e_rq *rq) > { > struct mlx5e_rq_stats *stats = rq->stats; > - struct dim_sample dim_sample; > + struct dim_sample dim_sample = {}; > > if (unlikely(!test_bit(MLX5E_RQ_STATE_AM, &rq->state))) > return; > diff --git a/lib/dim/dim.c b/lib/dim/dim.c > index 439d641ec796..38045d6d0538 100644 > --- a/lib/dim/dim.c > +++ b/lib/dim/dim.c > @@ -74,8 +74,8 @@ void dim_calc_stats(struct dim_sample *start, > struct dim_sample *end, > delta_us); > curr_stats->cpms = DIV_ROUND_UP(ncomps * USEC_PER_MSEC, > delta_us); > if (curr_stats->epms != 0) BTW unrelated to this changed but curr_stats->epms can never be 0 due to DIV_ROUND_UP. we can save a condition here. > - curr_stats->cpe_ratio = > - (curr_stats->cpms * 100) / curr_stats- > >epms; > + curr_stats->cpe_ratio = DIV_ROUND_DOWN_ULL( > + curr_stats->cpms * 100, curr_stats->epms); > else > curr_stats->cpe_ratio = 0; > LGTM, Acked-by: Saeed Mahameed <saeedm@mellanox.com> > -- > 2.20.1 >
On Tue, Jul 23, 2019 at 05:22:43PM +0000, Saeed Mahameed wrote: > On Tue, 2019-07-23 at 10:22 +0300, Leon Romanovsky wrote: > > From: Yamin Friedman <yaminf@mellanox.com> > > > > While using net_dim, a dim_sample was used without ever initializing > > the > > comps value. Added use of DIV_ROUND_DOWN_ULL() to prevent potential > > overflow, it should not be a problem to save the final result in an > > int > > because after the division by epms the value should not be larger > > than a > > few thousand. > > > > [ 1040.127124] UBSAN: Undefined behaviour in lib/dim/dim.c:78:23 > > [ 1040.130118] signed integer overflow: > > [ 1040.131643] 134718714 * 100 cannot be represented in type 'int' > > > > Fixes: 398c2b05bbee ("linux/dim: Add completions count to > > dim_sample") > > Signed-off-by: Yamin Friedman <yaminf@mellanox.com> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > --- > > drivers/net/ethernet/broadcom/bcmsysport.c | 2 +- > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +- > > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +- > > drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 4 ++-- > > lib/dim/dim.c | 4 ++-- > > 5 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c > > b/drivers/net/ethernet/broadcom/bcmsysport.c > > index b9c5cea8db16..9483553ce444 100644 > > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > > @@ -992,7 +992,7 @@ static int bcm_sysport_poll(struct napi_struct > > *napi, int budget) > > { > > struct bcm_sysport_priv *priv = > > container_of(napi, struct bcm_sysport_priv, napi); > > - struct dim_sample dim_sample; > > + struct dim_sample dim_sample = {}; > > net_dim implementation doesn't care about sample->comp_ctr, so this is > unnecessary for the sake of fixing the rdma overflow issue, but it > doens't hurt anyone to have this change in this patch. Yes, this is why we decided to change all drivers and not mlx5 only. Thanks
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index b9c5cea8db16..9483553ce444 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -992,7 +992,7 @@ static int bcm_sysport_poll(struct napi_struct *napi, int budget) { struct bcm_sysport_priv *priv = container_of(napi, struct bcm_sysport_priv, napi); - struct dim_sample dim_sample; + struct dim_sample dim_sample = {}; unsigned int work_done = 0; work_done = bcm_sysport_desc_rx(priv, budget); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 7134d2c3eb1c..7070349915bc 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -2136,7 +2136,7 @@ static int bnxt_poll(struct napi_struct *napi, int budget) } } if (bp->flags & BNXT_FLAG_DIM) { - struct dim_sample dim_sample; + struct dim_sample dim_sample = {}; dim_update_sample(cpr->event_ctr, cpr->rx_packets, diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index a2b57807453b..d3a0b614dbfa 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1895,7 +1895,7 @@ static int bcmgenet_rx_poll(struct napi_struct *napi, int budget) { struct bcmgenet_rx_ring *ring = container_of(napi, struct bcmgenet_rx_ring, napi); - struct dim_sample dim_sample; + struct dim_sample dim_sample = {}; unsigned int work_done; work_done = bcmgenet_desc_rx(ring, budget); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c index c50b6f0769c8..49b06b256c92 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c @@ -49,7 +49,7 @@ static inline bool mlx5e_channel_no_affinity_change(struct mlx5e_channel *c) static void mlx5e_handle_tx_dim(struct mlx5e_txqsq *sq) { struct mlx5e_sq_stats *stats = sq->stats; - struct dim_sample dim_sample; + struct dim_sample dim_sample = {}; if (unlikely(!test_bit(MLX5E_SQ_STATE_AM, &sq->state))) return; @@ -61,7 +61,7 @@ static void mlx5e_handle_tx_dim(struct mlx5e_txqsq *sq) static void mlx5e_handle_rx_dim(struct mlx5e_rq *rq) { struct mlx5e_rq_stats *stats = rq->stats; - struct dim_sample dim_sample; + struct dim_sample dim_sample = {}; if (unlikely(!test_bit(MLX5E_RQ_STATE_AM, &rq->state))) return; diff --git a/lib/dim/dim.c b/lib/dim/dim.c index 439d641ec796..38045d6d0538 100644 --- a/lib/dim/dim.c +++ b/lib/dim/dim.c @@ -74,8 +74,8 @@ void dim_calc_stats(struct dim_sample *start, struct dim_sample *end, delta_us); curr_stats->cpms = DIV_ROUND_UP(ncomps * USEC_PER_MSEC, delta_us); if (curr_stats->epms != 0) - curr_stats->cpe_ratio = - (curr_stats->cpms * 100) / curr_stats->epms; + curr_stats->cpe_ratio = DIV_ROUND_DOWN_ULL( + curr_stats->cpms * 100, curr_stats->epms); else curr_stats->cpe_ratio = 0;