[bug report] net/mlx5e: Support CT offload for tc nic flows

[bug report] net/mlx5e: Support CT offload for tc nic flows

[Dan Carpenter]

Hello Ariel Levkovich,

The patch aedd133d17bc: "net/mlx5e: Support CT offload for tc nic
flows" from Jul 21, 2020, leads to the following static checker
warning:

	drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:1132 mlx5e_tc_del_nic_flow()
	warn: passing freed memory 'flow'

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
  1105  static void mlx5e_tc_del_nic_flow(struct mlx5e_priv *priv,
  1106                                    struct mlx5e_tc_flow *flow)
  1107  {
  1108          struct mlx5_flow_attr *attr = flow->attr;
  1109          struct mlx5e_tc_table *tc = &priv->fs.tc;
  1110  
  1111          flow_flag_clear(flow, OFFLOADED);
  1112  
  1113          if (flow_flag_test(flow, CT))
  1114                  mlx5_tc_ct_delete_flow(get_ct_priv(flow->priv), flow, attr);
                                                                        ^^^^
I guess this used to free "flow" and Smatch's db hasn't totally caught
up yet.  Now it doesn't use "flow" at all.  Maybe we could just remove
that parameter?

  1115          else if (!IS_ERR_OR_NULL(flow->rule[0]))
  1116                  mlx5e_del_offloaded_nic_rule(priv, flow->rule[0], attr);
  1117  
  1118          /* Remove root table if no rules are left to avoid
  1119           * extra steering hops.
  1120           */
  1121          mutex_lock(&priv->fs.tc.t_lock);
  1122          if (!mlx5e_tc_num_filters(priv, MLX5_TC_FLAG(NIC_OFFLOAD)) &&
  1123              !IS_ERR_OR_NULL(tc->t)) {
  1124                  mlx5_chains_put_table(nic_chains(priv), 0, 1, MLX5E_TC_FT_LEVEL);
  1125                  priv->fs.tc.t = NULL;
  1126          }
  1127          mutex_unlock(&priv->fs.tc.t_lock);
  1128  
  1129          kvfree(attr->parse_attr);
  1130  
  1131          if (attr->action & MLX5_FLOW_CONTEXT_ACTION_MOD_HDR)
  1132                  mlx5e_detach_mod_hdr(priv, flow);
                                                   ^^^^
  1133  
  1134          mlx5_fc_destroy(priv->mdev, attr->counter);
  1135  
  1136          if (flow_flag_test(flow, HAIRPIN))
  1137                  mlx5e_hairpin_flow_del(priv, flow);
  1138  
  1139          kfree(flow->attr);
  1140  }

regards,
dan carpenter

Re: [bug report] net/mlx5e: Support CT offload for tc nic flows

[Ariel Levkovich]

On Sep 28, 2020, at 06:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> Hello Ariel Levkovich,
> 
> The patch aedd133d17bc: "net/mlx5e: Support CT offload for tc nic
> flows" from Jul 21, 2020, leads to the following static checker
> warning:
> 
>    drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:1132 mlx5e_tc_del_nic_flow()
>    warn: passing freed memory 'flow'
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>  1105  static void mlx5e_tc_del_nic_flow(struct mlx5e_priv *priv,
>  1106                                    struct mlx5e_tc_flow *flow)
>  1107  {
>  1108          struct mlx5_flow_attr *attr = flow->attr;
>  1109          struct mlx5e_tc_table *tc = &priv->fs.tc;
>  1110  
>  1111          flow_flag_clear(flow, OFFLOADED);
>  1112  
>  1113          if (flow_flag_test(flow, CT))
>  1114                  mlx5_tc_ct_delete_flow(get_ct_priv(flow->priv), flow, attr);
>                                                                        ^^^^
> I guess this used to free "flow" and Smatch's db hasn't totally caught
> up yet.  Now it doesn't use "flow" at all.  Maybe we could just remove
> that parameter?

Hi Dan, thanks for pointing this out.
Yes, we can simply drop the flow and use priv directly as it is passed to the function as a separate variable.

Ariel

> 
>  1115          else if (!IS_ERR_OR_NULL(flow->rule[0]))
>  1116                  mlx5e_del_offloaded_nic_rule(priv, flow->rule[0], attr);
>  1117  
>  1118          /* Remove root table if no rules are left to avoid
>  1119           * extra steering hops.
>  1120           */
>  1121          mutex_lock(&priv->fs.tc.t_lock);
>  1122          if (!mlx5e_tc_num_filters(priv, MLX5_TC_FLAG(NIC_OFFLOAD)) &&
>  1123              !IS_ERR_OR_NULL(tc->t)) {
>  1124                  mlx5_chains_put_table(nic_chains(priv), 0, 1, MLX5E_TC_FT_LEVEL);
>  1125                  priv->fs.tc.t = NULL;
>  1126          }
>  1127          mutex_unlock(&priv->fs.tc.t_lock);
>  1128  
>  1129          kvfree(attr->parse_attr);
>  1130  
>  1131          if (attr->action & MLX5_FLOW_CONTEXT_ACTION_MOD_HDR)
>  1132                  mlx5e_detach_mod_hdr(priv, flow);
>                                                   ^^^^
>  1133  
>  1134          mlx5_fc_destroy(priv->mdev, attr->counter);
>  1135  
>  1136          if (flow_flag_test(flow, HAIRPIN))
>  1137                  mlx5e_hairpin_flow_del(priv, flow);
>  1138  
>  1139          kfree(flow->attr);
>  1140  }
> 
> regards,
> dan carpenter