| Message ID | 20210527102914.27175-1-jianbol@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-upcall: Fix race condition while purging | expand |
On 5/27/21 12:29 PM, Jianbo Liu wrote: > There is a race condidtion between purger and handler in dpif-netlink. > Handler may create new ukey and install it while executing 'ovs-appctl > revalidator/purge' command. However, before handler calls > transition_ukey() in handle_upcalls(), purger may get this ukey from > umap, then evict and delete it. This will trigger ovs_abort in > transition_ukey() for handler because it is trying to set state to > EVICTED or OPERATIONAL, but ukey is already in DELETED state. > To fix this issue, purger must not delete ukey in VISIBLE state. Hi. This is not a good thing to trigger abort(), but "purge" means "purge". And, AFAIU, most of ukeys are visible. This is a purely debug interface that was introduced to test functionality of OVS and should not be used in production environment. The fact that "purge" doesn't remove visible ukeys, in my opinion, just makes the appctl "revalidator/purge" useless. Can we replace abort() with rate-limited error message for this scenario instead to avoid process termination? BTW, how did you catch this? Is it reproducible with system tests? Best regards, Ilya Maximets.
The 05/27/2021 12:43, Ilya Maximets wrote: > On 5/27/21 12:29 PM, Jianbo Liu wrote: > > There is a race condidtion between purger and handler in dpif-netlink. > > Handler may create new ukey and install it while executing 'ovs-appctl > > revalidator/purge' command. However, before handler calls > > transition_ukey() in handle_upcalls(), purger may get this ukey from > > umap, then evict and delete it. This will trigger ovs_abort in > > transition_ukey() for handler because it is trying to set state to > > EVICTED or OPERATIONAL, but ukey is already in DELETED state. > > To fix this issue, purger must not delete ukey in VISIBLE state. > > Hi. This is not a good thing to trigger abort(), but "purge" means > "purge". And, AFAIU, most of ukeys are visible. This is a purely > debug interface that was introduced to test functionality of OVS and > should not be used in production environment. The fact that "purge" > doesn't remove visible ukeys, in my opinion, just makes the appctl > "revalidator/purge" useless. Can we replace abort() with rate-limited But currently ukey is also not removed if we can't hold its lock. Besides, new ukey will be installed while purging. We can't make sure that no visilbe/operational ukeys exist at the monent this command is finished. So in order to resolve the race, visible ukey should be kept, it is just like a new incoming ukey. > error message for this scenario instead to avoid process termination? No sure if there are issues, because ukey is deleted (maybe freed) while handler still access it by the pointer. > > BTW, how did you catch this? Is it reproducible with system tests? We found this issue when testing CT offload, do purge without stop traffic. Thanks! Jianbo > > Best regards, Ilya Maximets.
On 5/28/21 5:16 AM, Jianbo Liu wrote: > The 05/27/2021 12:43, Ilya Maximets wrote: >> On 5/27/21 12:29 PM, Jianbo Liu wrote: >>> There is a race condidtion between purger and handler in dpif-netlink. >>> Handler may create new ukey and install it while executing 'ovs-appctl >>> revalidator/purge' command. However, before handler calls >>> transition_ukey() in handle_upcalls(), purger may get this ukey from >>> umap, then evict and delete it. This will trigger ovs_abort in >>> transition_ukey() for handler because it is trying to set state to >>> EVICTED or OPERATIONAL, but ukey is already in DELETED state. >>> To fix this issue, purger must not delete ukey in VISIBLE state. >> >> Hi. This is not a good thing to trigger abort(), but "purge" means >> "purge". And, AFAIU, most of ukeys are visible. This is a purely >> debug interface that was introduced to test functionality of OVS and >> should not be used in production environment. The fact that "purge" >> doesn't remove visible ukeys, in my opinion, just makes the appctl >> "revalidator/purge" useless. Can we replace abort() with rate-limited > > But currently ukey is also not removed if we can't hold its lock. > Besides, new ukey will be installed while purging. We can't make sure > that no visilbe/operational ukeys exist at the monent this command is > finished. So in order to resolve the race, visible ukey should be kept, > it is just like a new incoming ukey. The purpose of this call is to be used in the testsuite where we have a full control over traffic that flows through OVS ports, so locking issue or installation of a new ukey is not possible there. > >> error message for this scenario instead to avoid process termination? > > No sure if there are issues, because ukey is deleted (maybe freed) while > handler still access it by the pointer. Well, if there is a race that could lead to a crash, it make sense fixing, but it doesn't make sense to fix only for one datapath type. If it's not possible to fix keeping the semantics of the call, I'd prefer to not fix that at all as we need this appctl for unit testing, other use cases are not supported. i.e. there is no point to change this call if the fix beaks its main functionality. >> >> BTW, how did you catch this? Is it reproducible with system tests? > > We found this issue when testing CT offload, do purge without stop > traffic. What is the point of calling 'purge' in this scenario if it will not purge visible ukeys? > > Thanks! > Jianbo > >> >> Best regards, Ilya Maximets. >
The 05/31/2021 19:58, Ilya Maximets wrote: > On 5/28/21 5:16 AM, Jianbo Liu wrote: > > The 05/27/2021 12:43, Ilya Maximets wrote: > >> On 5/27/21 12:29 PM, Jianbo Liu wrote: > >>> There is a race condidtion between purger and handler in dpif-netlink. > >>> Handler may create new ukey and install it while executing 'ovs-appctl > >>> revalidator/purge' command. However, before handler calls > >>> transition_ukey() in handle_upcalls(), purger may get this ukey from > >>> umap, then evict and delete it. This will trigger ovs_abort in > >>> transition_ukey() for handler because it is trying to set state to > >>> EVICTED or OPERATIONAL, but ukey is already in DELETED state. > >>> To fix this issue, purger must not delete ukey in VISIBLE state. > >> > >> Hi. This is not a good thing to trigger abort(), but "purge" means > >> "purge". And, AFAIU, most of ukeys are visible. This is a purely > >> debug interface that was introduced to test functionality of OVS and > >> should not be used in production environment. The fact that "purge" > >> doesn't remove visible ukeys, in my opinion, just makes the appctl > >> "revalidator/purge" useless. Can we replace abort() with rate-limited > > > > But currently ukey is also not removed if we can't hold its lock. > > Besides, new ukey will be installed while purging. We can't make sure > > that no visilbe/operational ukeys exist at the monent this command is > > finished. So in order to resolve the race, visible ukey should be kept, > > it is just like a new incoming ukey. > > The purpose of this call is to be used in the testsuite where we have > a full control over traffic that flows through OVS ports, so locking > issue or installation of a new ukey is not possible there. > > > > >> error message for this scenario instead to avoid process termination? > > > > No sure if there are issues, because ukey is deleted (maybe freed) while > > handler still access it by the pointer. > > Well, if there is a race that could lead to a crash, it make sense > fixing, but it doesn't make sense to fix only for one datapath type. It is related to handle_upcalls(), and dpif-netlink calls it, but dpif-netdev does not, right? > > If it's not possible to fix keeping the semantics of the call, I'd > prefer to not fix that at all as we need this appctl for unit testing, > other use cases are not supported. i.e. there is no point to change > this call if the fix beaks its main functionality. I don't think it breaks. If it's used only for unit test as you suggested, there should not be VISIBLE ukey while purge for dpif-netlink, so the behavior is still the same. > > >> > >> BTW, how did you catch this? Is it reproducible with system tests? > > > > We found this issue when testing CT offload, do purge without stop > > traffic. > > What is the point of calling 'purge' in this scenario if it will > not purge visible ukeys? Maybe stability, I think. We don't know what command user will run on his system. Whatever he do, we must make sure openvswitch does not crash, right? > > > > > Thanks! > > Jianbo > > > >> > >> Best regards, Ilya Maximets. > > >
Jianbo Liu <jianbol@nvidia.com> 于2021年6月1日周二 10:02写道： > The 05/31/2021 19:58, Ilya Maximets wrote: > > On 5/28/21 5:16 AM, Jianbo Liu wrote: > > > The 05/27/2021 12:43, Ilya Maximets wrote: > > >> On 5/27/21 12:29 PM, Jianbo Liu wrote: > > >>> There is a race condidtion between purger and handler in > dpif-netlink. > > >>> Handler may create new ukey and install it while executing > 'ovs-appctl > > >>> revalidator/purge' command. However, before handler calls > > >>> transition_ukey() in handle_upcalls(), purger may get this ukey from > > >>> umap, then evict and delete it. This will trigger ovs_abort in > > >>> transition_ukey() for handler because it is trying to set state to > > >>> EVICTED or OPERATIONAL, but ukey is already in DELETED state. > > >>> To fix this issue, purger must not delete ukey in VISIBLE state. > > >> > > >> Hi. This is not a good thing to trigger abort(), but "purge" means > > >> "purge". And, AFAIU, most of ukeys are visible. This is a purely > > >> debug interface that was introduced to test functionality of OVS and > > >> should not be used in production environment. The fact that "purge" > > >> doesn't remove visible ukeys, in my opinion, just makes the appctl > > >> "revalidator/purge" useless. Can we replace abort() with rate-limited > > > > > > But currently ukey is also not removed if we can't hold its lock. > > > Besides, new ukey will be installed while purging. We can't make sure > > > that no visilbe/operational ukeys exist at the monent this command is > > > finished. So in order to resolve the race, visible ukey should be kept, > > > it is just like a new incoming ukey. > > > > The purpose of this call is to be used in the testsuite where we have > > a full control over traffic that flows through OVS ports, so locking > > issue or installation of a new ukey is not possible there. > > > > > > > >> error message for this scenario instead to avoid process termination? > > > > > > No sure if there are issues, because ukey is deleted (maybe freed) > while > > > handler still access it by the pointer. > > > > Well, if there is a race that could lead to a crash, it make sense > > fixing, but it doesn't make sense to fix only for one datapath type. > > It is related to handle_upcalls(), and dpif-netlink calls it, but > dpif-netdev does not, right? > even only dpif-netlink calls it, the userspace datapath has the similar code in the *push_dp_ops*, and currently I see a different processing logic here. So I guess this issue still exists in the userspace datapath. Am I right? I think the revalidator/purge command has its usage in the production environments. it provides, perhaps the only way, to fix issues when ukey and megaflows are inconsistent when there is a bug in revalidate code. Am I right? I think maybe we can limit the push_dp_ops to only do the transcation of the ukey state to EVICTED or OPERATIONAL if its state < DELETED, without changing the process of purge. and by the way, it seems that the kernel datapath has now different operations with the userspace datapath, the ukey turns into OPERATIONAL state while in the userspace, ukey is first set in the state of VISIBLE. It will turn into OPERATIONAL right after its megaflow gets dumped. > > > > > If it's not possible to fix keeping the semantics of the call, I'd > > prefer to not fix that at all as we need this appctl for unit testing, > > other use cases are not supported. i.e. there is no point to change > > this call if the fix beaks its main functionality. > > I don't think it breaks. If it's used only for unit test as you > suggested, there should not be VISIBLE ukey while purge for > dpif-netlink, so the behavior is still the same. > > > > > >> > > >> BTW, how did you catch this? Is it reproducible with system tests? > > > > > > We found this issue when testing CT offload, do purge without stop > > > traffic. > > > > What is the point of calling 'purge' in this scenario if it will > > not purge visible ukeys? > > Maybe stability, I think. We don't know what command user will run on > his system. Whatever he do, we must make sure openvswitch does not > crash, right? > > > > > > > > > Thanks! > > > Jianbo > > > > > >> > > >> Best regards, Ilya Maximets. > > > > > > > -- > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, May 27, 2021 at 10:29:14AM +0000, Jianbo Liu wrote: > There is a race condidtion between purger and handler in dpif-netlink. > Handler may create new ukey and install it while executing 'ovs-appctl > revalidator/purge' command. However, before handler calls > transition_ukey() in handle_upcalls(), purger may get this ukey from > umap, then evict and delete it. This will trigger ovs_abort in > transition_ukey() for handler because it is trying to set state to > EVICTED or OPERATIONAL, but ukey is already in DELETED state. > To fix this issue, purger must not delete ukey in VISIBLE state. > > Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.") > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > Reviewed-by: Roi Dayan <roid@nvidia.com> Hi Jianbo Liu, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index ccf97266c..963e8a17a 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -327,6 +327,12 @@ struct ukey_op { struct dpif_op dop; /* Flow operation. */ }; +enum sweep_type { + PURGE_NONE, + PURGE_SOFT, + PURGE_HARD, +}; + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); static struct ovs_list all_udpifs = OVS_LIST_INITIALIZER(&all_udpifs); @@ -345,7 +351,7 @@ static unsigned long udpif_get_n_flows(struct udpif *); static void revalidate(struct revalidator *); static void revalidator_pause(struct revalidator *); static void revalidator_sweep(struct revalidator *); -static void revalidator_purge(struct revalidator *); +static void revalidator_purge(struct revalidator *, enum sweep_type); static void upcall_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[], void *aux); static void upcall_unixctl_disable_megaflows(struct unixctl_conn *, int argc, @@ -541,7 +547,7 @@ udpif_stop_threads(struct udpif *udpif, bool delete_flows) if (delete_flows) { for (i = 0; i < udpif->n_revalidators; i++) { - revalidator_purge(&udpif->revalidators[i]); + revalidator_purge(&udpif->revalidators[i], PURGE_HARD); } } @@ -2772,7 +2778,8 @@ revalidator_pause(struct revalidator *revalidator) } static void -revalidator_sweep__(struct revalidator *revalidator, bool purge) +revalidator_sweep__(struct revalidator *revalidator, + enum sweep_type sweep_type) { struct udpif *udpif; uint64_t dump_seq, reval_seq; @@ -2803,13 +2810,13 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) } ukey_state = ukey->state; if (ukey_state == UKEY_OPERATIONAL - || (ukey_state == UKEY_VISIBLE && purge)) { + || (ukey_state == UKEY_VISIBLE && sweep_type == PURGE_HARD)) { struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; bool seq_mismatch = (ukey->dump_seq != dump_seq && ukey->reval_seq != reval_seq); enum reval_result result; - if (purge) { + if (sweep_type > PURGE_NONE) { result = UKEY_DELETE; } else if (!seq_mismatch) { result = UKEY_KEEP; @@ -2856,13 +2863,13 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) static void revalidator_sweep(struct revalidator *revalidator) { - revalidator_sweep__(revalidator, false); + revalidator_sweep__(revalidator, PURGE_NONE); } static void -revalidator_purge(struct revalidator *revalidator) +revalidator_purge(struct revalidator *revalidator, enum sweep_type sweep_type) { - revalidator_sweep__(revalidator, true); + revalidator_sweep__(revalidator, sweep_type); } /* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */ @@ -3053,10 +3060,17 @@ upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED, struct udpif *udpif; LIST_FOR_EACH (udpif, list_node, &all_udpifs) { + enum sweep_type sweep_type; int n; + if (udpif->dpif->dpif_class == &dpif_netlink_class) { + sweep_type = PURGE_SOFT; + } else { + sweep_type = PURGE_HARD; + } + for (n = 0; n < udpif->n_revalidators; n++) { - revalidator_purge(&udpif->revalidators[n]); + revalidator_purge(&udpif->revalidators[n], sweep_type); } } unixctl_command_reply(conn, "");
There is a race condidtion between purger and handler in dpif-netlink. Handler may create new ukey and install it while executing 'ovs-appctl revalidator/purge' command. However, before handler calls transition_ukey() in handle_upcalls(), purger may get this ukey from umap, then evict and delete it. This will trigger ovs_abort in transition_ukey() for handler because it is trying to set state to EVICTED or OPERATIONAL, but ukey is already in DELETED state. To fix this issue, purger must not delete ukey in VISIBLE state. Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.") Signed-off-by: Jianbo Liu <jianbol@nvidia.com> Reviewed-by: Roi Dayan <roid@nvidia.com> --- v2: Set PURGE_SOFT mode for dpif-netlink only. ofproto/ofproto-dpif-upcall.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)