| Message ID | 2C16ACF0E6269614+20260114063241.38751-1-i@liuyulong.me |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v4] ofproto-dpif-xlate: Use try-ref for xlate_table_action rule. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Test pass: https://cirrus-ci.com/build/5213878944006144 https://github.com/ovsrobot/ovs/actions/runs/20985028514 ------------------ Original ------------------ From: "LIU Yulong"<i@liuyulong.me>; Date: Wed, Jan 14, 2026 02:32 PM To: "dev"<dev@openvswitch.org>; Cc: "LIU Yulong"<i@liuyulong.me>; Subject: [PATCH v4] ofproto-dpif-xlate: Use try-ref for xlate_table_action rule. Our running ovs-vswitchd was cored with stack [1]. It shows a concurrent race condition. When the revalidator thread is performing rule revalidation/translation, another thread has already deleted the rule tag and reduced the reference count to 0. Using ofproto_rule_ref() at this point will trigger an assertion, as it assumes the object is still alive (refcount > 0). Replace the mandatory reference to the rule in xlate_table_action() with a non-fatal try-ref: ofproto_rule_try_ref(). If the reference fails (the rule is being/has been released), treat it as a table miss path, avoiding referencing "dead" rules and causing assertion errors. [1] call stack: *0 raise () from /lib64/libc.so.6 *1 abort () from /lib64/libc.so.6 *2 ovs_abort_valist () at lib/util.c:499 *3 vlog_abort_valist () at lib/vlog.c:1249 *4 vlog_abort () at lib/vlog.c:1263 *5 ovs_assert_failure () at lib/util.c:86 *6 ovs_refcount_ref () at lib/ovs-atomic.h:547 *7 ovs_refcount_ref () at ofproto/ofproto.c:3010 *8 ofproto_rule_ref () at ofproto/ofproto.c:3012 *9 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4506 *10 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 *11 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 *12 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 *13 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 *14 clone_xlate_actions () at ofproto/ofproto-dpif-xlate.c:5809 *15 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 *16 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 *17 patch_port_output () at ofproto/ofproto-dpif-xlate.c:3889 *18 compose_output_action__ () at ofproto/ofproto-dpif-xlate.c:4204 *19 compose_output_action () at ofproto/ofproto-dpif-xlate.c:4359 *20 xlate_output_action () at ofproto/ofproto-dpif-xlate.c:5304 *21 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7033 *22 xlate_actions () at ofproto/ofproto-dpif-xlate.c:8001 *23 xlate_key () at ofproto/ofproto-dpif-upcall.c:2321 *24 xlate_ukey () at ofproto/ofproto-dpif-upcall.c:2336 *25 revalidate_ukey__ () at ofproto/ofproto-dpif-upcall.c:2382 *26 revalidate_ukey () at ofproto/ofproto-dpif-upcall.c:2491 *27 revalidate () at ofproto/ofproto-dpif-upcall.c:2945 *28 udpif_revalidator () at ofproto/ofproto-dpif-upcall.c:1089 *29 ovsthread_wrapper () at lib/ovs-thread.c:422 *30 start_thread () from /lib64/libpthread.so.0 *31 clone () from /lib64/libc.so.6 Signed-off-by: LIU Yulong <i@liuyulong.me> --- V4: - Address ci and clang-analyze failure V3: - Addressed Addressed Ilya's and Adrián's comments: - release the reference unconditionally - Addressed Adrián's comments: - make code consistent in xlate_actions() V2: - Addressed Eelco's comments: - release the reference after ofproto_rule_try_ref() - Addressed Ilya's comments: - remove the commit message function args and mem addresses --- ofproto/ofproto-dpif-xlate.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 920d998e6..d21f7f003 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4793,7 +4793,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, tuple_swap(&ctx->xin->flow, ctx->wc); } - if (rule) { + if (rule && ofproto_rule_try_ref(&rule->up)) { /* Fill in the cache entry here instead of xlate_recursively * to make the reference counting more explicit. We take a * reference in the lookups above if we are going to cache the @@ -4805,6 +4805,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, entry->rule = rule; ofproto_rule_ref(&rule->up); } + ofproto_rule_unref(&rule->up); struct ovs_list *old_trace = ctx->xin->trace; xlate_report_table(ctx, rule, table_id); @@ -8457,15 +8458,18 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) if (ctx.xin->resubmit_stats) { rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false); } - if (ctx.xin->xcache) { - struct xc_entry *entry; + if (ctx.rule && ofproto_rule_try_ref(&ctx.rule->up)) { + if (ctx.xin->xcache) { + struct xc_entry *entry; - entry = xlate_cache_add_entry(ctx.xin->xcache, XC_RULE); - entry->rule = ctx.rule; - ofproto_rule_ref(&ctx.rule->up); - } + entry = xlate_cache_add_entry(ctx.xin->xcache, XC_RULE); + entry->rule = ctx.rule; + ofproto_rule_ref(&ctx.rule->up); + } + ofproto_rule_unref(&ctx.rule->up); - xlate_report_table(&ctx, ctx.rule, ctx.table_id); + xlate_report_table(&ctx, ctx.rule, ctx.table_id); + } if (OVS_UNLIKELY(ctx.xin->trace)) { hmapx_clear(ctx.conj_flows);
On 1/14/26 7:32 AM, LIU Yulong wrote: > Our running ovs-vswitchd was cored with stack [1]. It shows a > concurrent race condition. > > When the revalidator thread is performing rule revalidation/translation, > another thread has already deleted the rule tag and reduced the reference > count to 0. Using ofproto_rule_ref() at this point will trigger an assertion, > as it assumes the object is still alive (refcount > 0). > > Replace the mandatory reference to the rule in xlate_table_action() > with a non-fatal try-ref: ofproto_rule_try_ref(). > If the reference fails (the rule is being/has been released), > treat it as a table miss path, avoiding referencing "dead" rules > and causing assertion errors. > > [1] call stack: > *0 raise () from /lib64/libc.so.6 > *1 abort () from /lib64/libc.so.6 > *2 ovs_abort_valist () at lib/util.c:499 > *3 vlog_abort_valist () at lib/vlog.c:1249 > *4 vlog_abort () at lib/vlog.c:1263 > *5 ovs_assert_failure () at lib/util.c:86 > *6 ovs_refcount_ref () at lib/ovs-atomic.h:547 > *7 ovs_refcount_ref () at ofproto/ofproto.c:3010 > *8 ofproto_rule_ref () at ofproto/ofproto.c:3012 > *9 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4506 > *10 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 > *11 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 > *12 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 > *13 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 > *14 clone_xlate_actions () at ofproto/ofproto-dpif-xlate.c:5809 > *15 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 > *16 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 > *17 patch_port_output () at ofproto/ofproto-dpif-xlate.c:3889 > *18 compose_output_action__ () at ofproto/ofproto-dpif-xlate.c:4204 > *19 compose_output_action () at ofproto/ofproto-dpif-xlate.c:4359 > *20 xlate_output_action () at ofproto/ofproto-dpif-xlate.c:5304 > *21 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7033 > *22 xlate_actions () at ofproto/ofproto-dpif-xlate.c:8001 > *23 xlate_key () at ofproto/ofproto-dpif-upcall.c:2321 > *24 xlate_ukey () at ofproto/ofproto-dpif-upcall.c:2336 > *25 revalidate_ukey__ () at ofproto/ofproto-dpif-upcall.c:2382 > *26 revalidate_ukey () at ofproto/ofproto-dpif-upcall.c:2491 > *27 revalidate () at ofproto/ofproto-dpif-upcall.c:2945 > *28 udpif_revalidator () at ofproto/ofproto-dpif-upcall.c:1089 > *29 ovsthread_wrapper () at lib/ovs-thread.c:422 > *30 start_thread () from /lib64/libpthread.so.0 > *31 clone () from /lib64/libc.so.6 > > Signed-off-by: LIU Yulong <i@liuyulong.me> > --- > V4: > - Address ci and clang-analyze failure > V3: > - Addressed Addressed Ilya's and Adrián's comments: > - release the reference unconditionally > - Addressed Adrián's comments: > - make code consistent in xlate_actions() > V2: > - Addressed Eelco's comments: > - release the reference after ofproto_rule_try_ref() > - Addressed Ilya's comments: > - remove the commit message function args and mem addresses > --- > ofproto/ofproto-dpif-xlate.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 920d998e6..d21f7f003 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4793,7 +4793,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > tuple_swap(&ctx->xin->flow, ctx->wc); > } > > - if (rule) { > + if (rule && ofproto_rule_try_ref(&rule->up)) { This still doesn't allow to use the rule when we're not caching. As I said in my review for the very first version of the patch, if we're not going to cache, we should not attempt to take the reference and we should still proceed flow translation with the rule, as it should be around for the current grace period. It would look closer to how the first version of this patch looked like: if (rule && ctx->xin->xcache && !ofproto_rule_try_ref(&rule->up)) { rule = NULL; } Same for the other part inside the xlate_actions() function. However, it doesn't seem like the right solution in general. Reading the original change from 2015 that changed the code to take references directly: 1e1e1d19e6bb ("ofproto-dpif: Use a regular ref instead of try_ref for rule translation.") The logic in there still holds. For the issue to happen we need the classifier to have the rule in it during the current grace period, but also somehow the reference count should go down to zero within the same RCU grace period. That should not be possible, as the reference count decrease is on itself is RCU-postponed. So, after removal from the classifier, the rule must still have the non-zero reference until the next RCU grace period. It means that if we were able to look this rule up, it must have a non-zero reference count. And so we should not need to use the try_ref functions. It looks like this crash is just a symptom of the RCU management going wrong somewhere else, and the fact of taking the reference in the flow translation code is not the root cause of the issue and we shouldn't try to hide it by using a try_ref. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 920d998e6..d21f7f003 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4793,7 +4793,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, tuple_swap(&ctx->xin->flow, ctx->wc); } - if (rule) { + if (rule && ofproto_rule_try_ref(&rule->up)) { /* Fill in the cache entry here instead of xlate_recursively * to make the reference counting more explicit. We take a * reference in the lookups above if we are going to cache the @@ -4805,6 +4805,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, entry->rule = rule; ofproto_rule_ref(&rule->up); } + ofproto_rule_unref(&rule->up); struct ovs_list *old_trace = ctx->xin->trace; xlate_report_table(ctx, rule, table_id); @@ -8457,15 +8458,18 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) if (ctx.xin->resubmit_stats) { rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false); } - if (ctx.xin->xcache) { - struct xc_entry *entry; + if (ctx.rule && ofproto_rule_try_ref(&ctx.rule->up)) { + if (ctx.xin->xcache) { + struct xc_entry *entry; - entry = xlate_cache_add_entry(ctx.xin->xcache, XC_RULE); - entry->rule = ctx.rule; - ofproto_rule_ref(&ctx.rule->up); - } + entry = xlate_cache_add_entry(ctx.xin->xcache, XC_RULE); + entry->rule = ctx.rule; + ofproto_rule_ref(&ctx.rule->up); + } + ofproto_rule_unref(&ctx.rule->up); - xlate_report_table(&ctx, ctx.rule, ctx.table_id); + xlate_report_table(&ctx, ctx.rule, ctx.table_id); + } if (OVS_UNLIKELY(ctx.xin->trace)) { hmapx_clear(ctx.conj_flows);
Our running ovs-vswitchd was cored with stack [1]. It shows a concurrent race condition. When the revalidator thread is performing rule revalidation/translation, another thread has already deleted the rule tag and reduced the reference count to 0. Using ofproto_rule_ref() at this point will trigger an assertion, as it assumes the object is still alive (refcount > 0). Replace the mandatory reference to the rule in xlate_table_action() with a non-fatal try-ref: ofproto_rule_try_ref(). If the reference fails (the rule is being/has been released), treat it as a table miss path, avoiding referencing "dead" rules and causing assertion errors. [1] call stack: *0 raise () from /lib64/libc.so.6 *1 abort () from /lib64/libc.so.6 *2 ovs_abort_valist () at lib/util.c:499 *3 vlog_abort_valist () at lib/vlog.c:1249 *4 vlog_abort () at lib/vlog.c:1263 *5 ovs_assert_failure () at lib/util.c:86 *6 ovs_refcount_ref () at lib/ovs-atomic.h:547 *7 ovs_refcount_ref () at ofproto/ofproto.c:3010 *8 ofproto_rule_ref () at ofproto/ofproto.c:3012 *9 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4506 *10 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 *11 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 *12 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 *13 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7183 *14 clone_xlate_actions () at ofproto/ofproto-dpif-xlate.c:5809 *15 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4382 *16 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4511 *17 patch_port_output () at ofproto/ofproto-dpif-xlate.c:3889 *18 compose_output_action__ () at ofproto/ofproto-dpif-xlate.c:4204 *19 compose_output_action () at ofproto/ofproto-dpif-xlate.c:4359 *20 xlate_output_action () at ofproto/ofproto-dpif-xlate.c:5304 *21 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:7033 *22 xlate_actions () at ofproto/ofproto-dpif-xlate.c:8001 *23 xlate_key () at ofproto/ofproto-dpif-upcall.c:2321 *24 xlate_ukey () at ofproto/ofproto-dpif-upcall.c:2336 *25 revalidate_ukey__ () at ofproto/ofproto-dpif-upcall.c:2382 *26 revalidate_ukey () at ofproto/ofproto-dpif-upcall.c:2491 *27 revalidate () at ofproto/ofproto-dpif-upcall.c:2945 *28 udpif_revalidator () at ofproto/ofproto-dpif-upcall.c:1089 *29 ovsthread_wrapper () at lib/ovs-thread.c:422 *30 start_thread () from /lib64/libpthread.so.0 *31 clone () from /lib64/libc.so.6 Signed-off-by: LIU Yulong <i@liuyulong.me> --- V4: - Address ci and clang-analyze failure V3: - Addressed Addressed Ilya's and Adrián's comments: - release the reference unconditionally - Addressed Adrián's comments: - make code consistent in xlate_actions() V2: - Addressed Eelco's comments: - release the reference after ofproto_rule_try_ref() - Addressed Ilya's comments: - remove the commit message function args and mem addresses --- ofproto/ofproto-dpif-xlate.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)