Re: [ovs-dev] [ovs-dev v3] ofproto: add refcount to ofproto to fix ofproto use-after-free
Ah,
found a bug, will submit a v4.
Peng He 于2022年1月18日周二 21:08写道：
> From hepeng:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20200717015041.82746-1-hepeng.0...@bytedance.com/#2487473
>
> also from guohongzhi :
>
> http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.19884-1-guohongz...@huawei.com/
>
> also from a discussion about the mixing use of RCU and refcount in the mail
> list with Ilya Maximets, William Tu, Ben Pfaf, and Gaëtan Rivet.
>
> A summary, as quoted from Ilya:
>
> "
> RCU for ofproto was introduced for one
> and only one reason - to avoid freeing ofproto while rules are still
> alive. This was done in commit f416c8d61601 ("ofproto: RCU postpone
> rule destruction."). The goal was to allow using rules without
> refcounting them within a single grace period. And that forced us
> to postpone destruction of the ofproto for a single grace period.
> Later commit 39c9459355b6 ("Use classifier versioning.") made it
> possible for rules to be alive for more than one grace period, so
> the commit made ofproto wait for 2 grace periods by double postponing.
> As we can see now, that wasn't enough and we have to wait for more
> than 2 grace periods in certain cases.
> "
>
> In a short, the ofproto should have a longer life time than rule, if
> the rule lasts for more than 2 grace periods, the ofproto should live
> longer to ensure rule->ofproto is valid. It's hard to predict how long
> a ofproto should live, thus we need to use refcount on ofproto to make
> things easy. The controversial part is that we have already used RCU
> postpone
> to delay ofproto destrution, if we have to add refcount, is it simpler to
> use just refcount without RCU postpone?
>
> IMO, I think going back to the pure refcount solution is more
> complicated than mixing using both.
>
> Gaëtan Rive asks some questions on guohongzhi's v2 patch:
>
> during ofproto_rule_create, should we use ofproto_ref
> or ofproto_try_ref? how can we make sure the ofproto is alive?
>
> by using RCU, I think the answer is that, when you see a pointer to
> ofproto, it's alive at least in this grace peroid, so it is ok to use
> ofproto_ref instead of ofproto_try_ref. This shows that by mixing use
> of RCU and refcount we can save a lot of work to query if ofproto is
> alive.
>
> In short, the RCU part makes sure the ofproto is alive when we use it,
> and the refcount part makes sure it lives longer enough.
>
> Also regarding a new patch filed recently, people are now making use
> of RCU to protect ofproto:
>
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/1638530715-44436-1-git-send-email-wangyunj...@huawei.com/
>
> In this patch, I have merged guohongzhi's patch and mine, and fixes
> accoring to the previous comments.
>
> Signed-off-by: Peng He
> Signed-off-by: guohongzhi
> ---
> ofproto/ofproto-dpif-xlate-cache.c | 1 +
> ofproto/ofproto-dpif-xlate.c | 1 +
> ofproto/ofproto-dpif.c | 4 +--
> ofproto/ofproto-provider.h | 2 ++
> ofproto/ofproto.c | 45 ++
> ofproto/ofproto.h | 3 ++
> 6 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate-cache.c
> b/ofproto/ofproto-dpif-xlate-cache.c
> index dcc91cb38..0deee365d 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.c
> +++ b/ofproto/ofproto-dpif-xlate-cache.c
> @@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> {
> switch (entry->type) {
> case XC_TABLE:
> +ofproto_unref(&(entry->table.ofproto->up));
> break;
> case XC_RULE:
> ofproto_rule_unref(>rule->up);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 6fb59e170..0380928a9 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3025,6 +3025,7 @@ xlate_normal(struct xlate_ctx *ctx)
>
> /* Save just enough info to update mac learning table later. */
> entry = xlate_cache_add_entry(ctx->xin->xcache, XC_NORMAL);
> +ofproto_ref(>xbridge->ofproto->up);
> entry->normal.ofproto = ctx->xbridge->ofproto;
> entry->normal.in_port = flow->in_port.ofp_port;
> entry->normal.dl_src = flow->dl_src;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 8143dd965..6816896dd 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4472,8 +4472,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
> }
> if (xcache) {
> struct xc_entry *entry;
> -
> entry = xlate_cache_add_entry(xcache, XC_TABLE);
> +ofproto_ref(>up);
> entry->table.ofproto = ofproto;
> entry->table.id = *table_id;
> entry->table.match = true;
> @@ -4508,8 +4508,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
> }
> if (xcache) {
>
Re: [ovs-dev] [ovs-dev v3] ofproto: add refcount to ofproto to fix ofproto use-after-free
Bleep bloop. Greetings Peng He, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Peng He needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He , guohongzhi Lines checked: 258, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ovs-dev v3] ofproto: add refcount to ofproto to fix ofproto use-after-free
From hepeng:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200717015041.82746-1-hepeng.0...@bytedance.com/#2487473
also from guohongzhi :
http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.19884-1-guohongz...@huawei.com/
also from a discussion about the mixing use of RCU and refcount in the mail
list with Ilya Maximets, William Tu, Ben Pfaf, and Gaëtan Rivet.
A summary, as quoted from Ilya:
"
RCU for ofproto was introduced for one
and only one reason - to avoid freeing ofproto while rules are still
alive. This was done in commit f416c8d61601 ("ofproto: RCU postpone
rule destruction."). The goal was to allow using rules without
refcounting them within a single grace period. And that forced us
to postpone destruction of the ofproto for a single grace period.
Later commit 39c9459355b6 ("Use classifier versioning.") made it
possible for rules to be alive for more than one grace period, so
the commit made ofproto wait for 2 grace periods by double postponing.
As we can see now, that wasn't enough and we have to wait for more
than 2 grace periods in certain cases.
"
In a short, the ofproto should have a longer life time than rule, if
the rule lasts for more than 2 grace periods, the ofproto should live
longer to ensure rule->ofproto is valid. It's hard to predict how long
a ofproto should live, thus we need to use refcount on ofproto to make
things easy. The controversial part is that we have already used RCU postpone
to delay ofproto destrution, if we have to add refcount, is it simpler to
use just refcount without RCU postpone?
IMO, I think going back to the pure refcount solution is more
complicated than mixing using both.
Gaëtan Rive asks some questions on guohongzhi's v2 patch:
during ofproto_rule_create, should we use ofproto_ref
or ofproto_try_ref? how can we make sure the ofproto is alive?
by using RCU, I think the answer is that, when you see a pointer to
ofproto, it's alive at least in this grace peroid, so it is ok to use
ofproto_ref instead of ofproto_try_ref. This shows that by mixing use
of RCU and refcount we can save a lot of work to query if ofproto is
alive.
In short, the RCU part makes sure the ofproto is alive when we use it,
and the refcount part makes sure it lives longer enough.
Also regarding a new patch filed recently, people are now making use
of RCU to protect ofproto:
https://patchwork.ozlabs.org/project/openvswitch/patch/1638530715-44436-1-git-send-email-wangyunj...@huawei.com/
In this patch, I have merged guohongzhi's patch and mine, and fixes
accoring to the previous comments.
Signed-off-by: Peng He
Signed-off-by: guohongzhi
---
ofproto/ofproto-dpif-xlate-cache.c | 1 +
ofproto/ofproto-dpif-xlate.c | 1 +
ofproto/ofproto-dpif.c | 4 +--
ofproto/ofproto-provider.h | 2 ++
ofproto/ofproto.c | 45 ++
ofproto/ofproto.h | 3 ++
6 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate-cache.c
b/ofproto/ofproto-dpif-xlate-cache.c
index dcc91cb38..0deee365d 100644
--- a/ofproto/ofproto-dpif-xlate-cache.c
+++ b/ofproto/ofproto-dpif-xlate-cache.c
@@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
{
switch (entry->type) {
case XC_TABLE:
+ofproto_unref(&(entry->table.ofproto->up));
break;
case XC_RULE:
ofproto_rule_unref(>rule->up);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 6fb59e170..0380928a9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3025,6 +3025,7 @@ xlate_normal(struct xlate_ctx *ctx)
/* Save just enough info to update mac learning table later. */
entry = xlate_cache_add_entry(ctx->xin->xcache, XC_NORMAL);
+ofproto_ref(>xbridge->ofproto->up);
entry->normal.ofproto = ctx->xbridge->ofproto;
entry->normal.in_port = flow->in_port.ofp_port;
entry->normal.dl_src = flow->dl_src;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8143dd965..6816896dd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4472,8 +4472,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
}
if (xcache) {
struct xc_entry *entry;
-
entry = xlate_cache_add_entry(xcache, XC_TABLE);
+ofproto_ref(>up);
entry->table.ofproto = ofproto;
entry->table.id = *table_id;
entry->table.match = true;
@@ -4508,8 +4508,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
}
if (xcache) {
struct xc_entry *entry;
-
entry = xlate_cache_add_entry(xcache, XC_TABLE);
+ofproto_ref(>up);
entry->table.ofproto = ofproto;
entry->table.id = next_id;
entry->table.match = (rule != NULL);
diff --git
