[PATCH net 0/4] net: drop_monitor: fix concurrency issues, preemption warning, and buffer overrun

[PATCH net 0/4] net: drop_monitor: fix concurrency issues, preemption warning, and buffer overrun

[Eric Dumazet]

This series addresses several issues discovered in the drop_monitor subsystem:

Patch 1 adds missing tracepoint unregistration synchronization to the
net_dm_trace_on_set() error unwind path, preventing in-flight probes
from scheduling work after the module reference has been dropped.

Patch 2 resolves a race condition during monitoring teardown where per-CPU
timers can be re-armed after deletion if a concurrent worker encounters a
memory allocation failure, switching to timer_shutdown_sync().

Patch 3 fixes a CONFIG_DEBUG_PREEMPT warning reported by syzbot when
kfree_skb() is invoked from preemptible process context, using raw_cpu_ptr()
since each per-CPU queue is safely protected by its own spinlock.

Patch 4 fixes an out-of-bounds write in reset_per_cpu_data() where memset()
overwrote the allocated SKB tailroom by sizeof(struct nlattr) bytes.

Eric Dumazet (4):
  drop_monitor: synchronize tracepoint unregistration on error path
  drop_monitor: use timer_shutdown_sync() to prevent timer rearming
    during teardown
  drop_monitor: use raw_cpu_ptr() in tracepoint probes
  drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

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

-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 1/4] drop_monitor: synchronize tracepoint unregistration on error path

[Eric Dumazet]

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 <edumazet@google.com>
---
 net/core/drop_monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index abaf108ac4db8c762c23f1d9c708c687ee36c7c9..018d19e3a71de0be50bcc5753ba65f715f678aaa 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -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);
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 2/4] drop_monitor: use timer_shutdown_sync() to prevent timer rearming during teardown

[Eric Dumazet]

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 <edumazet@google.com>
---
 net/core/drop_monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 018d19e3a71de0be50bcc5753ba65f715f678aaa..873155ca72432924322bb7961996dd3430d052bc 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -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);
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 3/4] drop_monitor: use raw_cpu_ptr() in tracepoint probes

[Eric Dumazet]

syzbot reported a preemption warning in sk_skb_reason_drop():

 BUG: using smp_processor_id() in preemptible [00000000] code: syz.0.17/5917
 caller is net_dm_packet_trace_kfree_skb_hit+0x119/0x350 net/core/drop_monitor.c:519

In net_dm_packet_trace_kfree_skb_hit(), data = this_cpu_ptr(&dm_cpu_data)
is evaluated before spin_lock_irqsave(&data->drop_queue.lock, flags).
When kfree_skb() is called from preemptible context (e.g. process context
during close() on /dev/net/tun), preemption is enabled, triggering the
CONFIG_DEBUG_PREEMPT warning in smp_processor_id().

The same pattern exists in net_dm_hw_trap_summary_probe() and
net_dm_hw_trap_packet_probe() for dm_hw_cpu_data.

This is a false positive because each per-cpu structure is protected
by its own spinlock. If the task migrates to another CPU right after
reading the per-cpu pointer, the lock still safely synchronizes
access to that queue.

Use raw_cpu_ptr() instead of this_cpu_ptr() to silence
CONFIG_DEBUG_PREEMPT without disturbing interrupt state or breaking
PREEMPT_RT locking semantics.

