diff mbox

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

Message ID CAGnO3dpfCDevqKm4UvecfVNk-DgKSchftmde_r+FUsKzpChovw@mail.gmail.com
State Accepted
Headers show

Commit Message

Nick Lowe Feb. 19, 2016, 3:26 p.m. UTC 
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.

Signed-off-by: Nick Lowe <nick.lowe@lugatech.com>
---
 src/ap/accounting.c          | 75 ++++++++++++++++++++++++++------------------
 src/ap/ctrl_iface_ap.c       |  7 +++--
 src/ap/sta_info.h            | 10 +++---
 src/drivers/driver.h         |  1 +
 src/drivers/driver_hostap.c  | 14 +++++----
 src/drivers/driver_nl80211.c |  6 ++++
 6 files changed, 70 insertions(+), 43 deletions(-)

Comments

Nick Lowe Feb. 19, 2016, 6:45 p.m. UTC | #1 
Jouni,

Is this along the lines that you were thinking?

As far as I can see, we are ok to call NL80211_STA_INFO_RX_BYTES64
where it is not supported, and should get back 0 where it's not.

Caveat: I haven't actually tested it with an older Linux kernel. I'm
curious for your insight therefore! And wanted to point that out.

Cheers,

Nick
Jouni Malinen Feb. 20, 2016, 5:50 p.m. UTC | #2 
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.
Nick Lowe Feb. 20, 2016, 6:47 p.m. UTC | #3 
I did think it would be academically interesting to see where the
detected wraps vs actual 64-bit counter diverge, and that therefore
that the 16 bytes saving wasn't worth it.

Nick
diff mbox

Patch

From b374e78c163b37fbd19991e4fb776cbade6a2548 Mon Sep 17 00:00:00 2001
From: Nick Lowe <nick.lowe@lugatech.com>
Date: Fri, 19 Feb 2016 15:22:25 +0000
Subject: [PATCH] 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.

Signed-off-by: Nick Lowe <nick.lowe@lugatech.com>
---
 src/ap/accounting.c          | 75 ++++++++++++++++++++++++++------------------
 src/ap/ctrl_iface_ap.c       |  7 +++--
 src/ap/sta_info.h            | 10 +++---
 src/drivers/driver.h         |  1 +
 src/drivers/driver_hostap.c  | 14 +++++----
 src/drivers/driver_nl80211.c |  6 ++++
 6 files changed, 70 insertions(+), 43 deletions(-)

diff --git a/src/ap/accounting.c b/src/ap/accounting.c
index f3ce121..d807216 100644
--- a/src/ap/accounting.c
+++ b/src/ap/accounting.c
@@ -167,19 +167,30 @@  static int accounting_sta_update_stats(struct hostapd_data *hapd,
 	if (hostapd_drv_read_sta_data(hapd, data, sta->addr))
 		return -1;
 
-	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;
 
 	hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS,
 		       HOSTAPD_LEVEL_DEBUG, "updated TX/RX stats: "
-		       "Acct-Input-Octets=%lu Acct-Input-Gigawords=%u "
-		       "Acct-Output-Octets=%lu Acct-Output-Gigawords=%u",
-		       sta->last_rx_bytes, sta->acct_input_gigawords,
-		       sta->last_tx_bytes, sta->acct_output_gigawords);
+		       "last_rx_bytes_lo=%lu last_rx_bytes_hi=%lu "
+		       "last_tx_bytes_lo=%lu last_tx_bytes_hi=%lu "
+		       "last_rx_bytes64=%llu "
+		       "last_tx_bytes64=%llu",
+		       (unsigned long)sta->last_rx_bytes_lo,
+		       (unsigned long)(sta->last_rx_bytes_hi),
+		       (unsigned long)sta->last_tx_bytes_lo,
+		       (unsigned long)(sta->last_tx_bytes_hi),
+		       (unsigned long long)sta->last_rx_bytes64,
+		       (unsigned long long)sta->last_tx_bytes64
+		      );
 
 	return 0;
 }
@@ -224,8 +235,12 @@  void accounting_sta_start(struct hostapd_data *hapd, struct sta_info *sta)
 		       (long unsigned int) sta->acct_session_id);
 
 	os_get_reltime(&sta->acct_session_start);
-	sta->last_rx_bytes = sta->last_tx_bytes = 0;
-	sta->acct_input_gigawords = sta->acct_output_gigawords = 0;
+	sta->last_rx_bytes_hi = 0;
+	sta->last_rx_bytes_lo = 0;
+	sta->last_tx_bytes_hi = 0;
+	sta->last_tx_bytes_lo = 0;
+	sta->last_rx_bytes64 = 0;
+	sta->last_tx_bytes64 = 0;
 	hostapd_drv_sta_clear_stats(hapd, sta->addr);
 
 	if (!hapd->conf->radius->acct_server)
@@ -254,7 +269,7 @@  static void accounting_sta_report(struct hostapd_data *hapd,
 	int cause = sta->acct_terminate_cause;
 	struct hostap_sta_driver_data data;
 	struct os_reltime now_r, diff;
-	u32 gigawords;
+	unsigned long long bytes;
 
 	if (!hapd->conf->radius->acct_server)
 		return;
@@ -288,37 +303,35 @@  static void accounting_sta_report(struct hostapd_data *hapd,
 			wpa_printf(MSG_INFO, "Could not add Acct-Output-Packets");
 			goto fail;
 		}
+		if (data.rx_bytes64 > 0)
+			bytes = data.rx_bytes64;
+		else
+			bytes = ((unsigned long long)sta->last_rx_bytes_hi << 32) | data.rx_bytes;
 		if (!radius_msg_add_attr_int32(msg,
 					       RADIUS_ATTR_ACCT_INPUT_OCTETS,
-					       data.rx_bytes)) {
+					       (u32)bytes)) {
 			wpa_printf(MSG_INFO, "Could not add Acct-Input-Octets");
 			goto fail;
 		}
-		gigawords = sta->acct_input_gigawords;
-#if __WORDSIZE == 64
-		gigawords += data.rx_bytes >> 32;
-#endif
-		if (gigawords &&
-		    !radius_msg_add_attr_int32(
-			    msg, RADIUS_ATTR_ACCT_INPUT_GIGAWORDS,
-			    gigawords)) {
+		if (!radius_msg_add_attr_int32(msg,
+					       RADIUS_ATTR_ACCT_INPUT_GIGAWORDS,
+					       (u32)(bytes >> 32))) {
 			wpa_printf(MSG_INFO, "Could not add Acct-Input-Gigawords");
 			goto fail;
 		}
+		if (data.tx_bytes64 > 0)
+			bytes = data.tx_bytes64;
+		else
+			bytes = ((unsigned long long)sta->last_tx_bytes_hi << 32) | data.tx_bytes;
 		if (!radius_msg_add_attr_int32(msg,
 					       RADIUS_ATTR_ACCT_OUTPUT_OCTETS,
-					       data.tx_bytes)) {
+					       (u32)bytes)) {
 			wpa_printf(MSG_INFO, "Could not add Acct-Output-Octets");
 			goto fail;
 		}
-		gigawords = sta->acct_output_gigawords;
-#if __WORDSIZE == 64
-		gigawords += data.tx_bytes >> 32;
-#endif
-		if (gigawords &&
-		    !radius_msg_add_attr_int32(
-			    msg, RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS,
-			    gigawords)) {
+		if (!radius_msg_add_attr_int32(msg,
+					       RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS,
+					       (u32)(bytes >> 32))) {
 			wpa_printf(MSG_INFO, "Could not add Acct-Output-Gigawords");
 			goto fail;
 		}
diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c
index c98978f..bce48a7 100644
--- 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,
 		return 0;
 
 	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
+			);
 	if (os_snprintf_error(buflen, ret))
 		return 0;
 	return ret;
diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
index 359f480..d818afb 100644
--- a/src/ap/sta_info.h
+++ b/src/ap/sta_info.h
@@ -107,10 +107,12 @@  struct sta_info {
 	int acct_terminate_cause; /* Acct-Terminate-Cause */
 	int acct_interim_interval; /* Acct-Interim-Interval */
 
-	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;
 
 	u8 *challenge; /* IEEE 802.11 Shared Key Authentication Challenge */
 
diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index a4f6704..c2a81fe 100644
--- 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;
 };
 
 struct hostapd_sta_add_params {
diff --git a/src/drivers/driver_hostap.c b/src/drivers/driver_hostap.c
index 517a3bb..e1b7dc9 100644
--- a/src/drivers/driver_hostap.c
+++ b/src/drivers/driver_hostap.c
@@ -579,7 +579,6 @@  static int hostap_read_sta_data(void *priv,
 	struct hostap_driver_data *drv = priv;
 	char buf[1024], line[128], *pos;
 	FILE *f;
-	unsigned long val;
 
 	memset(data, 0, sizeof(*data));
 	snprintf(buf, sizeof(buf), "/proc/net/hostap/%s/" MACSTR,
@@ -597,15 +596,18 @@  static int hostap_read_sta_data(void *priv,
 		if (!pos)
 			continue;
 		*pos++ = '\0';
-		val = strtoul(pos, NULL, 10);
 		if (strcmp(line, "rx_packets") == 0)
-			data->rx_packets = val;
+			data->rx_packets = strtoul(pos, NULL, 10);
 		else if (strcmp(line, "tx_packets") == 0)
-			data->tx_packets = val;
+			data->tx_packets = strtoul(pos, NULL, 10);
 		else if (strcmp(line, "rx_bytes") == 0)
-			data->rx_bytes = val;
+			data->rx_bytes = strtoul(pos, NULL, 10);
 		else if (strcmp(line, "tx_bytes") == 0)
-			data->tx_bytes = val;
+			data->tx_bytes = strtoul(pos, NULL, 10);
+		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);
 	}
 
 	fclose(f);
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 0f312a0..9916bc6 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -5327,6 +5327,8 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 		[NL80211_STA_INFO_RX_PACKETS] = { .type = NLA_U32 },
 		[NL80211_STA_INFO_TX_PACKETS] = { .type = NLA_U32 },
 		[NL80211_STA_INFO_TX_FAILED] = { .type = NLA_U32 },
+		[NL80211_STA_INFO_RX_BYTES64] = { .type = NLA_U64 },
+		[NL80211_STA_INFO_TX_BYTES64] = { .type = NLA_U64 },
 	};
 
 	nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
@@ -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]);
 
 	return NL_SKIP;
 }
-- 
2.7.1