net: drop_monitor: fix concurrency issues, preemption warning, and buffer overrun

If register_trace_napi_poll() fails in net_dm_trace_on_set(), unregister_trace_kfree_skb() is called to roll back the kfree_skb tracepoint registration.

However, tracepoint_synchronize_unregister() is omitted before calling cancel_work_sync() and module_put(). An in-flight probe executing concurrently on another CPU could call schedule_work() after cancel_work_sync() has already returned, leaving a pending work item scheduled after the module reference is dropped. If the module is then unloaded, executing the work item triggers a kernel panic.

Add tracepoint_synchronize_unregister() after unregister_trace_kfree_skb() in the error path, matching net_dm_trace_off_set() and net_dm_hw_probe_unregister().

Fixes 7c747838a558 ("drop_monitor: Split tracing enable / disable to different functions")

Signed-off-by Eric Dumazet <[email protected]>

---
 net/core/drop_monitor.c | 1 +
 1 file changed, 1 insertion(+)

net/core/drop_monitor.c +1 -0

index abaf108ac4db8c762c23f1d9c708c687ee36c7c9..018d19e3a71de0be50bcc5753ba65f715f678aaa 100644
@@ -1173,6 +1173,7 @@ static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
 
 err_unregister_trace:
 	unregister_trace_kfree_skb(ops->kfree_skb_probe, NULL);
+	tracepoint_synchronize_unregister();
 err_module_put:
 	for_each_possible_cpu(cpu) {
 		struct per_cpu_dm_data *data = &per_cpu(dm_cpu_data, cpu);

Signature · 1 line

2.55.0.1007.g17ff1f9808-goog

In drop_monitor teardown paths (net_dm_trace_off_set(), net_dm_hw_monitor_stop(), and error unwind paths in net_dm_trace_on_set() and net_dm_hw_monitor_start()), per-CPU timers are stopped using timer_delete_sync() followed by cancel_work_sync().

However, there is a circular dependency between send_timer and dm_alert_work:
1) sched_send_work() (timer callback) schedules dm_alert_work.
2) send_dm_alert() / net_dm_hw_summary_work() calls reset_per_cpu_data() or net_dm_hw_reset_per_cpu_data().
3) If memory allocation fails under memory pressure in the reset function, it re-arms the timer via mod_timer(&data->send_timer, ...).

If dm_alert_work is running concurrently while timer_delete_sync() executes on another CPU, an allocation failure in the worker will re-arm the timer after timer_delete_sync() has already returned. Once cancel_work_sync() completes and module_put() is called, the timer remains active in the timer wheel. If the module is then unloaded, the timer will fire and execute sched_send_work() in freed memory, triggering a kernel panic / use-after-free.

Switch from timer_delete_sync() to timer_shutdown_sync(). This guarantees that any in-flight timer handler has finished and prevents subsequent re-arming attempts from running workers from succeeding. When monitoring is restarted later, timer_setup() is invoked, which cleanly re-initializes the timer.

Fixes 9398e9c0b1d4 ("drop_monitor: Perform cleanup upon probe registration failure")

Signed-off-by Eric Dumazet <[email protected]>

---
 net/core/drop_monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

net/core/drop_monitor.c +4 -4

index 018d19e3a71de0be50bcc5753ba65f715f678aaa..873155ca72432924322bb7961996dd3430d052bc 100644
@@ -1083,7 +1083,7 @@ static int net_dm_hw_monitor_start(struct netlink_ext_ack *extack)
 		struct per_cpu_dm_data *hw_data = &per_cpu(dm_hw_cpu_data, cpu);
 		struct sk_buff *skb;
 
-		timer_delete_sync(&hw_data->send_timer);
+		timer_shutdown_sync(&hw_data->send_timer);
 		cancel_work_sync(&hw_data->dm_alert_work);
 		while ((skb = __skb_dequeue(&hw_data->drop_queue))) {
 			struct devlink_trap_metadata *hw_metadata;

@@ -1117,7 +1117,7 @@ static void net_dm_hw_monitor_stop(struct netlink_ext_ack *extack)
 		struct per_cpu_dm_data *hw_data = &per_cpu(dm_hw_cpu_data, cpu);
 		struct sk_buff *skb;
 
-		timer_delete_sync(&hw_data->send_timer);
+		timer_shutdown_sync(&hw_data->send_timer);
 		cancel_work_sync(&hw_data->dm_alert_work);
 		while ((skb = __skb_dequeue(&hw_data->drop_queue))) {
 			struct devlink_trap_metadata *hw_metadata;

@@ -1179,7 +1179,7 @@ static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
 		struct per_cpu_dm_data *data = &per_cpu(dm_cpu_data, cpu);
 		struct sk_buff *skb;
 
-		timer_delete_sync(&data->send_timer);
+		timer_shutdown_sync(&data->send_timer);
 		cancel_work_sync(&data->dm_alert_work);
 		while ((skb = __skb_dequeue(&data->drop_queue)))
 			consume_skb(skb);

@@ -1207,7 +1207,7 @@ static void net_dm_trace_off_set(void)
 		struct per_cpu_dm_data *data = &per_cpu(dm_cpu_data, cpu);
 		struct sk_buff *skb;
 
-		timer_delete_sync(&data->send_timer);
+		timer_shutdown_sync(&data->send_timer);
 		cancel_work_sync(&data->dm_alert_work);
 		while ((skb = __skb_dequeue(&data->drop_queue)))
 			consume_skb(skb);

Signature · 1 line

2.55.0.1007.g17ff1f9808-goog

In reset_per_cpu_data(), al is computed as:

    al = sizeof(struct net_dm_alert_msg);
    al += dm_hit_limit * sizeof(struct net_dm_drop_point);
    al += sizeof(struct nlattr);

    skb = genlmsg_new(al, GFP_KERNEL);
    ...
    nla = nla_reserve(skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg));
    ...
    msg = nla_data(nla);
    memset(msg, 0, al);

Because al includes sizeof(struct nlattr) (the 4-byte attribute header), genlmsg_new() allocates al bytes of tailroom starting at nla. However, msg points to nla_data(nla), which is located sizeof(struct nlattr) bytes past nla. Calling memset(msg, 0, al) therefore writes al bytes starting from msg, exceeding the allocated buffer by sizeof(struct nlattr) (4 bytes) and corrupting skb_shared_info.

Fix this by letting al represent only the payload length, allocating the skb with genlmsg_new(nla_total_size(al), GFP_KERNEL), and zeroing al bytes from msg.

Fixes 683703a26e46 ("drop_monitor: Update netlink protocol to include netlink attribute header in alert message")

Signed-off-by Eric Dumazet <[email protected]>

---
 net/core/drop_monitor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/core/drop_monitor.c +1 -2

index 795c15dd1771a2a5f15e109d0ef2eed967c45443..edc660778408e1bb996124be5a5349dfc9be3568 100644
@@ -141,9 +141,8 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data)
 
 	al = sizeof(struct net_dm_alert_msg);
 	al += dm_hit_limit * sizeof(struct net_dm_drop_point);
