On 1/4/23 19:15, Mike Pattrick wrote:
> Currently tc offload flow packet counters will roll over every ~4
> billion packets. This is because the packet counter in struct
> tc_stats provided by TCA_STATS_BASIC is a 32bit integer.
>
> Now we check for the optional TCA_STATS_PKT64 attribute which provides
> the full 64bit packet counter if the 32bit one has rolled over. Because
> the TCA_STATS_PKT64 attribute may appear multiple times in a netlink
> message, the method of parsing attributes was changed.
>
> Fixes: f98e418fbdb6 ("tc: Add tc flower functions")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816
> Acked-by: Eelco Chaudron <[email protected]>
> Signed-off-by: Mike Pattrick <[email protected]>
>
> ---
>
> Since v1:
> - Retain support for pre-TCA_STATS_PKT64 kernels
> Since v2:
> - Added compat header
> Since v3:
> - Rebased on to current master
> Since v4:
> - Fixed alignment issue
> - Moved declarations and definitions
> Since v5:
> -Aesthetic changes
> Since v6:
> -Aesthetic changes introduced a compilation issue on some compilers
> Since v7:
> -Coding standards
>
> ---
> lib/tc.c | 115 +++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 73 insertions(+), 42 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index a66dc432f..fbb7522db 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -84,6 +84,11 @@ struct flower_key_to_pedit {
> int boundary_shift;
> };
>
> +struct tc_flow_stats {
> + uint64_t n_packets;
> + uint64_t n_bytes;
> +};
> +
> static struct flower_key_to_pedit flower_pedit_map[] = {
> {
> TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> @@ -1852,66 +1857,92 @@ static const struct nl_policy act_policy[] = {
> [TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, },
> };
>
> -static const struct nl_policy stats_policy[] = {
> - [TCA_STATS_BASIC] = { .type = NL_A_UNSPEC,
> - .min_len = sizeof(struct gnet_stats_basic),
> - .optional = false, },
> - [TCA_STATS_BASIC_HW] = { .type = NL_A_UNSPEC,
> - .min_len = sizeof(struct gnet_stats_basic),
> - .optional = true, },
> - [TCA_STATS_QUEUE] = { .type = NL_A_UNSPEC,
> - .min_len = sizeof(struct gnet_stats_queue),
> - .optional = true, },
> -};
> -
> static int
> nl_parse_action_stats(struct nlattr *act_stats,
> struct ovs_flow_stats *stats_sw,
> struct ovs_flow_stats *stats_hw,
> struct ovs_flow_stats *stats_dropped)
> {
> - struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> - struct gnet_stats_basic bs_all, bs_sw, bs_hw;
> - const struct gnet_stats_queue *qs;
> + struct tc_flow_stats s_sw = {0}, s_hw = {0};
> + uint16_t prev_type = __TCA_STATS_MAX;
> + const struct nlattr *nla;
> + uint32_t s_dropped = 0;
> + unsigned int seen = 0;
> + size_t left;
>
> - if (!nl_parse_nested(act_stats, stats_policy, stats_attrs,
> - ARRAY_SIZE(stats_policy))) {
> - VLOG_ERR_RL(&error_rl, "Failed to parse action stats policy");
> - return EPROTO;
> - }
> + /* Cannot use nl_parse_nested due to duplicate attributes. */
> + NL_NESTED_FOR_EACH (nla, left, act_stats) {
> + const struct gnet_stats_basic *stats_basic;
> + const struct gnet_stats_queue *qs;
> + uint16_t type = nl_attr_type(nla);
> + uint64_t packets;
>
> - memcpy(&bs_all,
> - nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof bs_all),
> - sizeof bs_all);
> - if (stats_attrs[TCA_STATS_BASIC_HW]) {
> - memcpy(&bs_hw, nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> - sizeof bs_hw),
> - sizeof bs_hw);
> + seen |= 1 << type;
>
> - bs_sw.packets = bs_all.packets - bs_hw.packets;
> - bs_sw.bytes = bs_all.bytes - bs_hw.bytes;
> - } else {
> - bs_sw.packets = bs_all.packets;
> - bs_sw.bytes = bs_all.bytes;
> + switch (type) {
> + case TCA_STATS_BASIC:
> + stats_basic = nl_attr_get_unspec(nla, sizeof *stats_basic);
> + s_sw.n_packets = get_unaligned_u32(&stats_basic->packets);
> + s_sw.n_bytes = get_unaligned_u64(&stats_basic->bytes);
It's not enough to use unaligned access functions, unfortunately.
The '->' operation itself on a misaligned pointer is a UB. We have
to memcpy the structure before accessing:
lib/tc.c:1891:62: runtime error: member access within misaligned address
0x619000019b1c for type 'const struct gnet_stats_basic', which requires 8 byte
alignment
0x619000019b1c: note: pointer points here
14 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 18 00 03 00 00
00 00 00 00 00 00 00
^
#0 0x55f794d21a63 in nl_parse_action_stats /root/ovs/lib/tc.c:1891:62
#1 0x55f794d2c2d6 in nl_parse_single_action /root/ovs/lib/tc.c:2007:12
#2 0x55f794d25d6e in nl_parse_flower_actions /root/ovs/lib/tc.c:2062:19
#3 0x55f794d25d6e in nl_parse_flower_options /root/ovs/lib/tc.c:2177:12
#4 0x55f794d2286d in parse_netlink_to_tc_flower /root/ovs/lib/tc.c:2225:12
#5 0x55f794d2de34 in tc_replace_flower /root/ovs/lib/tc.c:3818:19
#6 0x55f794cf1dd5 in probe_tc_block_support
/root/ovs/lib/netdev-offload-tc.c:2629:13
#7 0x55f794cf1dd5 in netdev_tc_init_flow_api
/root/ovs/lib/netdev-offload-tc.c:2760:9
#8 0x55f7948ffe4b in netdev_assign_flow_api
/root/ovs/lib/netdev-offload.c:184:14
#9 0x55f7948ffe4b in netdev_init_flow_api
/root/ovs/lib/netdev-offload.c:400:9
#10 0x55f7949026ff in netdev_ports_insert
/root/ovs/lib/netdev-offload.c:750:5
#11 0x55f794809618 in do_open /root/ovs/lib/dpif.c:369:17
#12 0x55f7948099a3 in dpif_create /root/ovs/lib/dpif.c:404:12
#13 0x55f7948099a3 in dpif_create_and_open /root/ovs/lib/dpif.c:417:13
#14 0x55f7945bf3cd in open_dpif_backer
/root/ovs/ofproto/ofproto-dpif.c:772:13
#15 0x55f7945bf3cd in construct /root/ovs/ofproto/ofproto-dpif.c:1668:13
#16 0x55f794554193 in ofproto_create /root/ovs/ofproto/ofproto.c:554:13
#17 0x55f7945073e5 in bridge_reconfigure /root/ovs/vswitchd/bridge.c:882:21
#18 0x55f79450018d in bridge_run /root/ovs/vswitchd/bridge.c:3291:9
#19 0x55f794539aff in main /root/ovs/vswitchd/ovs-vswitchd.c:129:9
#20 0x7f81cd02350f (/lib/x86_64-linux-gnu/libc.so.6+0x2350f) (BuildId:
d1704d25fbbb72fa95d517b883131828c0883fe9)
#21 0x7f81cd0235c8 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x235c8) (BuildId:
d1704d25fbbb72fa95d517b883131828c0883fe9)
#22 0x55f79443cf94 in _start (/root/ovs/vswitchd/ovs-vswitchd+0x6faf94)
(BuildId: c609b982dc98c4ad4af1c723afcf0d394b03c2d4)
You may reproduce that by running 'make check-offloads' with UBSan.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
