net/mlx5e: Extend encap entry with reference counter

bug report Dan Carpenter 2 participants 3 messages Aug 14, 2019 InfiniBand

Share & tools

Vlad Buslov

2019-08-14 13:49 UTC #1 lore raw

Quoted 10 lines · Dan Carpenter

[ I already wrote this email, but it looks like I deleted it instead of
sending it. So weird. I hopefully don't send it twice! ]
Hi Vlad,
I noticed a possible refcounting bug in commit 948993f2beeb ("net/mlx5e:
Extend encap entry with reference counter") from Jun 3, 2018.
	drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:1435 mlx5e_tc_update_neigh_used_value()
	error: dereferencing freed memory 'e'

Hi Dan,

Thanks for reporting!

Quoted 29 lines

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

1415 void mlx5e_tc_update_neigh_used_value(struct mlx5e_neigh_hash_entry *nhe) 1416 {

  1417          struct mlx5e_neigh *m_neigh = &nhe->m_neigh;
  1418          struct mlx5e_tc_flow *flow;
  1419          struct mlx5e_encap_entry *e;
  1420          struct mlx5_fc *counter;
  1421          struct neigh_table *tbl;
  1422          bool neigh_used = false;
  1423          struct neighbour *n;
  1424          u64 lastuse;
  1425
  1426          if (m_neigh->family == AF_INET)
  1427                  tbl = &arp_tbl;
  1428  #if IS_ENABLED(CONFIG_IPV6)
  1429          else if (m_neigh->family == AF_INET6)
  1430                  tbl = &nd_tbl;
  1431  #endif
  1432          else
  1433                  return;
  1434
  1435          list_for_each_entry_safe(e, tmp, &nhe->encap_list, encap_list) {
  1436                  struct encap_flow_item *efi, *tmp;
  1437
  1438                  if (!(e->flags & MLX5_ENCAP_ENTRY_VALID) ||
  1439                      !mlx5e_encap_take(e))
                            ^^^^^^^^^^^^^^^^^^^

Quoted 1 line

We take a reference here.

Okay, we take reference to encap at the beginning of outer loop.

Quoted 8 lines

  1440                          continue;
  1441
  1442                  list_for_each_entry_safe(efi, tmp, &e->flows, list) {
  1443                          flow = container_of(efi, struct mlx5e_tc_flow,
  1444                                              encaps[efi->index]);
  1445                          if (IS_ERR(mlx5e_flow_get(flow)))
  1446                                  continue;

Now we take reference to flow at the beginning of the inner loop.

Quoted 11 lines

  1447
  1448                          if (mlx5e_is_offloaded_flow(flow)) {
  1449                                  counter = mlx5e_tc_get_counter(flow);
  1450                                  lastuse = mlx5_fc_query_lastuse(counter);
  1451                                  if (time_after((unsigned long)lastuse, nhe->reported_lastuse)) {
  1452                                          mlx5e_flow_put(netdev_priv(e->out_dev), flow);
  1453                                          neigh_used = true;
  1454                                          break;

I think we need to call mlx5e_encap_put(netdev_priv(e->out_dev), e); before this break;

We are breaking from the inner loop (not returning from the function, just breaking the innermost loop) here after releasing the reference to flow which was obtained at the beginning of the loop.

Quoted 9 lines

  1455                                  }
  1456                          }
  1457
  1458                          mlx5e_flow_put(netdev_priv(e->out_dev), flow);
  1459                  }
  1460
  1461                  mlx5e_encap_put(netdev_priv(e->out_dev), e);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Now we are releasing the reference to encap, both when inner loop completed normally or when it was terminated with 'break'. If we were to release the encap before the break, this would be the second time we 'put' it.

What am I missing?

Quoted 7 lines

1462 if (neigh_used)
1463 break;
1464 }
1465
regards,
dan carpenter

Dan Carpenter

author

2019-08-14 14:00 UTC #2 lore raw

Quoted 9 lines · Vlad Buslov

Quoted 9 lines

  1447
  1448                          if (mlx5e_is_offloaded_flow(flow)) {
  1449                                  counter = mlx5e_tc_get_counter(flow);
  1450                                  lastuse = mlx5_fc_query_lastuse(counter);
  1451                                  if (time_after((unsigned long)lastuse, nhe->reported_lastuse)) {
  1452                                          mlx5e_flow_put(netdev_priv(e->out_dev), flow);
  1453                                          neigh_used = true;
  1454                                          break;

Quoted 7 lines

Quoted 2 lines
I think we need to call mlx5e_encap_put(netdev_priv(e->out_dev), e); before this break;
We are breaking from the inner loop (not returning from the function, just breaking the innermost loop) here after releasing the reference to flow which was obtained at the beginning of the loop.

Oh... Duh. I am embarrassed. I misread it to think we were breaking from the outer loop.

regards,
dan carpenter