On Mon Sep 21, 2026 at 2:26 PM CEST, Eelco Chaudron via dev wrote:
> Hi Eli/Gaetan,
>
> Ilya has been working on some Sashiko code review scripts, and as part of
> his tests, he was running them on the netdev-doca series. He shared the
> results with me, and I'm going over them to filter out the ones that seem
> relevant.
>
> See the one for this patch below. So be prepared to see some more review
> comments from Sashiko AI on the series ;)
>
> Cheers,
>
> Eelco
>
>
> commit 174078f14e26 ("refmap: Introduce reference map.")
> Author: Gaetan Rivet <[email protected]>
>
> This patch introduces a reference-counted concurrent map (refmap)
> with RCU-based lookups and deferred node freeing.
>
> > if (!ovs_refcount_try_ref_rcu(&node->refcount)) {
> > log_node(rfm, "try_ref-fail", node);
> > return NULL;
> > }
>
> In refmap_try_ref(), when ovs_refcount_try_ref_rcu() returns false,
> log_node() is called on a node whose value may have already been
> invalidated by a concurrent refmap_unref(). The race looks like
> this: thread A finds the node via refmap_lookup(), then thread B
> completes refmap_unref() -- transitioning the refcount from 1 to 0
> under map_lock and calling value_uninit() on the value:
>
> > if (old_refcount == 1) {
> > /* We transitioned 1->0 under lock. Safe to cleanup. */
> > rfm->value_uninit(refmap_node_value(rfm, node));
> > cmap_remove(&rfm->map, &node->map_node, node->hash);
> > [...]
> > ovsrcu_postpone(free, node);
>
> Thread A then enters log_node(), which calls rfm->value_format()
> on the now-uninited value:
>
> > key = refmap_node_key(node);
> > value = refmap_node_value(rfm, node);
> > [...]
> > rfm->value_format(&s, key, value);
>
> The node memory itself is still valid (RCU-deferred free), but
> value_uninit() may have nullified or freed pointers within the
> value structure. Can value_format() then dereference an invalid
> pointer inside the value, causing a null-pointer dereference or
> use-after-free?
>
> This only triggers when debug logging is enabled and value_format
> is non-null, so it is a cold path under default configuration.
> Would it make sense to skip the log_node() call on the try-ref
> failure path, or copy the needed fields before the try-ref
> attempt?
>
Hi Eelco,
I think either we skip log_node(), or an alternative is in log_node()
to only call value_format() if refcount is not zero.
This should be safe, as the only possibility to call log_node() with
ref == 0 is after the last unref, as the try-ref failed. At this point
there is no possibility of the refcount getting back above 0.
As such, within log_node() we can still print the value pointer,
which as I recall has been very useful to grep and trace the lifecycle
of one element -- here we would then see even the last try-ref that
failed, which I think is a pretty valuable information.
Let me know if you'd prefer we completely remove the call.
--
Gaetan
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
