On Mon, Sep 28, 2020 at 06:31:04PM +0000, Ariel Levkovich wrote:
> On Sep 28, 2020, at 13:42, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > The mlx5_tc_ct_init() function doesn't return error pointers it returns
> > NULL. Also we need to set the error codes on this path.
> >
> > Fixes: aedd133d17bc ("net/mlx5e: Support CT offload for tc nic flows")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index 104b1c339de0..438fbcf478d1 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -5224,8 +5224,10 @@ int mlx5e_tc_nic_init(struct mlx5e_priv *priv)
> >
> > tc->ct = mlx5_tc_ct_init(priv, tc->chains, &priv->fs.tc.mod_hdr,
> > MLX5_FLOW_NAMESPACE_KERNEL);
> > - if (IS_ERR(tc->ct))
> > + if (!tc->ct) {
> > + err = -ENOMEM;
> > goto err_ct;
> > + }
>
> Hi Dan,
> That was implement like that on purpose. If mlx5_tc_init_ct returns
> NULL it means the device doesn’t support CT offload which can happen
> with older devices or old FW on the devices.
> However, in this case we want to continue with the rest of the Tc
> initialization because we can still support other TC offloads. No
> need to fail the entire TC init in this case. Only if mlx5_tc_init_ct
> return err_ptr that means the tc init failed not because of lack of
> support but due to a real error and only then we want to fail the rest
> of the tc init.
>
> Your change will break compatibility for devices/FW versions that
> don’t have CT offload support.
>
I should have looked at this more closely. It seems the bug is in
mlx5_tc_ct_init().
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
1897 struct mlx5_tc_ct_priv *
1898 mlx5_tc_ct_init(struct mlx5e_priv *priv, struct mlx5_fs_chains *chains,
1899 struct mod_hdr_tbl *mod_hdr,
1900 enum mlx5_flow_namespace_type ns_type)
1901 {
1902 struct mlx5_tc_ct_priv *ct_priv;
1903 struct mlx5_core_dev *dev;
1904 const char *msg;
1905 int err;
1906
1907 dev = priv->mdev;
1908 err = mlx5_tc_ct_init_check_support(priv, ns_type, &msg);
1909 if (err) {
1910 mlx5_core_warn(dev,
1911 "tc ct offload not supported, %s\n",
1912 msg);
1913 goto err_support;
This should probably return NULL and it does.
1914 }
1915
1916 ct_priv = kzalloc(sizeof(*ct_priv), GFP_KERNEL);
1917 if (!ct_priv)
1918 goto err_alloc;
This should probably return an ERR_PTR(-ENOMEM) but it instead returns
NULL.
1919
1920 ct_priv->zone_mapping = mapping_create(sizeof(u16), 0, true);
1921 if (IS_ERR(ct_priv->zone_mapping)) {
1922 err = PTR_ERR(ct_priv->zone_mapping);
1923 goto err_mapping_zone;
^^^^^^^^^^^^^^^^^^^^^^
This sets "err" but it still returns NULL.
Then in the caller if the mlx5_tc_ct_init() call returns an error
pointer, it should set the error code. (NULL is a special case of
success etc).
Can you fix this and give me a reported-by tag? I think my new analysis
is correct...
regards,
dan carpenter
