On 22 Sep 2026, at 17:40, Gaetan Rivet wrote:
> 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.
Sounds like a good middle ground. The refcount read inside log_node()
would need to use memory_order_acquire rather than a relaxed load
(I think). A comment explaining why we skip on 0, would also be helpful
as a reference in the code.
> 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