-	al += sizeof(struct nlattr);
 
-	skb = genlmsg_new(al, GFP_KERNEL);
+	skb = genlmsg_new(nla_total_size(al), GFP_KERNEL);
 
 	if (!skb)
 		goto err;

Signature · 1 line

2.55.0.1007.g17ff1f9808-goog

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Commit message overstates the impact: the pre-patch memset() in reset_per_cpu_data() (net/core/drop_monitor.c:165) is NOT an out-…

--

drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

The patch stops adding sizeof(struct nlattr) to al in reset_per_cpu_data(), and instead allocates the skb with genlmsg_new(nla_total_size(al), GFP_KERNEL) so that al describes only the attribute payload that is later zeroed via memset(msg, 0, al).

net/core/drop_monitor.c static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data) · line 145

Quoted 12 lines

net/core/drop_monitor.c +1 -2

index 795c15dd1771a..edc660778408e 100644
@@ -141,9 +141,8 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data)
 
      al = sizeof(struct net_dm_alert_msg);
      al += dm_hit_limit * sizeof(struct net_dm_drop_point);
-     al += sizeof(struct nlattr);
 
-     skb = genlmsg_new(al, GFP_KERNEL);
+     skb = genlmsg_new(nla_total_size(al), GFP_KERNEL);

[Severity: Medium]
Is the impact described in the changelog accurate? The subject says "fix out-of-bounds write", and the body says the memset() is

exceeding the allocated buffer by sizeof(struct nlattr) (4 bytes) and corrupting skb_shared_info.

With the in-tree constants, dm_hit_limit is a fixed "static int dm_hit_limit = 64" with no module_param and no way for userspace to change it, sizeof(struct net_dm_alert_msg) is 4 and sizeof(struct net_dm_drop_point) is 12, so the old al was 4 + 768 + 4 = 776.

That gives genlmsg_new(776) -> nlmsg_new(780) -> alloc_skb(796), and with nlmsghdr [0,16), genlmsghdr [16,20) and the nlattr header [20,24), msg sits at offset 24 and memset(msg, 0, 776) writes up to offset 800, i.e. 4 bytes past the 796 bytes that were requested.

But the head buffer that gets allocated is larger than the request, and skb_shared_info is placed at the end of the rounded-up zone:

net/core/skbuff.c:kmalloc_reserve() {
        ...
        obj_size = SKB_HEAD_ALIGN(*size);
        ...
        obj_size = kmalloc_size_roundup(obj_size);
        *size = (unsigned int)obj_size;
}

net/core/skbuff.c:__alloc_skb() {
        ...
        /* kmalloc_size_roundup() might give us more room than requested.
         * Put skb_shared_info exactly at the end of allocated zone,
         * to allow max possible filling before reallocation.
         */
        __finalize_skb_around(skb, data, size);
}

For a 796-byte request SKB_HEAD_ALIGN(796) is 832 + 320 = 1152, which kmalloc_size_roundup() rounds to 2048, so skb->end lands near offset 1728 and the 4 extra zeroed bytes fall in unused tailroom. KASAN would also not report anything, since kmalloc is called with the already-rounded size. The small-head-cache path gives *size = SKB_SMALL_HEAD_CACHE_SIZE, which is also comfortably above 800.

Also, nla_total_size(772) is 776, exactly the previous genlmsg_new() argument, so the allocation size is unchanged by this patch; only the memset length shrinks from 776 to 772, which matches the usable payload (796 - 24 = 772 = 4 + 64 * 12) and the fill limit enforced in trace_drop_common().

The code change itself looks correct, but with the Fixes: 683703a26e46 tag this will be picked up for stable and CVE triage as an out-of-bounds write. Could the changelog be reworded to describe it as a size-accounting cleanup, and to drop the claim about corrupting skb_shared_info?