kaspar/RIOT
1
0
Fork 0
mirror of https://github.com/RIOT-OS/RIOT.git synced 2025-12-25 14:33:52 +01:00
Code Releases Activity

sys/net/netopt: change NETOPT_STATS semantics

Browse Source 
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.
This commit is contained in:
Marian Buschsieweke 2022-06-28 10:29:42 +02:00
parent 1d2547558f
commit 973b6f69bf
No known key found for this signature in database
GPG Key ID: CB8E3238CE715A94
4 changed files with 85 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
sys/include/net/netopt.h
View File

@ -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,

65
sys/net/gnrc/netif/gnrc_netif.c
View File

@ -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;
}

16
sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c
View File

@ -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
}

24
sys/shell/commands/sc_gnrc_netif.c
View File

@ -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;