Fixes: ca30707dee2b ("drop_monitor: Add packet alert mode")
Fixes: 5855357cd40e ("drop_monitor: Prepare probe functions for devlink tracepoint")
Reported-by: syzbot+dc57fd6722deb17e92af@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6aa316b2.f81106d8.2ab401.0014.GAE@google.com/
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/drop_monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 873155ca72432924322bb7961996dd3430d052bc..795c15dd1771a2a5f15e109d0ef2eed967c45443 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -448,7 +448,7 @@ net_dm_hw_trap_summary_probe(void *ignore, const struct devlink *devlink,
 	if (metadata->trap_type == DEVLINK_TRAP_TYPE_CONTROL)
 		return;
 
-	hw_data = this_cpu_ptr(&dm_hw_cpu_data);
+	hw_data = raw_cpu_ptr(&dm_hw_cpu_data);
 	raw_spin_lock_irqsave(&hw_data->lock, flags);
 	hw_entries = hw_data->hw_entries;
 
@@ -516,7 +516,7 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
 	 */
 	nskb->tstamp = tstamp;
 
-	data = this_cpu_ptr(&dm_cpu_data);
+	data = raw_cpu_ptr(&dm_cpu_data);
 
 	spin_lock_irqsave(&data->drop_queue.lock, flags);
 	if (skb_queue_len(&data->drop_queue) < net_dm_queue_len)
@@ -983,7 +983,7 @@ net_dm_hw_trap_packet_probe(void *ignore, const struct devlink *devlink,
 	NET_DM_SKB_CB(nskb)->hw_metadata = n_hw_metadata;
 	nskb->tstamp = tstamp;
 
-	hw_data = this_cpu_ptr(&dm_hw_cpu_data);
+	hw_data = raw_cpu_ptr(&dm_hw_cpu_data);
 
 	spin_lock_irqsave(&hw_data->drop_queue.lock, flags);
 	if (skb_queue_len(&hw_data->drop_queue) < net_dm_queue_len)
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net 4/4] drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

[Eric Dumazet]

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 <edumazet@google.com>
---
 net/core/drop_monitor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 795c15dd1771a2a5f15e109d0ef2eed967c45443..edc660778408e1bb996124be5a5349dfc9be3568 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -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;
-- 
2.55.0.1007.g17ff1f9808-goog

Re: [PATCH net 1/4] drop_monitor: synchronize tracepoint unregistration on error path

[Hangbin Liu]

On Thu, Sep 10, 2026 at 08:46:09PM +0000, Eric Dumazet wrote:
> 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 <edumazet@google.com>
> ---
>  net/core/drop_monitor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index abaf108ac4db8c762c23f1d9c708c687ee36c7c9..018d19e3a71de0be50bcc5753ba65f715f678aaa 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -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);
> -- 
> 2.55.0.1007.g17ff1f9808-goog
> 

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

Re: [PATCH net 2/4] drop_monitor: use timer_shutdown_sync() to prevent timer rearming during teardown

[Hangbin Liu]

On Thu, Sep 10, 2026 at 08:46:10PM +0000, Eric Dumazet wrote:
> 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")

Nit, not sure if we need add

70c69274f354 ("drop_monitor: Initialize timer and work item upon tracing enable")
and 8e94c3bc922e ("drop_monitor: Allow user to start monitoring hardware drops")

Thanks
Hangbin

> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/drop_monitor.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 018d19e3a71de0be50bcc5753ba65f715f678aaa..873155ca72432924322bb7961996dd3430d052bc 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -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);
> -- 
> 2.55.0.1007.g17ff1f9808-goog
> 

Re: [PATCH net 4/4] drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

[Hangbin Liu]

On Thu, Sep 10, 2026 at 08:46:12PM +0000, Eric Dumazet wrote:
> 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 <edumazet@google.com>
> ---
>  net/core/drop_monitor.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 795c15dd1771a2a5f15e109d0ef2eed967c45443..edc660778408e1bb996124be5a5349dfc9be3568 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -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;
> -- 
> 2.55.0.1007.g17ff1f9808-goog
> 

Yes, nla_total_size is safer.

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

Re: [PATCH net 4/4] drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

[netdev-bot+sashiko]

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).

> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 795c15dd1771a..edc660778408e 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910204612.3762015-1-edumazet%40google.com

Re: [PATCH net 4/4] drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

[Eric Dumazet]

On Fri, Sep 11, 2026 at 2:08 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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).
>
> > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> > index 795c15dd1771a..edc660778408e 100644
> > --- a/net/core/drop_monitor.c
> > +++ b/net/core/drop_monitor.c
> > @@ -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?

This feedback is quite shocking and annoying.

Do I need to double check everything and make precise computations?

No, I do not.