* [PATCH for-4.15 0/3] tools/oxenstored bugfixes
@ 2021-02-03 17:35 Andrew Cooper
2021-02-03 17:35 ` [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST Andrew Cooper
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Andrew Cooper @ 2021-02-03 17:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Christian Lindig, Ian Jackson, Wei Liu
All of these been posted before, but were tangled in other content which is
not appropriate for 4.15 any more. As a consequence, I didn't get around to
committing them before the code freeze.
They were all found with unit testing, specifically fuzzing the
serialising/deserialising logic introduced for restartiblity, asserting that
the tree before and after was identical.
The unit testing/fuzzing content isn't suitable for 4.15, but these bugfixes
want backporting to all releases, and should therefore be considered for 4.15
at this point.
Edwin Török (3):
tools/oxenstored: Fix quota calculation for mkdir EEXIST
tools/oxenstored: Reject invalid watch paths early
tools/oxenstored: mkdir conflicts were sometimes missed
tools/ocaml/xenstored/connection.ml | 5 ++---
tools/ocaml/xenstored/connections.ml | 4 +++-
tools/ocaml/xenstored/store.ml | 1 +
tools/ocaml/xenstored/transaction.ml | 2 +-
4 files changed, 7 insertions(+), 5 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
@ 2021-02-03 17:35 ` Andrew Cooper
2021-02-03 17:40 ` Ian Jackson
2021-02-03 17:35 ` [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early Andrew Cooper
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2021-02-03 17:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Edwin Török, Christian Lindig, Ian Jackson, Wei Liu
From: Edwin Török <edvin.torok@citrix.com>
We increment the domain's quota on mkdir even when the node already exists.
This results in a quota inconsistency after live update, where reconstructing
the tree from scratch results in a different quota.
Not a security issue because the domain uses up quota faster, so it will only
get a Quota error sooner than it should.
Found by the structured fuzzer.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
tools/ocaml/xenstored/store.ml | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml
index 1bd0c81f6f..20e67b1427 100644
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -419,6 +419,7 @@ let mkdir store perm path =
(* It's upt to the mkdir logic to decide what to do with existing path *)
if not (existing || (Perms.Connection.is_dom0 perm)) then Quota.check store.quota owner 0;
store.root <- path_mkdir store perm path;
+ if not existing then
Quota.add_entry store.quota owner
let rm store perm path =
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
2021-02-03 17:35 ` [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST Andrew Cooper
@ 2021-02-03 17:35 ` Andrew Cooper
2021-02-04 10:51 ` Christian Lindig
2021-02-03 17:35 ` [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed Andrew Cooper
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2021-02-03 17:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Edwin Török, Christian Lindig, Ian Jackson, Wei Liu
From: Edwin Török <edvin.torok@citrix.com>
Watches on invalid paths were accepted, but they would never trigger. The
client also got no notification that its watch is bad and would never trigger.
Found again by the structured fuzzer, due to an error on live update reload:
the invalid watch paths would get rejected during live update and the list of
watches would be different pre/post live update.
The testcase is watch on `//`, which is an invalid path.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
tools/ocaml/xenstored/connection.ml | 5 ++---
tools/ocaml/xenstored/connections.ml | 4 +++-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index d09a0fa405..65f99ea6f2 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -158,18 +158,17 @@ let get_children_watches con path =
let is_dom0 con =
Perms.Connection.is_dom0 (get_perm con)
-let add_watch con path token =
+let add_watch con (path, apath) token =
if !Quota.activate && !Define.maxwatch > 0 &&
not (is_dom0 con) && con.nb_watches > !Define.maxwatch then
raise Quota.Limit_reached;
- let apath = get_watch_path con path in
let l = get_watches con apath in
if List.exists (fun w -> w.token = token) l then
raise Define.Already_exist;
let watch = watch_create ~con ~token ~path in
Hashtbl.replace con.watches apath (watch :: l);
con.nb_watches <- con.nb_watches + 1;
- apath, watch
+ watch
let del_watch con path token =
let apath = get_watch_path con path in
diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index 8a66eeec3a..3c7429fe7f 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -114,8 +114,10 @@ let key_of_path path =
"" :: Store.Path.to_string_list path
let add_watch cons con path token =
- let apath, watch = Connection.add_watch con path token in
+ let apath = Connection.get_watch_path con path in
+ (* fail on invalid paths early by calling key_of_str before adding watch *)
let key = key_of_str apath in
+ let watch = Connection.add_watch con (path, apath) token in
let watches =
if Trie.mem cons.watches key
then Trie.find cons.watches key
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
2021-02-03 17:35 ` [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST Andrew Cooper
2021-02-03 17:35 ` [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early Andrew Cooper
@ 2021-02-03 17:35 ` Andrew Cooper
2021-02-04 10:52 ` Christian Lindig
2021-02-03 17:41 ` [PATCH for-4.15 0/3] tools/oxenstored bugfixes Ian Jackson
2021-02-04 10:46 ` Christian Lindig
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2021-02-03 17:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Edwin Török, Christian Lindig, Ian Jackson, Wei Liu
From: Edwin Török <edvin.torok@citrix.com>
Due to how set_write_lowpath was used here it didn't detect create/delete
conflicts. When we create an entry we must mark our parent as modified
(this is what creating a new node via write does).
Otherwise we can have 2 transactions one creating, and another deleting a node
both succeeding depending on timing. Or one transaction reading an entry,
concluding it doesn't exist, do some other work based on that information and
successfully commit even if another transaction creates the node via mkdir
meanwhile.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
tools/ocaml/xenstored/transaction.ml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/ocaml/xenstored/transaction.ml b/tools/ocaml/xenstored/transaction.ml
index 25bc8c3b4a..17b1bdf2ea 100644
--- a/tools/ocaml/xenstored/transaction.ml
+++ b/tools/ocaml/xenstored/transaction.ml
@@ -165,7 +165,7 @@ let write t perm path value =
let mkdir ?(with_watch=true) t perm path =
Store.mkdir t.store perm path;
- set_write_lowpath t path;
+ set_write_lowpath t (Store.Path.get_parent path);
if with_watch then
add_wop t Xenbus.Xb.Op.Mkdir path
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST
2021-02-03 17:35 ` [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST Andrew Cooper
@ 2021-02-03 17:40 ` Ian Jackson
0 siblings, 0 replies; 9+ messages in thread
From: Ian Jackson @ 2021-02-03 17:40 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Edwin Török, Christian Lindig, Wei Liu
Andrew Cooper writes ("[PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST"):
> From: Edwin Török <edvin.torok@citrix.com>
>
> We increment the domain's quota on mkdir even when the node already exists.
> This results in a quota inconsistency after live update, where reconstructing
> the tree from scratch results in a different quota.
>
> Not a security issue because the domain uses up quota faster, so it will only
> get a Quota error sooner than it should.
>
> Found by the structured fuzzer.
Thanks for these. They look like straightforward bugfixes, so they
don't need a release ack, but FTR
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
I don't feel qualified to give a maintainer-ack...
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.15 0/3] tools/oxenstored bugfixes
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
` (2 preceding siblings ...)
2021-02-03 17:35 ` [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed Andrew Cooper
@ 2021-02-03 17:41 ` Ian Jackson
2021-02-04 10:46 ` Christian Lindig
4 siblings, 0 replies; 9+ messages in thread
From: Ian Jackson @ 2021-02-03 17:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Christian Lindig, Wei Liu
Andrew Cooper writes ("[PATCH for-4.15 0/3] tools/oxenstored bugfixes"):
> All of these been posted before, but were tangled in other content which is
> not appropriate for 4.15 any more. As a consequence, I didn't get around to
> committing them before the code freeze.
>
> They were all found with unit testing, specifically fuzzing the
> serialising/deserialising logic introduced for restartiblity, asserting that
> the tree before and after was identical.
>
> The unit testing/fuzzing content isn't suitable for 4.15, but these bugfixes
> want backporting to all releases, and should therefore be considered for 4.15
> at this point.
I just gave my
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
in that other mail. FTAOD that applies to all three.
Christian, would you be able to do a maintainer review ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.15 0/3] tools/oxenstored bugfixes
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
` (3 preceding siblings ...)
2021-02-03 17:41 ` [PATCH for-4.15 0/3] tools/oxenstored bugfixes Ian Jackson
@ 2021-02-04 10:46 ` Christian Lindig
4 siblings, 0 replies; 9+ messages in thread
From: Christian Lindig @ 2021-02-04 10:46 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel, Edwin Torok; +Cc: Ian Jackson, Wei Liu
Acked-by: Christian Lindig <christian.lindig@citrix.com>
________________________________________
From: Andrew Cooper <andrew.cooper3@citrix.com>
Sent: 03 February 2021 17:35
To: Xen-devel
Cc: Andrew Cooper; Christian Lindig; Ian Jackson; Wei Liu
Subject: [PATCH for-4.15 0/3] tools/oxenstored bugfixes
All of these been posted before, but were tangled in other content which is
not appropriate for 4.15 any more. As a consequence, I didn't get around to
committing them before the code freeze.
They were all found with unit testing, specifically fuzzing the
serialising/deserialising logic introduced for restartiblity, asserting that
the tree before and after was identical.
The unit testing/fuzzing content isn't suitable for 4.15, but these bugfixes
want backporting to all releases, and should therefore be considered for 4.15
at this point.
Edwin Török (3):
tools/oxenstored: Fix quota calculation for mkdir EEXIST
tools/oxenstored: Reject invalid watch paths early
tools/oxenstored: mkdir conflicts were sometimes missed
tools/ocaml/xenstored/connection.ml | 5 ++---
tools/ocaml/xenstored/connections.ml | 4 +++-
tools/ocaml/xenstored/store.ml | 1 +
tools/ocaml/xenstored/transaction.ml | 2 +-
4 files changed, 7 insertions(+), 5 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early
2021-02-03 17:35 ` [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early Andrew Cooper
@ 2021-02-04 10:51 ` Christian Lindig
0 siblings, 0 replies; 9+ messages in thread
From: Christian Lindig @ 2021-02-04 10:51 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Edwin Torok, Ian Jackson, Wei Liu
Acked-by: Christian Lindig <christian.lindig@citrix.com>
Great work. Fuzzing is often thought as best to find bugs in languages like C where memory is explicitly managed but here it reveals logical bugs.
________________________________________
From: Andrew Cooper <andrew.cooper3@citrix.com>
Sent: 03 February 2021 17:35
To: Xen-devel
Cc: Edwin Torok; Christian Lindig; Ian Jackson; Wei Liu
Subject: [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early
From: Edwin Török <edvin.torok@citrix.com>
Watches on invalid paths were accepted, but they would never trigger. The
client also got no notification that its watch is bad and would never trigger.
Found again by the structured fuzzer, due to an error on live update reload:
the invalid watch paths would get rejected during live update and the list of
watches would be different pre/post live update.
The testcase is watch on `//`, which is an invalid path.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
tools/ocaml/xenstored/connection.ml | 5 ++---
tools/ocaml/xenstored/connections.ml | 4 +++-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index d09a0fa405..65f99ea6f2 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -158,18 +158,17 @@ let get_children_watches con path =
let is_dom0 con =
Perms.Connection.is_dom0 (get_perm con)
-let add_watch con path token =
+let add_watch con (path, apath) token =
if !Quota.activate && !Define.maxwatch > 0 &&
not (is_dom0 con) && con.nb_watches > !Define.maxwatch then
raise Quota.Limit_reached;
- let apath = get_watch_path con path in
let l = get_watches con apath in
if List.exists (fun w -> w.token = token) l then
raise Define.Already_exist;
let watch = watch_create ~con ~token ~path in
Hashtbl.replace con.watches apath (watch :: l);
con.nb_watches <- con.nb_watches + 1;
- apath, watch
+ watch
let del_watch con path token =
let apath = get_watch_path con path in
diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml
index 8a66eeec3a..3c7429fe7f 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -114,8 +114,10 @@ let key_of_path path =
"" :: Store.Path.to_string_list path
let add_watch cons con path token =
- let apath, watch = Connection.add_watch con path token in
+ let apath = Connection.get_watch_path con path in
+ (* fail on invalid paths early by calling key_of_str before adding watch *)
let key = key_of_str apath in
+ let watch = Connection.add_watch con (path, apath) token in
let watches =
if Trie.mem cons.watches key
then Trie.find cons.watches key
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed
2021-02-03 17:35 ` [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed Andrew Cooper
@ 2021-02-04 10:52 ` Christian Lindig
0 siblings, 0 replies; 9+ messages in thread
From: Christian Lindig @ 2021-02-04 10:52 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Edwin Torok, Ian Jackson, Wei Liu
Acked-by: Christian Lindig <christian.lindig@citrix.com>
________________________________________
From: Andrew Cooper <andrew.cooper3@citrix.com>
Sent: 03 February 2021 17:35
To: Xen-devel
Cc: Edwin Torok; Christian Lindig; Ian Jackson; Wei Liu
Subject: [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed
From: Edwin Török <edvin.torok@citrix.com>
Due to how set_write_lowpath was used here it didn't detect create/delete
conflicts. When we create an entry we must mark our parent as modified
(this is what creating a new node via write does).
Otherwise we can have 2 transactions one creating, and another deleting a node
both succeeding depending on timing. Or one transaction reading an entry,
concluding it doesn't exist, do some other work based on that information and
successfully commit even if another transaction creates the node via mkdir
meanwhile.
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
tools/ocaml/xenstored/transaction.ml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/ocaml/xenstored/transaction.ml b/tools/ocaml/xenstored/transaction.ml
index 25bc8c3b4a..17b1bdf2ea 100644
--- a/tools/ocaml/xenstored/transaction.ml
+++ b/tools/ocaml/xenstored/transaction.ml
@@ -165,7 +165,7 @@ let write t perm path value =
let mkdir ?(with_watch=true) t perm path =
Store.mkdir t.store perm path;
- set_write_lowpath t path;
+ set_write_lowpath t (Store.Path.get_parent path);
if with_watch then
add_wop t Xenbus.Xb.Op.Mkdir path
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-04 10:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 17:35 [PATCH for-4.15 0/3] tools/oxenstored bugfixes Andrew Cooper
2021-02-03 17:35 ` [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST Andrew Cooper
2021-02-03 17:40 ` Ian Jackson
2021-02-03 17:35 ` [PATCH 2/3] tools/oxenstored: Reject invalid watch paths early Andrew Cooper
2021-02-04 10:51 ` Christian Lindig
2021-02-03 17:35 ` [PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed Andrew Cooper
2021-02-04 10:52 ` Christian Lindig
2021-02-03 17:41 ` [PATCH for-4.15 0/3] tools/oxenstored bugfixes Ian Jackson
2021-02-04 10:46 ` Christian Lindig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.