From d77f4d449827cd248428ddc68bb535e28041c4c5 Mon Sep 17 00:00:00 2001 From: Thomas Kolb Date: Sun, 15 Dec 2024 22:24:21 +0100 Subject: [PATCH] Multiple fixes in packet handling - add and handle layer 2 packet type correctly in data packets - don't produce garbage packets if a packet could not be decoded or was not a data packet - handle beacon/connection request/connection parameters handshake - digipeater cycle timeout does not reset beacon timer anymore. This prevented any beacon transmission. - Reset the connection timeout when empty packets are received --- impl/src/layer2/connection.c | 80 ++++++++++++++------ impl/src/layer2/connection.h | 7 +- impl/src/layer2/connection_list.c | 6 +- impl/src/layer2/digipeater.c | 15 +++- impl/src/results.h | 8 ++ impl/test/layer2_over_udp/l2udptest_client.c | 24 +++++- 6 files changed, 107 insertions(+), 33 deletions(-) diff --git a/impl/src/layer2/connection.c b/impl/src/layer2/connection.c index e2c6f89..7915e31 100644 --- a/impl/src/layer2/connection.c +++ b/impl/src/layer2/connection.c @@ -100,6 +100,9 @@ result_t connection_handle_packet(connection_ctx_t *ctx, const uint8_t *buf, siz break; } + data_packet->payload_type = L2_PAYLOAD_TYPE_INVALID; + data_packet->payload_len = 0; + // check the CRC size_t packet_size = buf_len - crc_sizeof_key(PAYLOAD_CRC_SCHEME); @@ -122,7 +125,8 @@ result_t connection_handle_packet(connection_ctx_t *ctx, const uint8_t *buf, siz return OK; } - if(!ham64_is_equal(&header.dst_addr, &ctx->my_addr)) { + if(ham64_get_addr_type(&header.dst_addr) != HAM64_ADDR_TYPE_BROADCAST + && !ham64_is_equal(&header.dst_addr, &ctx->my_addr)) { char fmt_dst_addr[HAM64_FMT_MAX_LEN]; char fmt_my_addr[HAM64_FMT_MAX_LEN]; @@ -157,29 +161,41 @@ static result_t handle_conn_mgmt( uint8_t packet_type = payload[0]; - if(packet_type == CONN_MGMT_TYPE_BEACON) { - if(ctx->conn_state == CONN_STATE_CONNECTING) { - LOG(LVL_INFO, "Received beacon; queueing connection request."); + switch(packet_type) { + case CONN_MGMT_TYPE_BEACON: + if(ctx->conn_state == CONN_STATE_CONNECTING) { + LOG(LVL_INFO, "Received beacon; queueing connection request."); - // enqueue a connection request packet - layer2_packet_header_t conn_request_header; + // enqueue a connection request packet + layer2_packet_header_t conn_request_header; - conn_request_header.tx_request = true; - conn_request_header.dst_addr = ctx->peer_addr; - conn_request_header.src_addr = ctx->my_addr; - conn_request_header.msg_type = L2_MSG_TYPE_CONN_MGMT; - conn_request_header.rx_seq_nr = 0; - conn_request_header.tx_seq_nr = 0; + conn_request_header.tx_request = true; + conn_request_header.dst_addr = ctx->peer_addr; + conn_request_header.src_addr = ctx->my_addr; + conn_request_header.msg_type = L2_MSG_TYPE_CONN_MGMT; + conn_request_header.rx_seq_nr = 0; + conn_request_header.tx_seq_nr = 0; - // create a persistent copy of the packet data. - uint8_t packetbuf[1]; - packetbuf[0] = CONN_MGMT_TYPE_CONNECTION_REQUEST; + // create a persistent copy of the packet data. + uint8_t packetbuf[1]; + packetbuf[0] = CONN_MGMT_TYPE_CONNECTION_REQUEST; - connection_enqueue_packet(ctx, &conn_request_header, packetbuf, 1); - } else { - LOG(LVL_WARN, "Beacons are ignored in states other than CONNECTING."); - return ERR_INVALID_STATE; - } + connection_enqueue_packet(ctx, &conn_request_header, packetbuf, 1); + } else { + LOG(LVL_WARN, "Beacons are ignored in states other than CONNECTING."); + return ERR_INVALID_STATE; + } + break; + + case CONN_MGMT_TYPE_CONNECTION_PARAMETERS: + LOG(LVL_INFO, "Connection parameters received! -> connection established"); + ctx->next_expected_seq = 1; // connection parameters are packet 0 + ctx->conn_state = CONN_STATE_ESTABLISHED; + break; + + default: + LOG(LVL_WARN, "Ignored connection management type %d", packet_type); + break; } return OK; @@ -231,6 +247,10 @@ result_t connection_handle_packet_prechecked( switch(header->msg_type) { case L2_MSG_TYPE_EMPTY: LOG(LVL_DEBUG, "Empty packet: accepted ACK for %u.", ctx->last_acked_seq); + + // empty packets also reset the timeout timer + ctx->last_rx_time = get_hires_time(); + // handle the acknowledgement internally connection_handle_ack(ctx, header->rx_seq_nr, false); return OK; // do not ACK @@ -318,7 +338,11 @@ result_t connection_enqueue_packet( } -result_t connection_enqueue_data_packet(connection_ctx_t *ctx, uint8_t *buf, size_t buf_len) +result_t connection_enqueue_data_packet( + connection_ctx_t *ctx, + layer2_payload_type_t payload_type, + uint8_t *buf, + size_t buf_len) { // check the connection state switch(ctx->conn_state) { @@ -343,7 +367,12 @@ result_t connection_enqueue_data_packet(connection_ctx_t *ctx, uint8_t *buf, siz header.tx_request = 0; header.tx_seq_nr = ctx->next_seq_nr; - ERR_CHECK(connection_enqueue_packet(ctx, &header, buf, buf_len)); + uint8_t packet_with_type[buf_len + 1]; + + packet_with_type[0] = (uint8_t)payload_type; + memcpy(packet_with_type+1, buf, buf_len); + + ERR_CHECK(connection_enqueue_packet(ctx, &header, packet_with_type, sizeof(packet_with_type))); ctx->next_seq_nr++; ctx->next_seq_nr &= SEQ_NR_MASK; @@ -531,8 +560,11 @@ void connection_handle_ack(connection_ctx_t *ctx, uint8_t acked_seq, bool do_ack packet_queue_delete(&ctx->packet_queue, packets_to_remove); // send the next requested packet (all previous ones were deleted above). - assert(ctx->next_packet_index >= packets_to_remove); - ctx->next_packet_index -= packets_to_remove; + if(ctx->next_packet_index >= packets_to_remove) { + ctx->next_packet_index -= packets_to_remove; + } else { + ctx->next_packet_index = 0; + } packets_available = packet_queue_get_used_space(&ctx->packet_queue); diff --git a/impl/src/layer2/connection.h b/impl/src/layer2/connection.h index 017fe04..ea9ac8e 100644 --- a/impl/src/layer2/connection.h +++ b/impl/src/layer2/connection.h @@ -138,10 +138,15 @@ result_t connection_enqueue_packet( /*!\brief Enqueue a data packet for transmission. * \param ctx The connection context. + * \param payload_type Type of the payload. * \param buf Pointer to the data buffer. * \param buf_len Length of the data. */ -result_t connection_enqueue_data_packet(connection_ctx_t *ctx, uint8_t *buf, size_t buf_len); +result_t connection_enqueue_data_packet( + connection_ctx_t *ctx, + layer2_payload_type_t payload_type, + uint8_t *buf, + size_t buf_len); /*!\brief Check if there is free space in the TX packet queue. * \param ctx The connection context. diff --git a/impl/src/layer2/connection_list.c b/impl/src/layer2/connection_list.c index 8ead247..95c7c27 100644 --- a/impl/src/layer2/connection_list.c +++ b/impl/src/layer2/connection_list.c @@ -1,10 +1,14 @@ #include #include +#define LOGGER_MODULE_NAME "clist" +#include "logger.h" + #include "connection_list.h" #include "layer2/connection.h" #include "results.h" +#include "utils.h" #define SEQ_NR_MASK 0xF @@ -213,7 +217,7 @@ result_t connection_list_enqueue_packet(connection_list_t *list, uint8_t *data, return ERR_INVALID_ADDRESS; } - return connection_enqueue_data_packet(&ptr->connection, data, data_len); + return connection_enqueue_data_packet(&ptr->connection, L2_PAYLOAD_TYPE_IPV6, data, data_len); } diff --git a/impl/src/layer2/digipeater.c b/impl/src/layer2/digipeater.c index 1da927d..38aa352 100644 --- a/impl/src/layer2/digipeater.c +++ b/impl/src/layer2/digipeater.c @@ -186,7 +186,7 @@ result_t digipeater_fill_packet_queues_from_tundev(digipeater_ctx_t *ctx, int tu // first check if any queue is already full, so we don't have to drop packets // from the TUN device queue. if(!connection_list_can_enqueue_packet(&ctx->conn_list)) { - LOG(LVL_DEBUG, "No free space in queues."); + LOG(LVL_DEBUG, "No connection or no free space in queues."); return OK; // do nothing } @@ -215,6 +215,9 @@ result_t digipeater_fill_packet_queues_from_tundev(digipeater_ctx_t *ctx, int tu } else if(ret == 0) { // no more data, should not happen break; + } else if(ret < 4) { + LOG(LVL_ERR, "Not enough data from TUN read() to check packet type!"); + return ERR_SYSCALL; } uint16_t flags = *(uint16_t*)packetbuf; @@ -222,9 +225,13 @@ result_t digipeater_fill_packet_queues_from_tundev(digipeater_ctx_t *ctx, int tu LOG(LVL_DUMP, "TUN Flags: 0x%04x", flags); LOG(LVL_DUMP, "TUN Proto: 0x%04x", proto); + uint8_t *packet_data = packetbuf + 4; + size_t packet_length = ret - 4; + + // note: octets are swapped in case statements switch(proto) { - case 0x86dd: // IPv6 - ERR_CHECK(connection_list_enqueue_packet(&ctx->conn_list, packetbuf, ret)); + case 0xdd86: // IPv6 + WARN_ON_ERR(connection_list_enqueue_packet(&ctx->conn_list, packet_data, packet_length)); break; default: @@ -258,6 +265,7 @@ size_t digipeater_encode_next_packet(digipeater_ctx_t *ctx, uint8_t *buf, size_t switch(ctx->state) { case DIGIPEATER_STATE_BEACON: packet_size = encode_beacon_packet(ctx, buf, buf_len); + ctx->next_beacon_time += HRTIME_MS(BEACON_INTERVAL_MS); *end_burst = true; break; @@ -318,7 +326,6 @@ result_t digipeater_end_cycle(digipeater_ctx_t *ctx) uint64_t now = get_hires_time(); if(now >= ctx->next_beacon_time) { - ctx->next_beacon_time += HRTIME_MS(BEACON_INTERVAL_MS); ctx->state = DIGIPEATER_STATE_BEACON; } else { // TODO: adjust the time based on connection activity; right now this results diff --git a/impl/src/results.h b/impl/src/results.h index cb847f4..faf5e4a 100644 --- a/impl/src/results.h +++ b/impl/src/results.h @@ -49,4 +49,12 @@ typedef enum { } \ } while(0); +#define WARN_ON_ERR(call) \ + do { \ + result_t err_check_result = call; \ + if(err_check_result != OK) { \ + LOG(LVL_WARN, "Error ignored at %s:%d: %d", __FILE__, __LINE__, err_check_result); \ + } \ + } while(0); + #endif // RESULTS_H diff --git a/impl/test/layer2_over_udp/l2udptest_client.c b/impl/test/layer2_over_udp/l2udptest_client.c index 69a6cb4..5937004 100644 --- a/impl/test/layer2_over_udp/l2udptest_client.c +++ b/impl/test/layer2_over_udp/l2udptest_client.c @@ -21,6 +21,7 @@ #include #include "layer2/ham64.h" +#include "results.h" #include "utils.h" #define LOGGER_MODULE_NAME "main" @@ -314,12 +315,25 @@ int main(void) } else if(ret == 0) { // no more data, should not happen break; + } else if(ret < 4) { + LOG(LVL_ERR, "Not enough data from TUN read() to check packet type!"); + return ERR_SYSCALL; } - LOG(LVL_DUMP, "TUN Flags: 0x%04x", *(uint16_t*)packetbuf); - LOG(LVL_DUMP, "TUN Proto: 0x%04x", *((uint16_t*)packetbuf + 1)); + uint16_t flags = *(uint16_t*)packetbuf; + uint16_t proto = *((uint16_t*)packetbuf + 1); + LOG(LVL_DUMP, "TUN Flags: 0x%04x", flags); + LOG(LVL_DUMP, "TUN Proto: 0x%04x", proto); - RESULT_CHECK(connection_enqueue_data_packet(&l2conn, packetbuf, ret)); + uint8_t *packet_data = packetbuf + 4; + size_t packet_length = ret - 4; + + if(proto != 0xdd86) { + LOG(LVL_WARN, "Non-IPv6 packet ignored. Proto: 0x%04x", proto); + continue; + } + + RESULT_CHECK(connection_enqueue_data_packet(&l2conn, L2_PAYLOAD_TYPE_IPV6, packet_data, packet_length)); } } @@ -411,6 +425,10 @@ int main(void) LOG(LVL_ERR, "Packet not in the expected sequence."); break; + case ERR_INVALID_STATE: + LOG(LVL_WARN, "Packet ignored due to invalid state."); + break; + default: // all other errors LOG(LVL_ERR, "layer2_rx_handle_packet() returned error code %u.", result); break;