Skip to content

icmp rate limiter #607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FRIDM636 wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −0
Open

icmp rate limiter #607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions modules/infra/cli/icmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "cli_l3.h"

#include <gr_api.h>
#include <gr_ip4.h>
#include <gr_net_types.h>

#include <ecoli.h>
Expand Down Expand Up @@ -39,6 +40,18 @@ static cmd_status_t ping(struct gr_api_client *c, const struct ec_pnode *p) {
return CMD_ERROR;
}

static cmd_status_t icmp_rate_limit(struct gr_api_client *c, const struct ec_pnode *p) {
struct gr_ip4_icmp_rl_req req;

if (arg_u32(p, "INTERVAL", &req.rate_limit))
return CMD_ERROR;

if (gr_api_client_send_recv(c, GR_IP4_ICMP_RATE_LIMIT, sizeof(req), &req, NULL) < 0)
return CMD_ERROR;

return CMD_SUCCESS;
}

static cmd_status_t traceroute(struct gr_api_client *c, const struct ec_pnode *p) {
struct cli_icmp_ops *ops;
uint8_t ip[16];
Expand Down Expand Up @@ -94,6 +107,19 @@ static int ctx_init(struct ec_node *root) {
ec_node_uint("IDENT", 1, UINT16_MAX, 10)
)
);
if (ret < 0)
return ret;

ret = CLI_COMMAND(
CLI_CONTEXT(root, CTX_ARG("icmp", "Icmp rate limit config")),
"rate-limit (INTERVAL)",
icmp_rate_limit,
"Set the icmp rate limiter",
with_help(
"The time space interval in milliseconds",
ec_node_uint("INTERVAL", 1, UINT32_MAX, 10)
)
);

return ret;
}
Expand Down
7 changes: 7 additions & 0 deletions modules/ip/api/gr_ip4.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ enum gr_ip4_requests : uint32_t {
GR_IP4_ICMP_RECV,
GR_IP4_FIB_DEFAULT_SET,
GR_IP4_FIB_INFO_LIST,
GR_IP4_ICMP_RATE_LIMIT,
};

// routes //////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -148,6 +149,12 @@ struct gr_ip4_icmp_recv_resp {

GR_REQ(GR_IP4_ICMP_RECV, struct gr_ip4_icmp_recv_req, struct gr_ip4_icmp_recv_resp);

struct gr_ip4_icmp_rl_req {
uint32_t rate_limit;
};

GR_REQ(GR_IP4_ICMP_RATE_LIMIT, struct gr_ip4_icmp_rl_req, struct gr_empty);

// fib info ////////////////////////////////////////////////////////////////////

// FIB status for a VRF.
Expand Down
10 changes: 10 additions & 0 deletions modules/ip/control/icmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: BSD-3-Clause
// Copyright (c) 2024 Christophe Fontaine

#include "icmp_rl.h"
#include "ip4.h"
#include "ip4_datapath.h"
#include "log.h"
Expand Down Expand Up @@ -87,6 +88,13 @@ static struct rte_mbuf *get_icmp_response(uint16_t ident, uint16_t seq_num, cloc
return errno_set_null(ENOENT);
}

static struct api_out icmp_rate_limit(const void *request, struct api_ctx *) {
const struct gr_ip4_icmp_rl_req *rl = request;
icmp_rl_init(rl->rate_limit);

return api_out(0, 0, NULL);
}

