[PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

[PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

[Ren Wei]

From: Jiexun Wang <wangjiexun2025@gmail.com>

BAT IV keeps the last-hop neighbor address in each neigh_node, but some
paths also cache an originator pointer derived from a temporary lookup.
That pointer is not owned by the neigh_node and may no longer refer to a
live originator entry after purge handling runs.

Stop storing the auxiliary originator pointer in the BAT IV neighbor
state. When BAT IV needs the neighbor originator data, resolve it from
the stored neighbor address and drop the reference again after use.

Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/batman-adv/bat_iv_ogm.c | 56 +++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index f28e9cbf8ad5..168b413dd18b 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -173,19 +173,12 @@ batadv_iv_ogm_orig_get(struct batadv_priv *bat_priv, const u8 *addr)
 static struct batadv_neigh_node *
 batadv_iv_ogm_neigh_new(struct batadv_hard_iface *hard_iface,
 			const u8 *neigh_addr,
-			struct batadv_orig_node *orig_node,
-			struct batadv_orig_node *orig_neigh)
+			struct batadv_orig_node *orig_node)
 {
 	struct batadv_neigh_node *neigh_node;
 
 	neigh_node = batadv_neigh_node_get_or_create(orig_node,
 						     hard_iface, neigh_addr);
-	if (!neigh_node)
-		goto out;
-
-	neigh_node->orig_node = orig_neigh;
-
-out:
 	return neigh_node;
 }
 
@@ -906,6 +899,31 @@ static u8 batadv_iv_orig_ifinfo_sum(struct batadv_orig_node *orig_node,
 	return sum;
 }
 
+/**
+ * batadv_iv_ogm_neigh_ifinfo_sum() - Get bcast_own sum for a last-hop neighbor
+ * @bat_priv: the bat priv with all the mesh interface information
+ * @neigh_node: last-hop neighbor of an originator
+ *
+ * Return: Number of replied (rebroadcasted) OGMs for the originator currently
+ * announced by the neighbor. Returns 0 if the neighbor's originator entry is
+ * not available anymore.
+ */
+static u8 batadv_iv_ogm_neigh_ifinfo_sum(struct batadv_priv *bat_priv,
+					 const struct batadv_neigh_node *neigh_node)
+{
+	struct batadv_orig_node *orig_neigh;
+	u8 sum;
+
+	orig_neigh = batadv_orig_hash_find(bat_priv, neigh_node->addr);
+	if (!orig_neigh)
+		return 0;
+
+	sum = batadv_iv_orig_ifinfo_sum(orig_neigh, neigh_node->if_incoming);
+	batadv_orig_node_put(orig_neigh);
+
+	return sum;
+}
+
 /**
  * batadv_iv_ogm_orig_update() - use OGM to update corresponding data in an
  *  originator
@@ -975,17 +993,9 @@ batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
 	}
 
 	if (!neigh_node) {
-		struct batadv_orig_node *orig_tmp;
-
-		orig_tmp = batadv_iv_ogm_orig_get(bat_priv, ethhdr->h_source);
-		if (!orig_tmp)
-			goto unlock;
-
 		neigh_node = batadv_iv_ogm_neigh_new(if_incoming,
 						     ethhdr->h_source,
-						     orig_node, orig_tmp);
-
-		batadv_orig_node_put(orig_tmp);
+						     orig_node);
 		if (!neigh_node)
 			goto unlock;
 	} else {
@@ -1037,10 +1047,9 @@ batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
 	 */
 	if (router_ifinfo &&
 	    neigh_ifinfo->bat_iv.tq_avg == router_ifinfo->bat_iv.tq_avg) {
-		sum_orig = batadv_iv_orig_ifinfo_sum(router->orig_node,
-						     router->if_incoming);
-		sum_neigh = batadv_iv_orig_ifinfo_sum(neigh_node->orig_node,
-						      neigh_node->if_incoming);
+		sum_orig = batadv_iv_ogm_neigh_ifinfo_sum(bat_priv, router);
+		sum_neigh = batadv_iv_ogm_neigh_ifinfo_sum(bat_priv,
+							   neigh_node);
 		if (sum_orig >= sum_neigh)
 			goto out;
 	}
@@ -1106,7 +1115,6 @@ static bool batadv_iv_ogm_calc_tq(struct batadv_orig_node *orig_node,
 	if (!neigh_node)
 		neigh_node = batadv_iv_ogm_neigh_new(if_incoming,
 						     orig_neigh_node->orig,
-						     orig_neigh_node,
 						     orig_neigh_node);
 
 	if (!neigh_node)
@@ -1372,8 +1380,8 @@ batadv_iv_ogm_process_per_outif(const struct sk_buff *skb, int ogm_offset,
 
 	router = batadv_orig_router_get(orig_node, if_outgoing);
 	if (router) {
-		router_router = batadv_orig_router_get(router->orig_node,
-						       if_outgoing);
+		router_router = batadv_orig_to_router(bat_priv, router->addr,
+						      if_outgoing);
 		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
 	}
 
-- 
2.34.1

Re: [PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

[Sven Eckelmann]

On Sun, 03 May 2026 12:28:58 +0800, Ren Wei <n05ec@lzu.edu.cn> wrote:
> [...]
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>

This looks half finished from the types perspective: 

net/batman-adv/bat_v_ogm.c:
713 |         if (router && router->orig_node != orig_node && !orig_neigh_router) {

net/batman-adv/originator.c:
697 |         neigh_node->orig_node = orig_node;

net/batman-adv/types.h:

631 |         struct batadv_orig_node *orig_node;


Not sure if __private and ACCESS_PRIVATE() would be an option - just to handle
this non-deref comparison while still allowing a fast comparison of the pointer
value.

I don't want to make this a show-stopper - just a possibility to think about
this for a moment. Especially because I am waiting for some info about the
sashiko.dev "Embargoed" state

>
>
> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index f28e9cbf..168b413d 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c
> @@ -906,6 +899,31 @@ static u8 batadv_iv_orig_ifinfo_sum(struct batadv_orig_node *orig_node,
> [ ... skip 22 lines ... ]
> +	sum = batadv_iv_orig_ifinfo_sum(orig_neigh, neigh_node->if_incoming);
> +	batadv_orig_node_put(orig_neigh);
> +
> +	return sum;
> +}
> +

@Marek, @Simon, Maybe you can also get a good cup of tea and think (besides
performance implications) think about other side effects.

-- 
Sven Eckelmann <sven@narfation.org>

Re: [PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

[Sven Eckelmann]

On Sun, 03 May 2026 12:28:58 +0800, Ren Wei wrote:
> batman-adv: stop caching unowned originator pointers in BAT IV

Applied, thanks!

[1/1] batman-adv: stop caching unowned originator pointers in BAT IV
      https://git.open-mesh.org/batadv/c/37760490adc1

Best regards,
-- 
Sven Eckelmann <sven@narfation.org>

Re: [PATCH net 1/1] batman-adv: stop caching unowned originator pointers in BAT IV

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

I do have concerns that this review needs to be addressed in
order for this patch to progress.

OTOH, there is another AI-generated review of this patch available at
https://sashiko.dev. In that case, I believe the review relates to
a pre-existing condition, and addressing that (or not) can be treated
as a follow-up.

---
batman-adv: stop caching unowned originator pointers in BAT IV

This patch removes the orig_node back-pointer cached inside each BAT IV
neigh_node and resolves the originator on demand via batadv_orig_hash_find()
from the stored neighbor address. Tagged Fixes: c6c8fea29769 and
Cc: stable@kernel.org.

> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index f28e9cbf8ad5..168b413dd18b 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c

[ ... ]

> @@ -1372,8 +1380,8 @@ batadv_iv_ogm_process_per_outif(const struct sk_buff *skb, int ogm_offset,
>  
>  	router = batadv_orig_router_get(orig_node, if_outgoing);
>  	if (router) {
> -		router_router = batadv_orig_router_get(router->orig_node,
> -						       if_outgoing);
> +		router_router = batadv_orig_to_router(bat_priv, router->addr,
> +						      if_outgoing);
>  		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
>  	}

Is this replacement semantically equivalent to the original call?

The previous code used batadv_orig_router_get() on the cached orig_node to
fetch router_router. The new code calls batadv_orig_to_router(), which
internally runs batadv_find_router():

    net/batman-adv/originator.c:batadv_orig_to_router() {
        orig_node = batadv_orig_hash_find(bat_priv, orig_addr);
        ...
        neigh_node = batadv_find_router(bat_priv, orig_node, if_outgoing);
        ...
    }

batadv_find_router() has a bonding path that is taken when
recv_if == BATADV_IF_DEFAULT and bonding is enabled:

    net/batman-adv/routing.c:batadv_find_router() {
        ...
        if (!(recv_if == BATADV_IF_DEFAULT &&
              atomic_read(&bat_priv->bonding)))
            return router;

        /* bonding: loop through the list of possible routers ... */
        ...
        if (next_candidate) {
            ...
            batadv_last_bonding_replace(orig_node, next_candidate);
        } else if (first_candidate) {
            ...
            batadv_last_bonding_replace(orig_node, first_candidate);
        } else {
            batadv_last_bonding_replace(orig_node, NULL);
        }
        ...
    }

batadv_iv_ogm_process() invokes batadv_iv_ogm_process_per_outif() once with
BATADV_IF_DEFAULT per received OGM:

    net/batman-adv/bat_iv_ogm.c:batadv_iv_ogm_process() {
        ...
        batadv_iv_ogm_process_per_outif(skb, ogm_offset, orig_node,
                                        if_incoming, BATADV_IF_DEFAULT);
        ...
    }

With bonding enabled, does every received OGM now rotate
orig_node->last_bonding_candidate on the last-hop neighbor's originator via
batadv_last_bonding_replace(), where previously that state was advanced only
from forwarding paths?

And can the bonding candidate selection return a different router_router than
batadv_orig_router_get() would have, which then feeds the subsequent
loop-avoidance check?

    if (router && router_router &&
        (batadv_compare_eth(router->addr, prev_sender)) &&
        !(batadv_compare_eth(ogm_packet->orig, prev_sender)) &&
        (batadv_compare_eth(router->addr, router_router->addr))) {
        ...
        goto out;
    }

The sister helper added by this same patch, batadv_iv_ogm_neigh_ifinfo_sum(),
uses batadv_orig_hash_find() followed by batadv_iv_orig_ifinfo_sum() rather
than batadv_orig_to_router():

> +static u8 batadv_iv_ogm_neigh_ifinfo_sum(struct batadv_priv *bat_priv,
> +					 const struct batadv_neigh_node *neigh_node)
> +{
> +	struct batadv_orig_node *orig_neigh;
> +	u8 sum;
> +
> +	orig_neigh = batadv_orig_hash_find(bat_priv, neigh_node->addr);
> +	if (!orig_neigh)
> +		return 0;
> +
> +	sum = batadv_iv_orig_ifinfo_sum(orig_neigh, neigh_node->if_incoming);
> +	batadv_orig_node_put(orig_neigh);
> +
> +	return sum;
> +}

Would a similar pattern work here, something like:

    orig = batadv_orig_hash_find(bat_priv, router->addr);
    router_router = batadv_orig_router_get(orig, if_outgoing);
    batadv_orig_node_put(orig);

so that the lookup avoids invoking the bonding candidate machinery from the
OGM processing path?

The commit message describes only removing an unowned cached originator
pointer and does not mention a routing or bonding behavior change. Is the
change in router_router resolution intentional, and if so could the commit
message call it out?