Project

General

Profile

Actions

Bug #432

closed

bat_v: Leak of aggregated skbs

Added by Sven Eckelmann 5 months ago. Updated 4 months ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
05/13/2026
Due date:
% Done:

0%

Estimated time:

Description

See

Could a concurrent interface disable cause a memory leak later in this
function?

If a thread is preempted after passing the mesh_iface check but before
acquiring aggr_list.lock, a concurrent batadv_hardif_disable_interface() could
lock and flush the aggr_list and permanently cancel the aggr_wq worker.

When the preempted thread resumes, it takes the lock and queues the skb to the
aggr_list. Since the work queue is permanently cancelled and the list is not
explicitly purged upon destruction (batadv_hardif_release() only calls
kfree_rcu()), does the queued skb leak indefinitely?

Actions #1

Updated by Sven Eckelmann 5 months ago

@Antonio Quartulli @Marek Lindner @Linus Lüssing I don't know who is currently developing B.A.T.M.A.N. V but this has to be fixed by one of you.

Actions #2

Updated by Linus Lüssing 5 months ago

The report seems valid, this is how I think I can reliably reproduce it.

Some added mdelay()'s:


diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index 9e693cf0f761..943b0aa5d3ca 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -242,6 +242,7 @@ static void batadv_v_ogm_queue_on_if(struct batadv_priv *bat_priv,
                      struct sk_buff *skb,
                      struct batadv_hard_iface *hard_iface)
 {
+    //printk("~~~ %s:%i: start\n", __func__, __LINE__);
     if (hard_iface->mesh_iface != bat_priv->mesh_iface) {
         kfree_skb(skb);
         return;
@@ -252,10 +253,28 @@ static void batadv_v_ogm_queue_on_if(struct batadv_priv *bat_priv,
         return;
     }

+//    if (hard_iface->if_status == BATADV_IF_NOT_IN_USE) {
+//    printk("~~~ %s:%i: before locking, sleeping\n", __func__, __LINE__);
+        // delay until after batadv_v_ogm_iface_disable()
+        mdelay(50);
+        mdelay(50);
+        mdelay(50);
+        mdelay(50);
+        mdelay(50);
+//    printk("~~~ %s:%i: locking\n", __func__, __LINE__);
+//    }
+
     spin_lock_bh(&hard_iface->bat_v.aggr_list.lock);
     if (!batadv_v_ogm_queue_left(skb, hard_iface))
         batadv_v_ogm_aggr_send(bat_priv, hard_iface);

+    if (hard_iface->if_status == BATADV_IF_NOT_IN_USE)
+    printk("~~~ %s:%i: in lock, hard-iface not in use?\n", __func__, __LINE__);
+    if (hard_iface->mesh_iface != bat_priv->mesh_iface)
+    printk("~~~ %s:%i: in lock, hard_iface->mesh_iface != bat_priv->mesh_iface\n", __func__, __LINE__);
+
+
+
     hard_iface->bat_v.aggr_len += batadv_v_ogm_len(skb);
     __skb_queue_tail(&hard_iface->bat_v.aggr_list, skb);
     spin_unlock_bh(&hard_iface->bat_v.aggr_list.lock);
@@ -430,9 +449,11 @@ void batadv_v_ogm_iface_disable(struct batadv_hard_iface *hard_iface)
 {
     cancel_delayed_work_sync(&hard_iface->bat_v.aggr_wq);

+printk("~~~ %s:%i: before lock\n", __func__, __LINE__);
     spin_lock_bh(&hard_iface->bat_v.aggr_list.lock);
     batadv_v_ogm_aggr_list_free(hard_iface);
     spin_unlock_bh(&hard_iface->bat_v.aggr_list.lock);
+printk("~~~ %s:%i: after lock\n", __func__, __LINE__);
 }

 /**
diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index ca9f5c37fb54..fe95edeff1f5 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -857,7 +857,9 @@ void batadv_hardif_disable_interface(struct batadv_hard_iface *hard_iface)
         batadv_hardif_put(new_if);
     }

+printk("~~~ %s:%i: before iface.disable\n", __func__, __LINE__);
     bat_priv->algo_ops->iface.disable(hard_iface);
+printk("~~~ %s:%i: after iface.disable\n", __func__, __LINE__);
     hard_iface->if_status = BATADV_IF_NOT_IN_USE;

     /* delete all references to this hard_iface */
@@ -872,7 +874,9 @@ void batadv_hardif_disable_interface(struct batadv_hard_iface *hard_iface)
     if (batadv_hardif_cnt(hard_iface->mesh_iface) <= 1)
         batadv_gw_check_client_stop(bat_priv);

+printk("~~~ %s:%i: before mesh_iface = NULL\n", __func__, __LINE__);
     hard_iface->mesh_iface = NULL;
+printk("~~~ %s:%i: after mesh_iface = NULL\n", __func__, __LINE__);
     batadv_hardif_put(hard_iface);

 out:

And on a console in a VM running 7.1.0-rc4+:


ip link add dev brtest type bridge; ip link set up dev brtest
NUM_NODES=100; for i in `seq 1 $NUM_NODES`; do ip link add dev veth$i-in type veth peer name veth$i-out; ip link set dev veth$i-out master brtest up; ip link set dev brtest up; done; dmesg -C; echo clear > /sys/kernel/debug/kmemleak

echo clear > /sys/kernel/debug/kmemleak; sleep 5; insmod net/batman-adv/batman-adv.ko; batctl routing_algo BATMAN_V; for i in `seq 1 $NUM_NODES`; do ip link add dev bat$i type batadv; batctl meshif bat$i it 40; ip link set dev veth$i-in master bat$i up; ip link set dev bat$i up; done; sleep 5; rmmod batman-adv; sleep 30; echo scan > /sys/kernel/debug/kmemleak; sleep 5; cat /sys/kernel/debug/kmemleak

Result:


root@linus-work:/mnt/net/mesh/batman/batman-adv# cat /sys/kernel/debug/kmemleak                                               
unreferenced object 0xffff8e850a212a40 (size 704):
  comm "kworker/u32:3", pid 3692, jiffies 4295664365
  hex dump (first 32 bytes):
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
  backtrace (crc 67b3cd19):
    kmem_cache_alloc_node_noprof+0x4af/0x700
    kmalloc_reserve+0x86/0x100
    __alloc_skb+0xf4/0x240
    __netdev_alloc_skb+0x3a/0x330
    0xffffffffc0d8bfd7
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
unreferenced object 0xffff8e8505920ac0 (size 232):
  comm "kworker/u32:3", pid 3692, jiffies 4295664365
  hex dump (first 32 bytes):
    88 da f0 39 85 8e ff ff 88 da f0 39 85 8e ff ff  ...9.......9....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc 781e300e):
    kmem_cache_alloc_noprof+0x5be/0x6a0
    skb_clone+0x63/0x1e0
    0xffffffffc0d8c102
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
unreferenced object 0xffff8e8509861c40 (size 704):
  comm "kworker/u32:3", pid 3692, jiffies 4295664491
  hex dump (first 32 bytes):
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
  backtrace (crc d541e123):
    kmem_cache_alloc_node_noprof+0x4af/0x700
    kmalloc_reserve+0x86/0x100
    __alloc_skb+0xf4/0x240
    __netdev_alloc_skb+0x3a/0x330
    0xffffffffc0d8bfd7
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
unreferenced object 0xffff8e850e88a1c0 (size 232):
  comm "kworker/u32:3", pid 3692, jiffies 4295664491
  hex dump (first 32 bytes):
    88 aa f0 39 85 8e ff ff 88 aa f0 39 85 8e ff ff  ...9.......9....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc dbcc6002):
    kmem_cache_alloc_noprof+0x5be/0x6a0
    skb_clone+0x63/0x1e0
    0xffffffffc0d8c102
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
unreferenced object 0xffff8e850a5dd440 (size 704):
  comm "kworker/u32:0", pid 6197, jiffies 4295690210
  hex dump (first 32 bytes):
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
  backtrace (crc 513a5524):
    kmem_cache_alloc_node_noprof+0x4af/0x700
    kmalloc_reserve+0x86/0x100
    __alloc_skb+0xf4/0x240
    __netdev_alloc_skb+0x3a/0x330
    0xffffffffc0d8bfd7
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
unreferenced object 0xffff8e850abff540 (size 232):
  comm "kworker/u32:0", pid 6197, jiffies 4295690210
  hex dump (first 32 bytes):
    88 86 08 21 85 8e ff ff 88 86 08 21 85 8e ff ff  ...!.......!....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc b2b116e):
    kmem_cache_alloc_noprof+0x5be/0x6a0
    skb_clone+0x63/0x1e0
    0xffffffffc0d8c102
    process_one_work+0x228/0x600
    worker_thread+0x1ee/0x3d0
    kthread+0xf4/0x130
    ret_from_fork+0x38e/0x420
    ret_from_fork_asm+0x1a/0x30
[...]

Will try to come up with a fix for it.

Actions #3

Updated by Sven Eckelmann 4 months ago

  • Description updated (diff)
Actions #5

Updated by Sven Eckelmann 4 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Sven Eckelmann 4 months ago

  • Status changed from In Progress to Resolved
Actions #7

Updated by Sven Eckelmann 4 months ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF