Skip to content

Commit

Permalink
bpf: ct: fix off-by-1 in ICMP packet statistics
Browse files Browse the repository at this point in the history
ct_create*() initializes the CT entry's packet/byte counters for the first
packet. As this entry is then also used to create the related ICMP entry,
the counter values leak over. The entry then indicate the processing of an
ICMP packet, when there was actually no such packet.

Fix it by re-ordering ct_create*() to create the ICMP entry first, and
only afterwards initialize the counters.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
  • Loading branch information
julianwiedmann committed May 7, 2024
1 parent 4b2c9ab commit e3346ed
Showing 1 changed file with 24 additions and 20 deletions.
44 changes: 24 additions & 20 deletions bpf/lib/conntrack.h
Original file line number Diff line number Diff line change
Expand Up @@ -953,17 +953,9 @@ static __always_inline int ct_create6(const void *map_main, const void *map_rela
seen_flags.value |= is_tcp ? TCP_FLAG_SYN : 0;
ct_update_timeout(&entry, is_tcp, dir, seen_flags);

#ifdef CONNTRACK_ACCOUNTING
entry.packets = 1;
entry.bytes = ctx_full_len(ctx);
#endif
cilium_dbg3(ctx, DBG_CT_CREATED6, entry.rev_nat_index,
entry.src_sec_id, 0);

err = map_update_elem(map_main, tuple, &entry, 0);
if (unlikely(err < 0))
goto err_ct_fill_up;

if (map_related != NULL) {
/* Create an ICMPv6 entry to relate errors */
struct ipv6_ct_tuple icmp_tuple = {
Expand All @@ -980,6 +972,16 @@ static __always_inline int ct_create6(const void *map_main, const void *map_rela
if (unlikely(err < 0))
goto err_ct_fill_up;
}

#ifdef CONNTRACK_ACCOUNTING
entry.packets = 1;
entry.bytes = ctx_full_len(ctx);
#endif

err = map_update_elem(map_main, tuple, &entry, 0);
if (unlikely(err < 0))
goto err_ct_fill_up;

return 0;

err_ct_fill_up:
Expand Down Expand Up @@ -1008,17 +1010,9 @@ static __always_inline int ct_create4(const void *map_main,
seen_flags.value |= is_tcp ? TCP_FLAG_SYN : 0;
ct_update_timeout(&entry, is_tcp, dir, seen_flags);

#ifdef CONNTRACK_ACCOUNTING
entry.packets = 1;
entry.bytes = ctx_full_len(ctx);
#endif
cilium_dbg3(ctx, DBG_CT_CREATED4, entry.rev_nat_index,
entry.src_sec_id, 0);

err = map_update_elem(map_main, tuple, &entry, 0);
if (unlikely(err < 0))
goto err_ct_fill_up;

if (map_related != NULL) {
/* Create an ICMP entry to relate errors */
struct ipv4_ct_tuple icmp_tuple = {
Expand All @@ -1030,14 +1024,24 @@ static __always_inline int ct_create4(const void *map_main,
.flags = tuple->flags | TUPLE_F_RELATED,
};

/* Previous map update succeeded, we could delete it in case
* the below throws an error, but we might as well just let
* it time out.
*/
err = map_update_elem(map_related, &icmp_tuple, &entry, 0);
if (unlikely(err < 0))
goto err_ct_fill_up;
}

#ifdef CONNTRACK_ACCOUNTING
entry.packets = 1;
entry.bytes = ctx_full_len(ctx);
#endif

/* Previous map update succeeded, we could delete it in case
* the below throws an error, but we might as well just let
* it time out.
*/
err = map_update_elem(map_main, tuple, &entry, 0);
if (unlikely(err < 0))
goto err_ct_fill_up;

return 0;

err_ct_fill_up:
Expand Down

0 comments on commit e3346ed

Please sign in to comment.