static struct api_out icmp_send(const void *request, struct api_ctx *) {
const struct gr_ip4_icmp_send_req *req = request;
const struct nexthop *nh;
Expand Down Expand Up @@ -167,6 +175,7 @@ static void icmp_init(struct event_base *) {
SOCKET_ID_ANY,
0 // flags
);

if (pool == NULL)
ABORT("rte_mempool_create(icmp_queue) failed");
}
Expand All @@ -191,6 +200,7 @@ RTE_INIT(icmp_module_init) {
module_register(&icmp_module);
api_handler(GR_IP4_ICMP_SEND, icmp_send);
api_handler(GR_IP4_ICMP_RECV, icmp_recv);
api_handler(GR_IP4_ICMP_RATE_LIMIT, icmp_rate_limit);
icmp_input_register_callback(RTE_ICMP_TYPE_DEST_UNREACHABLE, icmp_input_cb);
icmp_input_register_callback(RTE_ICMP_TYPE_TTL_EXCEEDED, icmp_input_cb);
icmp_input_register_callback(RTE_ICMP_TYPE_ECHO_REPLY, icmp_input_cb);
Expand Down
26 changes: 26 additions & 0 deletions modules/ip/control/icmp_rl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// SPDX-License-Identifier: BSD-3-Clause
// Copyright (c) 2026 Farid Mihoub

#include "icmp_rl.h"

#include <gr_clock.h>

#include <stdint.h>
#include <time.h>

static struct time_space icmp_rl = {0};

void icmp_rl_init(uint32_t interval_ms) {
icmp_rl.interval_ms = interval_ms;
icmp_rl.last_ts = gr_clock_us();
}

bool icmp_rl_allow() {
uint64_t now = gr_clock_us();

if ((now - icmp_rl.last_ts) < (icmp_rl.interval_ms * 1000))
return false;

icmp_rl.last_ts = now;
return true;
Comment on lines +11 to +25
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Serialize the shared limiter state.

icmp_rl_allow() and icmp_rl_init() both read/write the same global icmp_rl object, but this code is exercised from the ICMP input node, which can run on multiple workers. As written, concurrent packets can race through the check/update and both be accepted, so the rate limit is not enforced reliably.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@modules/ip/control/icmp_rl.c` around lines 11 - 25, The global rate-limiter
state (icmp_rl of type time_space) is accessed concurrently by icmp_rl_init()
and icmp_rl_allow(), causing races; serialize access by protecting reads/writes
to icmp_rl.last_ts and icmp_rl.interval_ms with a concurrency primitive (e.g., a
mutex/spinlock or atomic CAS) so only one worker can update/check the timestamp
at a time. Modify icmp_rl_init() to acquire the lock before setting
icmp_rl.interval_ms and icmp_rl.last_ts and release it afterwards, and modify
icmp_rl_allow() to atomically read/compare/update icmp_rl.last_ts (or acquire
the same lock around the now - last_ts check and the assignment) to ensure the
interval logic is enforced across workers; keep identifiers icmp_rl,
icmp_rl_init, icmp_rl_allow, last_ts, interval_ms and time_space for locating
the changes.

Comment on lines +13 to +25
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't arm the limiter with the current timestamp.

icmp_rl_init() currently makes the first Echo Reply after configuration wait a full interval, and the interval_ms * 1000 conversion is still done in 32-bit arithmetic. Please backdate last_ts by the configured interval and widen the microsecond conversion to uint64_t.

Possible fix 
 void icmp_rl_init(uint32_t interval_ms) {
+	uint64_t interval_us = (uint64_t)interval_ms * 1000ULL;
 	icmp_rl.interval_ms = interval_ms;
-	icmp_rl.last_ts = gr_clock_us();
+	icmp_rl.last_ts = gr_clock_us() - interval_us;
 }
 
 bool icmp_rl_allow() {
 	uint64_t now = gr_clock_us();
+	uint64_t interval_us = (uint64_t)icmp_rl.interval_ms * 1000ULL;
 
-	if ((now - icmp_rl.last_ts) < (icmp_rl.interval_ms * 1000))
+	if ((now - icmp_rl.last_ts) < interval_us)
 		return false;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@modules/ip/control/icmp_rl.c` around lines 13 - 25, icmp_rl_init currently
sets icmp_rl.last_ts to now which forces the first icmp_rl_allow() call to wait
a full interval; instead subtract the configured interval (in microseconds) from
the current timestamp so the first reply is allowed immediately, and when
comparing/multiplying use 64-bit arithmetic: convert icmp_rl.interval_ms to
microseconds with a uint64_t cast (e.g. (uint64_t)icmp_rl.interval_ms * 1000)
and use that value both when backdating icmp_rl.last_ts in icmp_rl_init and when
comparing in icmp_rl_allow.

}
16 changes: 16 additions & 0 deletions modules/ip/control/icmp_rl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: BSD-3-Clause
// Copyright (c) 2026 Farid Mihoub

#pragma once

#include <stdbool.h>
#include <stdint.h>
#include <time.h>

struct time_space {
uint64_t last_ts;
uint32_t interval_ms;
};

void icmp_rl_init(uint32_t interval_ms);
bool icmp_rl_allow();
1 change: 1 addition & 0 deletions modules/ip/control/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ src += files(
'icmp.c',
'nexthop.c',
'route.c',
'icmp_rl.c',
)
inc += include_directories('.')
8 changes: 8 additions & 0 deletions modules/ip/datapath/icmp_input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "control_output.h"
#include "graph.h"
#include "icmp_rl.h"
#include "ip4_datapath.h"
#include "log.h"
#include "mbuf.h"
Expand All @@ -17,6 +18,7 @@ enum {
CONTROL,
INVALID,
UNSUPPORTED,
RATE_LIMITED,
EDGE_COUNT,
};

Expand Down Expand Up @@ -49,6 +51,10 @@ icmp_input_process(struct rte_graph *graph, struct rte_node *node, void **objs,
edge = INVALID;
goto next;
}
if (!icmp_rl_allow()) {
edge = RATE_LIMITED;
goto next;
}
icmp->icmp_type = RTE_ICMP_TYPE_ECHO_REPLY;
ip = ip_data->dst;
ip_data->dst = ip_data->src;
Expand Down Expand Up @@ -95,6 +101,7 @@ static struct rte_node_register icmp_input_node = {
[CONTROL] = "control_output",
[INVALID] = "icmp_input_invalid",
[UNSUPPORTED] = "icmp_input_unsupported",
[RATE_LIMITED] = "icmp_rate_limited",
},
};

Expand All @@ -109,3 +116,4 @@ GR_NODE_REGISTER(icmp_input_info);

GR_DROP_REGISTER(icmp_input_invalid);
GR_DROP_REGISTER(icmp_input_unsupported);
GR_DROP_REGISTER(icmp_rate_limited);
Loading