From 973b6f69bfab11ec0ebc4df32aea65d12189e5bb Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Tue, 28 Jun 2022 10:29:42 +0200 Subject: [PATCH] sys/net/netopt: change NETOPT_STATS semantics Instead of retrieving a pointer with NETOPT_STATS, retrieve the current data. This avoids data corruptions when reading from one thread (e.g. the thread running the shell (ifconfig command)) while another thread is updating it (e.g. the netif thread). The issue affects all boards, as users typically expect the count of TX packets and the number of TX bytes to refer to the same state. For 16 bit and 8 bit platforms even a single netstat entry can read back corrupted. This fixes the issue by just copying the whole netstat_t struct over without requiring explicit locking on the user side. A multi-threaded network stack still needs to synchronize the thread responding to netopt_get with the thread writing to the netstat_t structure, but that is an implementation detail no relevant to the user of the API. --- sys/include/net/netopt.h | 9 +-- sys/net/gnrc/netif/gnrc_netif.c | 65 ++++++++++++++++----- sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c | 16 +++++ sys/shell/commands/sc_gnrc_netif.c | 24 ++++---- 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/sys/include/net/netopt.h b/sys/include/net/netopt.h index 1580386740..f2258ade82 100644 --- a/sys/include/net/netopt.h +++ b/sys/include/net/netopt.h @@ -391,11 +391,12 @@ typedef enum { NETOPT_CCA_MODE, /** - * @brief (@ref netstats_t*) get statistics about sent and received packets and data of the - * device or protocol + * @brief (@ref netstats_t) get statistics about sent and received packets + * and data of the device or protocol * - * Expects a pointer to a @ref netstats_t struct that will be pointed to - * the corresponding @ref netstats_t of the module. + * A get operation expects a @ref netstats_t and will copy the current + * statistics into it, atomically. A set operation resets the statistics + * (zeros it out) regardless of the parameter given. */ NETOPT_STATS, diff --git a/sys/net/gnrc/netif/gnrc_netif.c b/sys/net/gnrc/netif/gnrc_netif.c index f6bcbf7eec..0aa690e08d 100644 --- a/sys/net/gnrc/netif/gnrc_netif.c +++ b/sys/net/gnrc/netif/gnrc_netif.c @@ -186,25 +186,35 @@ int gnrc_netif_get_from_netdev(gnrc_netif_t *netif, gnrc_netapi_opt_t *opt) res = sizeof(uint8_t); break; case NETOPT_STATS: - /* XXX discussed this with Oleg, it's supposed to be a pointer */ switch ((int16_t)opt->context) { #if IS_USED(MODULE_NETSTATS_IPV6) && IS_USED(MODULE_GNRC_NETIF_IPV6) - case NETSTATS_IPV6: - assert(opt->data_len == sizeof(netstats_t *)); - *((netstats_t **)opt->data) = &netif->ipv6.stats; - res = sizeof(&netif->ipv6.stats); - break; + case NETSTATS_IPV6: + { + assert(opt->data_len == sizeof(netstats_t)); + /* IPv6 thread is updating this, to prevent data + * corruptions, we have to guarantee mutually exclusive + * access */ + unsigned irq_state = irq_disable(); + memcpy(opt->data, &netif->ipv6.stats, + sizeof(netif->ipv6.stats)); + irq_restore(irq_state); + res = sizeof(netif->ipv6.stats); + } + break; #endif #ifdef MODULE_NETSTATS_L2 - case NETSTATS_LAYER2: - assert(opt->data_len == sizeof(netstats_t *)); - *((netstats_t **)opt->data) = &netif->stats; - res = sizeof(&netif->stats); - break; + case NETSTATS_LAYER2: + assert(opt->data_len == sizeof(netstats_t)); + /* this is only accesses from the netif thread (us), so no need + * to lock this */ + memcpy(opt->data, &netif->stats, + sizeof(netif->stats)); + res = sizeof(netif->stats); + break; #endif - default: - /* take from device */ - break; + default: + /* take from device */ + break; } break; #if IS_USED(MODULE_GNRC_NETIF_IPV6) @@ -413,6 +423,33 @@ int gnrc_netif_set_from_netdev(gnrc_netif_t *netif, opt->data_len); res = sizeof(netopt_enable_t); break; + case NETOPT_STATS: + switch ((int16_t)opt->context) { +#if IS_USED(MODULE_NETSTATS_IPV6) && IS_USED(MODULE_GNRC_NETIF_IPV6) + case NETSTATS_IPV6: + { + /* IPv6 thread is updating this, to prevent data + * corruptions, we have to guarantee mutually exclusive + * access */ + unsigned irq_state = irq_disable(); + memset(&netif->ipv6.stats, 0, sizeof(netif->ipv6.stats)); + irq_restore(irq_state); + res = 0; + } + break; +#endif +#ifdef MODULE_NETSTATS_L2 + case NETSTATS_LAYER2: + /* this is only accesses from the netif thread (us), so no need + * to lock this */ + memset(&netif->stats, 0, sizeof(netif->stats)); + res = 0; + break; +#endif + default: + /* take from device */ + break; + } default: break; } diff --git a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c index 2de00e89ae..89b03d98b0 100644 --- a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c +++ b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c @@ -271,8 +271,12 @@ static void _send_to_iface(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt) ipv6_addr_to_str(addr_str, &hdr->dst, sizeof(addr_str)), hdr->nh, byteorder_ntohs(hdr->len)); #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_success++; netif->ipv6.stats.tx_bytes += gnrc_pkt_len(pkt->next); + irq_restore(irq_state); #endif #ifdef MODULE_GNRC_SIXLOWPAN @@ -510,7 +514,11 @@ static void _send_unicast(gnrc_pktsnip_t *pkt, bool prep_hdr, netif->pid); /* and send to interface */ #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_unicast_count++; + irq_restore(irq_state); #endif _send_to_iface(netif, pkt); } @@ -533,7 +541,11 @@ static inline void _send_multicast_over_iface(gnrc_pktsnip_t *pkt, } DEBUG("ipv6: send multicast over interface %" PRIkernel_pid "\n", netif->pid); #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_mcast_count++; + irq_restore(irq_state); #endif /* and send to interface */ _send_to_iface(netif, pkt); @@ -737,9 +749,13 @@ static void _receive(gnrc_pktsnip_t *pkt) netif = gnrc_netif_hdr_get_netif(netif_hdr->data); #ifdef MODULE_NETSTATS_IPV6 assert(netif != NULL); + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netstats_t *stats = &netif->ipv6.stats; stats->rx_count++; stats->rx_bytes += (gnrc_pkt_len(pkt) - netif_hdr->size); + irq_restore(irq_state); #endif } diff --git a/sys/shell/commands/sc_gnrc_netif.c b/sys/shell/commands/sc_gnrc_netif.c index f9233ca111..94e28ccca8 100644 --- a/sys/shell/commands/sc_gnrc_netif.c +++ b/sys/shell/commands/sc_gnrc_netif.c @@ -152,16 +152,18 @@ static const char *_netstats_module_to_str(uint8_t module) static int _netif_stats(netif_t *iface, unsigned module, bool reset) { - netstats_t *stats; + netstats_t stats; int res = netif_get_opt(iface, NETOPT_STATS, module, &stats, - sizeof(&stats)); + sizeof(stats)); if (res < 0) { puts(" Protocol or device doesn't provide statistics."); } else if (reset) { - memset(stats, 0, sizeof(netstats_t)); - printf("Reset statistics for module %s!\n", _netstats_module_to_str(module)); + res = netif_set_opt(iface, NETOPT_STATS, module, NULL, 0); + printf("Reset statistics for module %s: %s!\n", + _netstats_module_to_str(module), + (res < 0) ? "failed" : "succeeded"); } else { printf(" Statistics for %s\n" @@ -169,13 +171,13 @@ static int _netif_stats(netif_t *iface, unsigned module, bool reset) " TX packets %u (Multicast: %u) bytes %u\n" " TX succeeded %u errors %u\n", _netstats_module_to_str(module), - (unsigned) stats->rx_count, - (unsigned) stats->rx_bytes, - (unsigned) (stats->tx_unicast_count + stats->tx_mcast_count), - (unsigned) stats->tx_mcast_count, - (unsigned) stats->tx_bytes, - (unsigned) stats->tx_success, - (unsigned) stats->tx_failed); + (unsigned)stats.rx_count, + (unsigned)stats.rx_bytes, + (unsigned)(stats.tx_unicast_count + stats.tx_mcast_count), + (unsigned)stats.tx_mcast_count, + (unsigned)stats.tx_bytes, + (unsigned)stats.tx_success, + (unsigned)stats.tx_failed); res = 0; } return res;