[PATCH v4] Add rx_bytes64 and tx_bytes64 and use them for RADIUS accounting

[Jouni Malinen]

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