Skip to contentSkip to conversation

batman-adv: fix tp_meter counter underflow during shutdown

PATCH In review Ren Wei 2 participants 2 messages May 7 1 message not in the archive

Subsystems (3)
Networking Network drivers Batman advanced
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author
#1 lore raw

PATCH 1/1 batman-adv: fix tp_meter counter underflow during shutdown

From: Luxiao Xu <[email protected]>

batadv_tp_sender_shutdown() unconditionally decrements the "sending" atomic counter. If multiple paths (e.g. timeout, user cancel, and normal finish) call this function, the counter can underflow to -1.

Since the sender logic treats any non-zero value as "still sending", a negative value causes the sender kthread to loop indefinitely. This leads to a use-after-free when the interface is removed while the zombie thread is still active.

Fix this by using atomic_xchg() to ensure the counter only transitions from 1 to 0 once.

Fixes 33a3bb4a3345 ("batman-adv: throughput meter implementation")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Luxiao Xu <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 net/batman-adv/tp_meter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/batman-adv/tp_meter.c +1 -1
index 2e42f6b348c8..4c582443f67c 100644
@@ -435,7 +435,7 @@ static void batadv_tp_sender_end(struct batadv_priv *bat_priv,
 static void batadv_tp_sender_shutdown(struct batadv_tp_vars *tp_vars,
 				      enum batadv_tp_meter_reason reason)
 {
-	if (!atomic_dec_and_test(&tp_vars->sending))
+	if (atomic_xchg(&tp_vars->sending, 0) != 1)
 		return;
 
 	tp_vars->reason = reason;
Signature · 1 line
2.43.0
maintainer
#2 lore raw

net/batman-adv/tp_meter.c static void batadv_tp_sender_end(struct batadv_priv *bat_priv, · line 441

Quoted 13 lines · Ren Wei
net/batman-adv/tp_meter.c +1 -1
index 2e42f6b348c8..4c582443f67c 100644
@@ -435,7 +435,7 @@ static void batadv_tp_sender_end(struct batadv_priv *bat_priv,
 static void batadv_tp_sender_shutdown(struct batadv_tp_vars *tp_vars,
 				      enum batadv_tp_meter_reason reason)
 {
-	if (!atomic_dec_and_test(&tp_vars->sending))
+	if (atomic_xchg(&tp_vars->sending, 0) != 1)
 		return;
 
 	tp_vars->reason = reason;

What about:

	/* ensure nobody else tries to stop the thread now */
	if (atomic_dec_and_test(&tp_vars->sending))
		tp_vars->reason = err;
	break;

in batadv_tp_send()? If shutdown is called and then batadv_tp_send reaches this part, isn't this also ending up at -1?

Regarding the netdev mail situation: Please don't send it to [email protected] directly - they want us to first handle it internally before forwarding it later to netdev. This should reduce the patch volume for them. There is already a change to the MAINTAINERS file to remove the batman-adv paths from the NETWORKING section (netdev@....) queued up
- but it was not yet forwarded to net-next (and I am just wondering whether I should ask Simon to directly submit it to net).

Regards,
Sven

Signature · 1 line
Sven Eckelmann <[email protected]>