On Mon, Feb 22, 2016 at 09:12:13PM +0000, Nick Lowe wrote:
> Agreed. I am now of the opinion that the best approach is likely to be
> as you say, all things considered. If the need to retry occurs due to
> no ACK being received, do so with fresh dynamic data.
The following changes should do this (not yet in the master branch, but
at least initial testing seems to indicate that this works):
[PATCH] RADIUS: Update full message for interim accounting updates
Instead of using the RADIUS client retransmission design with the old
RADIUS message contents for each retry, trigger a completely new interim
accounting update instance more quickly (using the same schedule as
RADIUS message retransmissions) to improve accounting updates in cases
where RADIUS message delivery fails. This allows the server to get up to
date information from the time the "retry" message was sent instead of
the old information from the time the first failed attempt was sent.
Signed-off-by: Jouni Malinen <j@xxxxx>
---
src/ap/accounting.c | 59 +++++++++++++++++++++++++++++
src/ap/sta_info.h | 1 +
src/radius/radius_client.c | 92 ++++++++++++++++++++++++----------------------
src/radius/radius_client.h | 3 ++
4 files changed, 112 insertions(+), 43 deletions(-)
diff --git a/src/ap/accounting.c b/src/ap/accounting.c
index 9357a46..010ba05 100644
--- a/src/ap/accounting.c
+++ b/src/ap/accounting.c
@@ -450,6 +450,63 @@ static void accounting_report_state(struct hostapd_data *hapd, int on)
}
+static void accounting_interim_error_cb(const u8 *addr, void *ctx)
+{
+ struct hostapd_data *hapd = ctx;
+ struct sta_info *sta;
+ unsigned int i, wait_time;
+ int res;
+
+ sta = ap_get_sta(hapd, addr);
+ if (!sta)
+ return;
+ sta->acct_interim_errors++;
+ if (sta->acct_interim_errors > 10 /* RADIUS_CLIENT_MAX_RETRIES */) {
+ wpa_printf(MSG_DEBUG,
+ "Interim RADIUS accounting update failed for " MACSTR
+ " - too many errors, abandon this interim accounting update",
+ MAC2STR(addr));
+ sta->acct_interim_errors = 0;
+ /* Next update will be tried after normal update interval */
+ return;
+ }
+
+ /*
+ * Use a shorter update interval as an improved retransmission mechanism
+ * for failed interim accounting updates. This allows the statistics to
+ * be updated for each retransmission.
+ *
+ * RADIUS client code has already waited RADIUS_CLIENT_FIRST_WAIT.
+ * Schedule the first retry attempt immediately and every following one
+ * with exponential backoff.
+ */
+ if (sta->acct_interim_errors == 1) {
+ wait_time = 0;
+ } else {
+ wait_time = 3; /* RADIUS_CLIENT_FIRST_WAIT */
+ for (i = 1; i < sta->acct_interim_errors; i++)
+ wait_time *= 2;
+ }
+ res = eloop_deplete_timeout(wait_time, 0, accounting_interim_update,
+ hapd, sta);
+ if (res == 1)
+ wpa_printf(MSG_DEBUG,
+ "Interim RADIUS accounting update failed for " MACSTR
+ " (error count: %u) - schedule next update in %u seconds",
+ MAC2STR(addr), sta->acct_interim_errors, wait_time);
+ else if (res == 0)
+ wpa_printf(MSG_DEBUG,
+ "Interim RADIUS accounting update failed for " MACSTR
+ " (error count: %u)", MAC2STR(addr),
+ sta->acct_interim_errors);
+ else
+ wpa_printf(MSG_DEBUG,
+ "Interim RADIUS accounting update failed for " MACSTR
+ " (error count: %u) - no timer found", MAC2STR(addr),
+ sta->acct_interim_errors);
+}
+
+
/**
* accounting_init: Initialize accounting
* @hapd: hostapd BSS data
@@ -464,6 +521,8 @@ int accounting_init(struct hostapd_data *hapd)
if (radius_client_register(hapd->radius, RADIUS_ACCT,
accounting_receive, hapd))
return -1;
+ radius_client_set_interim_error_cb(hapd->radius,
+ accounting_interim_error_cb, hapd);
accounting_report_state(hapd, 1);
diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
index e223341..cd89e99 100644
--- a/src/ap/sta_info.h
+++ b/src/ap/sta_info.h
@@ -118,6 +118,7 @@ struct sta_info {
int acct_session_started;
int acct_terminate_cause; /* Acct-Terminate-Cause */
int acct_interim_interval; /* Acct-Interim-Interval */
+ unsigned int acct_interim_errors;
/* For extending 32-bit driver counters to 64-bit counters */
u32 last_rx_bytes_hi;
diff --git a/src/radius/radius_client.c b/src/radius/radius_client.c
index b24bbf8..5e705e6 100644
--- a/src/radius/radius_client.c
+++ b/src/radius/radius_client.c
@@ -226,6 +226,16 @@ struct radius_client_data {
* next_radius_identifier - Next RADIUS message identifier to use
*/
u8 next_radius_identifier;
+
+ /**
+ * interim_error_cb - Interim accounting error callback
+ */
+ void (*interim_error_cb)(const u8 *addr, void *ctx);
+
+ /**
+ * interim_error_cb_ctx - interim_error_cb() context data
+ */
+ void *interim_error_cb_ctx;
};
@@ -297,6 +307,25 @@ int radius_client_register(struct radius_client_data *radius,
}
+/**
+ * radius_client_set_interim_erro_cb - Register an interim acct error callback
+ * @radius: RADIUS client context from radius_client_init()
+ * @addr: Station address from the failed message
+ * @cb: Handler for interim accounting errors
+ * @ctx: Context pointer for handler callbacks
+ *
+ * This function is used to register a handler for processing failed
+ * transmission attempts of interim accounting update messages.
+ */
+void radius_client_set_interim_error_cb(struct radius_client_data *radius,
+ void (*cb)(const u8 *addr, void *ctx),
+ void *ctx)
+{
+ radius->interim_error_cb = cb;
+ radius->interim_error_cb_ctx = ctx;
+}
+
+
/*
* Returns >0 if message queue was flushed (i.e., the message that triggered
* the error is not available anymore)
@@ -371,6 +400,18 @@ static int radius_client_retransmit(struct radius_client_data *radius,
conf->auth_server->retransmissions++;
}
}
+
+ if (entry->msg_type == RADIUS_ACCT_INTERIM) {
+ wpa_printf(MSG_DEBUG,
+ "RADIUS: Failed to transmit interim accounting update to "
+ MACSTR " - drop message and request a new update",
+ MAC2STR(entry->addr));
+ if (radius->interim_error_cb)
+ radius->interim_error_cb(entry->addr,
+ radius->interim_error_cb_ctx);
+ return 1;
+ }
+
if (s < 0) {
wpa_printf(MSG_INFO,
"RADIUS: No valid socket for retransmission");
@@ -624,39 +665,6 @@ static void radius_client_list_add(struct radius_client_data *radius,
}
-static void radius_client_list_del(struct radius_client_data *radius,
- RadiusType msg_type, const u8 *addr)
-{
- struct radius_msg_list *entry, *prev, *tmp;
-
- if (addr == NULL)
- return;
-
- entry = radius->msgs;
- prev = NULL;
- while (entry) {
- if (entry->msg_type == msg_type &&
- os_memcmp(entry->addr, addr, ETH_ALEN) == 0) {
- if (prev)
- prev->next = entry->next;
- else
- radius->msgs = entry->next;
- tmp = entry;
- entry = entry->next;
- hostapd_logger(radius->ctx, addr,
- HOSTAPD_MODULE_RADIUS,
- HOSTAPD_LEVEL_DEBUG,
- "Removing matching RADIUS message");
- radius_client_msg_free(tmp);
- radius->num_msgs--;
- continue;
- }
- prev = entry;
- entry = entry->next;
- }
-}
-
-
/**
* radius_client_send - Send a RADIUS request
* @radius: RADIUS client context from radius_client_init()
@@ -668,16 +676,19 @@ static void radius_client_list_del(struct radius_client_data *radius,
* This function is used to transmit a RADIUS authentication (RADIUS_AUTH) or
* accounting request (RADIUS_ACCT or RADIUS_ACCT_INTERIM). The only difference
* between accounting and interim accounting messages is that the interim
- * message will override any pending interim accounting updates while a new
- * accounting message does not remove any pending messages.
+ * message will not be retransmitted. Instead, a callback is used to indicate
+ * that the transmission failed for the specific station @addr so that a new
+ * interim accounting update message can be generated with up-to-date session
+ * data instead of trying to resend old information.
*
* The message is added on the retransmission queue and will be retransmitted
* automatically until a response is received or maximum number of retries
- * (RADIUS_CLIENT_MAX_RETRIES) is reached.
+ * (RADIUS_CLIENT_MAX_RETRIES) is reached. No such retries are used with
+ * RADIUS_ACCT_INTERIM, i.e., such a pending message is removed from the queue
+ * automatically on transmission failure.
*
* The related device MAC address can be used to identify pending messages that
- * can be removed with radius_client_flush_auth() or with interim accounting
- * updates.
+ * can be removed with radius_client_flush_auth().
*/
int radius_client_send(struct radius_client_data *radius,
struct radius_msg *msg, RadiusType msg_type,
@@ -690,11 +701,6 @@ int radius_client_send(struct radius_client_data *radius,
int s, res;
struct wpabuf *buf;
- if (msg_type == RADIUS_ACCT_INTERIM) {
- /* Remove any pending interim acct update for the same STA. */
- radius_client_list_del(radius, msg_type, addr);
- }
-
if (msg_type == RADIUS_ACCT || msg_type == RADIUS_ACCT_INTERIM) {
if (conf->acct_server && radius->acct_sock < 0)
radius_client_init_acct(radius);
diff --git a/src/radius/radius_client.h b/src/radius/radius_client.h
index 3db16aa..8ca0874 100644
--- a/src/radius/radius_client.h
+++ b/src/radius/radius_client.h
@@ -241,6 +241,9 @@ int radius_client_register(struct radius_client_data *radius,
const u8 *shared_secret, size_t shared_secret_len,
void *data),
void *data);
+void radius_client_set_interim_error_cb(struct radius_client_data *radius,
+ void (*cb)(const u8 *addr, void *ctx),
+ void *ctx);
int radius_client_send(struct radius_client_data *radius,
struct radius_msg *msg,
RadiusType msg_type, const u8 *addr);
--
1.9.1
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
