On Fri, Feb 19, 2016 at 03:26:01PM +0000, Nick Lowe wrote:
> Add rx_bytes64 and tx_bytes64 and populate them using
> NL80211_STA_INFO_RX_BYTES64 and NL80211_STA_INFO_TX_BYTES64 where support for
> these are available. This resolves the race vulnerable 32-bit value
> wrap/overflow. Rework RADIUS accounting to use these for Acct-Input-Octets,
> Acct-Input-Gigawords, Acct-Output-Octets and Acct-Output-Gigawords, these
> values are often used for billing purposes.
Thanks, applied with some cleanup.
> diff --git a/src/ap/accounting.c b/src/ap/accounting.c
> @@ -167,19 +167,30 @@ static int accounting_sta_update_stats(struct
> - if (sta->last_rx_bytes > data->rx_bytes)
> - sta->acct_input_gigawords++;
> - if (sta->last_tx_bytes > data->tx_bytes)
> - sta->acct_output_gigawords++;
> - sta->last_rx_bytes = data->rx_bytes;
> - sta->last_tx_bytes = data->tx_bytes;
> + if (sta->last_rx_bytes_lo > data->rx_bytes)
> + sta->last_rx_bytes_hi++;
> + sta->last_rx_bytes_lo = data->rx_bytes;
> +
> + if (sta->last_tx_bytes_lo > data->tx_bytes)
> + sta->last_tx_bytes_hi++;
> + sta->last_tx_bytes_lo = data->tx_bytes;
> +
> + sta->last_rx_bytes64 = data->rx_bytes64;
> + sta->last_tx_bytes64 = data->tx_bytes64;
There is no need to store a copy of the 64-bit values from struct
hostap_sta_driver_data, i.e., only the 32-bit lo/hi values are needed to
be able to extend 32-bit driver counters into 64-bit counters. The
64-bit value can be used directly. In addition, these extension
operations can be made conditional on the driver counters actually being
32-bit in length.
> diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c
> @@ -35,9 +35,12 @@ static int hostapd_get_sta_tx_rx(struct hostapd_data *hapd,
> ret = os_snprintf(buf, buflen, "rx_packets=%lu\ntx_packets=%lu\n"
> - "rx_bytes=%lu\ntx_bytes=%lu\n",
> + "rx_bytes=%lu\ntx_bytes=%lu\n"
> + "rx_bytes64=%llu\ntx_bytes64=%llu\n",
> data.rx_packets, data.tx_packets,
> - data.rx_bytes, data.tx_bytes);
> + data.rx_bytes, data.tx_bytes,
> + data.rx_bytes64, data.tx_bytes64
> + );
It looks cleaner to just print the 64-bit values here since this does
not even try to use the extended version.
> diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
> @@ -107,10 +107,12 @@ struct sta_info {
> - unsigned long last_rx_bytes;
> - unsigned long last_tx_bytes;
> - u32 acct_input_gigawords; /* Acct-Input-Gigawords */
> - u32 acct_output_gigawords; /* Acct-Output-Gigawords */
> + unsigned long last_rx_bytes_hi;
> + unsigned long last_rx_bytes_lo;
> + unsigned long last_tx_bytes_hi;
> + unsigned long last_tx_bytes_lo;
> + unsigned long long last_rx_bytes64;
> + unsigned long long last_tx_bytes64;
I dropped those last two to save 16 bytes per STA entry.
> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> @@ -1377,6 +1377,7 @@ struct hostap_sta_driver_data {
> unsigned long tx_retry_count;
> int last_rssi;
> int last_ack_rssi;
> + unsigned long long rx_bytes64, tx_bytes64;
This looks unnecessary complexity, i.e., I replaced the existing
tx/rx_bytes values with 64-bit versions and added an indication if the
driver supports 64-bit counters.
> diff --git a/src/drivers/driver_hostap.c b/src/drivers/driver_hostap.c
> @@ -597,15 +596,18 @@ static int hostap_read_sta_data(void *priv,
> + else if (strcmp(line, "rx_bytes64") == 0)
> + data->rx_bytes64 = strtoull(pos, NULL, 20);
> + else if (strcmp(line, "tx_bytes64") == 0)
> + data->tx_bytes64 = strtoull(pos, NULL, 20);
The hostap driver does not provide 64-bit values, i.e., these
rx_bytes64 and tx_bytes64 do not exist. As such, I dropped all changes
to driver_hostap.c.
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> @@ -5365,6 +5367,10 @@ static int get_sta_handler(struct nl_msg *msg, void *arg)
> if (stats[NL80211_STA_INFO_TX_FAILED])
> data->tx_retry_failed =
> nla_get_u32(stats[NL80211_STA_INFO_TX_FAILED]);
> + if (stats[NL80211_STA_INFO_RX_BYTES64])
> + data->rx_bytes64 = nla_get_u64(stats[NL80211_STA_INFO_RX_BYTES64]);
> + if (stats[NL80211_STA_INFO_TX_BYTES64])
> + data->tx_bytes64 = nla_get_u64(stats[NL80211_STA_INFO_TX_BYTES64]);
This is quite a bit clearer with the design I mentioned above, i.e.,
making tx/rx_bytes 64-bit and only exposing a single pair of values from
the driver interface code.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
