| Message ID | 1495543807-14956-1-git-send-email-tariqt@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 23, 2017 at 03:50:07PM +0300, Tariq Toukan wrote: > From: Talat Batheesh <talatb@mellanox.com> > > Our previous patch (cited below) introduced a regression > for RAW Eth QPs. > > Fix it by checking if the QP number provided by user-space > exists, hence allowing steering rules to be added for valid > QPs only. > > Fixes: 89c557687a32 ("net/mlx4_en: Avoid adding steering rules with ...") > Reported-by: Or Gerlitz <gerlitz.or@gmail.com> > Signed-off-by: Talat Batheesh <talatb@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > --- > drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 3 +-- > drivers/net/ethernet/mellanox/mlx4/qp.c | 14 ++++++++++++++ > include/linux/mlx4/qp.h | 1 + > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > index ae5fdc2df654..00a7cd3dcc2e 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct net_device *dev, > qpn = priv->drop_qp.qpn; > else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { > qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); > - if (qpn < priv->rss_map.base_qpn || > - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { > + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { > en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); > return -EINVAL; > } > diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c > index 2d6abd4662b1..1eff2fe32a8b 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/qp.c > +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c > @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev *dev, int qpn) > __mlx4_qp_free_icm(dev, qpn); > } > > +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) > +{ > + struct mlx4_qp_table *qp_table = &mlx4_priv(dev)->qp_table; > + struct mlx4_qp *qp; > + > + spin_lock(&qp_table->lock); > + > + qp = __mlx4_qp_lookup(dev, qpn); > + > + spin_unlock(&qp_table->lock); > + return qp; > +} > +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); Tariq, Why do you need this export and header fils? You are using this function in one place only. Thanks > + > int mlx4_qp_alloc(struct mlx4_dev *dev, int qpn, struct mlx4_qp *qp, gfp_t gfp) > { > struct mlx4_priv *priv = mlx4_priv(dev); > diff --git a/include/linux/mlx4/qp.h b/include/linux/mlx4/qp.h > index b4ee8f62ce8d..8e2828d48d7f 100644 > --- a/include/linux/mlx4/qp.h > +++ b/include/linux/mlx4/qp.h > @@ -470,6 +470,7 @@ struct mlx4_update_qp_params { > u16 rate_val; > }; > > +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn); > int mlx4_update_qp(struct mlx4_dev *dev, u32 qpn, > enum mlx4_update_qp_attr attr, > struct mlx4_update_qp_params *params); > -- > 1.8.3.1 >
On 5/23/2017 3:50 PM, Tariq Toukan wrote: > From: Talat Batheesh <talatb@mellanox.com> > > Our previous patch (cited below) introduced a regression > for RAW Eth QPs. > > Fix it by checking if the QP number provided by user-space > exists, hence allowing steering rules to be added for valid > QPs only. > > Fixes: 89c557687a32 ("net/mlx4_en: Avoid adding steering rules with ...") > Reported-by: Or Gerlitz <gerlitz.or@gmail.com> > Signed-off-by: Talat Batheesh <talatb@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> Acked-by: Or Gerlitz <ogerlitz@mellanox.com>
On Tue, May 23, 2017 at 04:15:13PM +0300, Leon Romanovsky wrote: > On Tue, May 23, 2017 at 03:50:07PM +0300, Tariq Toukan wrote: > > From: Talat Batheesh <talatb@mellanox.com> > > > > Our previous patch (cited below) introduced a regression > > for RAW Eth QPs. > > > > Fix it by checking if the QP number provided by user-space > > exists, hence allowing steering rules to be added for valid > > QPs only. > > > > Fixes: 89c557687a32 ("net/mlx4_en: Avoid adding steering rules with ...") > > Reported-by: Or Gerlitz <gerlitz.or@gmail.com> > > Signed-off-by: Talat Batheesh <talatb@mellanox.com> > > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > > --- > > drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 3 +-- > > drivers/net/ethernet/mellanox/mlx4/qp.c | 14 ++++++++++++++ > > include/linux/mlx4/qp.h | 1 + > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > > index ae5fdc2df654..00a7cd3dcc2e 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > > @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct net_device *dev, > > qpn = priv->drop_qp.qpn; > > else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { > > qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); > > - if (qpn < priv->rss_map.base_qpn || > > - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { > > + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { > > en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); > > return -EINVAL; > > } > > diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c > > index 2d6abd4662b1..1eff2fe32a8b 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/qp.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c > > @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev *dev, int qpn) > > __mlx4_qp_free_icm(dev, qpn); > > } > > > > +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) > > +{ > > + struct mlx4_qp_table *qp_table = &mlx4_priv(dev)->qp_table; > > + struct mlx4_qp *qp; > > + > > + spin_lock(&qp_table->lock); > > + > > + qp = __mlx4_qp_lookup(dev, qpn); > > + > > + spin_unlock(&qp_table->lock); > > + return qp; > > +} > > +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); > > Tariq, > > Why do you need this export and header fils? You are using this function in one place only. > Let's keep it as you proposed for better separation of en vs. core modules. Reviewed-by: Leon Romanovsky <leonro@mellanox.com> Thanks
From: Tariq Toukan <tariqt@mellanox.com> Date: Tue, 23 May 2017 15:50:07 +0300 > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > index ae5fdc2df654..00a7cd3dcc2e 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct net_device *dev, > qpn = priv->drop_qp.qpn; > else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { > qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); > - if (qpn < priv->rss_map.base_qpn || > - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { > + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { > en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); > return -EINVAL; > } > diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c > index 2d6abd4662b1..1eff2fe32a8b 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/qp.c > +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c > @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev *dev, int qpn) > __mlx4_qp_free_icm(dev, qpn); > } > > +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) ... > +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); > + This phony separation between MLX4_CORE and MLX4_EN is the only reason you need this unreasonable symbol export. I doubt you'll ever use this function anywhere outside of en_ethtool.c so this export is wasted space in the kernel image. Probably compiler could inline it decently as well. So find another way to do this without the symbol export. I don't really want to hear any stories about "clean separation" or whatever. What's happening here is exactly why this separate modules scheme results in ugly unreasonable code, and unnecessary gymnastics and wasted object space just to make routines available in one place from another.
On 24/05/2017 10:36 PM, David Miller wrote: > From: Tariq Toukan <tariqt@mellanox.com> > Date: Tue, 23 May 2017 15:50:07 +0300 > >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> index ae5fdc2df654..00a7cd3dcc2e 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct net_device *dev, >> qpn = priv->drop_qp.qpn; >> else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { >> qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); >> - if (qpn < priv->rss_map.base_qpn || >> - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { >> + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { >> en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); >> return -EINVAL; >> } >> diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c >> index 2d6abd4662b1..1eff2fe32a8b 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/qp.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c >> @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev *dev, int qpn) >> __mlx4_qp_free_icm(dev, qpn); >> } >> >> +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) > ... >> +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); >> + > > This phony separation between MLX4_CORE and MLX4_EN is the only reason > you need this unreasonable symbol export. > > I doubt you'll ever use this function anywhere outside of en_ethtool.c > so this export is wasted space in the kernel image. Probably compiler > could inline it decently as well. > > So find another way to do this without the symbol export. I don't > really want to hear any stories about "clean separation" or whatever. > What's happening here is exactly why this separate modules scheme > results in ugly unreasonable code, and unnecessary gymnastics and > wasted object space just to make routines available in one place from > another. I see. I'll remove this EXPORT_SYMBOL_GPL and send a re-spin shortly. Function will still be accessible within EN driver, as mlx4_en.h includes qp.h. Regards, Tariq
On 25/05/2017 4:07 PM, Tariq Toukan wrote: > > On 24/05/2017 10:36 PM, David Miller wrote: >> From: Tariq Toukan <tariqt@mellanox.com> >> Date: Tue, 23 May 2017 15:50:07 +0300 >> >>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >>> b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >>> index ae5fdc2df654..00a7cd3dcc2e 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >>> @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct >>> net_device *dev, >>> qpn = priv->drop_qp.qpn; >>> else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { >>> qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); >>> - if (qpn < priv->rss_map.base_qpn || >>> - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { >>> + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { >>> en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); >>> return -EINVAL; >>> } >>> diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c >>> b/drivers/net/ethernet/mellanox/mlx4/qp.c >>> index 2d6abd4662b1..1eff2fe32a8b 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx4/qp.c >>> +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c >>> @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev >>> *dev, int qpn) >>> __mlx4_qp_free_icm(dev, qpn); >>> } >>> +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) >> ... >>> +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); >>> + >> >> This phony separation between MLX4_CORE and MLX4_EN is the only reason >> you need this unreasonable symbol export. >> >> I doubt you'll ever use this function anywhere outside of en_ethtool.c >> so this export is wasted space in the kernel image. Probably compiler >> could inline it decently as well. >> >> So find another way to do this without the symbol export. I don't >> really want to hear any stories about "clean separation" or whatever. >> What's happening here is exactly why this separate modules scheme >> results in ugly unreasonable code, and unnecessary gymnastics and >> wasted object space just to make routines available in one place from >> another. > > I see. I'll remove this EXPORT_SYMBOL_GPL and send a re-spin shortly. > Function will still be accessible within EN driver, as mlx4_en.h > includes qp.h. > Hmm, my bad, this doesn't work. Please ignore V2. > Regards, > Tariq >
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c index ae5fdc2df654..00a7cd3dcc2e 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c @@ -1562,8 +1562,7 @@ static int mlx4_en_flow_replace(struct net_device *dev, qpn = priv->drop_qp.qpn; else if (cmd->fs.ring_cookie & EN_ETHTOOL_QP_ATTACH) { qpn = cmd->fs.ring_cookie & (EN_ETHTOOL_QP_ATTACH - 1); - if (qpn < priv->rss_map.base_qpn || - qpn >= priv->rss_map.base_qpn + priv->rx_ring_num) { + if (!mlx4_qp_lookup(priv->mdev->dev, qpn)) { en_warn(priv, "rxnfc: QP (0x%x) doesn't exist\n", qpn); return -EINVAL; } diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c index 2d6abd4662b1..1eff2fe32a8b 100644 --- a/drivers/net/ethernet/mellanox/mlx4/qp.c +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c @@ -384,6 +384,20 @@ static void mlx4_qp_free_icm(struct mlx4_dev *dev, int qpn) __mlx4_qp_free_icm(dev, qpn); } +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn) +{ + struct mlx4_qp_table *qp_table = &mlx4_priv(dev)->qp_table; + struct mlx4_qp *qp; + + spin_lock(&qp_table->lock); + + qp = __mlx4_qp_lookup(dev, qpn); + + spin_unlock(&qp_table->lock); + return qp; +} +EXPORT_SYMBOL_GPL(mlx4_qp_lookup); + int mlx4_qp_alloc(struct mlx4_dev *dev, int qpn, struct mlx4_qp *qp, gfp_t gfp) { struct mlx4_priv *priv = mlx4_priv(dev); diff --git a/include/linux/mlx4/qp.h b/include/linux/mlx4/qp.h index b4ee8f62ce8d..8e2828d48d7f 100644 --- a/include/linux/mlx4/qp.h +++ b/include/linux/mlx4/qp.h @@ -470,6 +470,7 @@ struct mlx4_update_qp_params { u16 rate_val; }; +struct mlx4_qp *mlx4_qp_lookup(struct mlx4_dev *dev, u32 qpn); int mlx4_update_qp(struct mlx4_dev *dev, u32 qpn, enum mlx4_update_qp_attr attr, struct mlx4_update_qp_params *params);