Re: [bug report] net/mlx5e: Implement CT entry update

Re: [bug report] net/mlx5e: Implement CT entry update

[Leon Romanovsky]

On Thu, Jun 20, 2024 at 11:50:33AM +0300, Dan Carpenter wrote:
> Hello Vlad Buslov,
> 
> Commit 94ceffb48eac ("net/mlx5e: Implement CT entry update") from Dec
> 1, 2022 (linux-next), leads to the following Smatch static checker
> warning:
> 
> 	drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c:1163 mlx5_tc_ct_entry_replace_rules()
> 	error: uninitialized symbol 'err'.

This error was introduced by the patch 49d37d05f216 ("net/mlx5: CT: Separate CT and CT-NAT tuple entries")
https://lore.kernel.org/all/20240613210036.1125203-3-tariqt@nvidia.com/

Thanks

> 
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>     1142 static int
>     1143 mlx5_tc_ct_entry_replace_rules(struct mlx5_tc_ct_priv *ct_priv,
>     1144                                struct flow_rule *flow_rule,
>     1145                                struct mlx5_ct_entry *entry,
>     1146                                u8 zone_restore_id)
>     1147 {
>     1148         int err;
>     1149 
>     1150         if (mlx5_tc_ct_entry_in_ct_table(entry)) {
>     1151                 err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, false,
>     1152                                                     zone_restore_id);
>     1153                 if (err)
>     1154                         return err;
>     1155         }
>     1156 
>     1157         if (mlx5_tc_ct_entry_in_ct_nat_table(entry)) {
>     1158                 err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, true,
>     1159                                                     zone_restore_id);
>     1160                 if (err && mlx5_tc_ct_entry_in_ct_table(entry))
>     1161                         mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
>     1162         }
> 
> Can the entry not be in either table?
> 
> --> 1163         return err;
> 
> If so then err is uninitialized.
> 
>     1164 }
> 
> regards,
> dan carpenter
> 

Re: [bug report] net/mlx5e: Implement CT entry update

[Cosmin Ratiu]

On Mon, 2024-06-24 at 14:09 +0300, Leon Romanovsky wrote:
> On Thu, Jun 20, 2024 at 11:50:33AM +0300, Dan Carpenter wrote:
> > Hello Vlad Buslov,
> > 
> > Commit 94ceffb48eac ("net/mlx5e: Implement CT entry update") from Dec
> > 1, 2022 (linux-next), leads to the following Smatch static checker
> > warning:
> > 
> > 	drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c:1163 mlx5_tc_ct_entry_replace_rules()
> > 	error: uninitialized symbol 'err'.
> 
> This error was introduced by the patch 49d37d05f216 ("net/mlx5: CT: Separate CT and CT-NAT tuple entries")
> https://lore.kernel.org/all/20240613210036.1125203-3-tariqt@nvidia.com/

Yes, Leon is correct.
We also caught the error in our static checker runs but the patch was
already sent to net-next before we could alter it with the fix.

> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> >     1142 static int
> >     1143 mlx5_tc_ct_entry_replace_rules(struct mlx5_tc_ct_priv *ct_priv,
> >     1144                                struct flow_rule *flow_rule,
> >     1145                                struct mlx5_ct_entry *entry,
> >     1146                                u8 zone_restore_id)
> >     1147 {
> >     1148         int err;
> >     1149 
> >     1150         if (mlx5_tc_ct_entry_in_ct_table(entry)) {
> >     1151                 err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, false,
> >     1152                                                     zone_restore_id);
> >     1153                 if (err)
> >     1154                         return err;
> >     1155         }
> >     1156 
> >     1157         if (mlx5_tc_ct_entry_in_ct_nat_table(entry)) {
> >     1158                 err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, true,
> >     1159                                                     zone_restore_id);
> >     1160                 if (err && mlx5_tc_ct_entry_in_ct_table(entry))
> >     1161                         mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
> >     1162         }
> > 
> > Can the entry not be in either table?
> > 
> > --> 1163         return err;
> > 
> > If so then err is uninitialized.

In practice, the entry will definitely be in at least one of the tables
(if not both), so the uninitialized err will never happen in practice.
Regardless, to fix the noise, there's a patch already pending
upstreaming, about to be sent by Tariq soon.

Cosmin.