[v4,01/11] tools/ocaml/libs/{xb, mmap}: use Data_abstract_val wrapper

Commit Message

Edwin Török Dec. 16, 2022, 6:25 p.m. UTC

This is not strictly necessary since it is essentially a no-op
currently: a cast to void* and value*, even in OCaml 5.0.

However it does make it clearer that what we have here is not a regular
OCaml value, but one allocated with Abstract_tag or Custom_tag,
and follows the example from the manual more closely:
https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head

It also makes it clearer that these modules have been reviewed for
compat with OCaml 5.0.

We cannot use OCaml finalizers here, because we want control over when to unmap
these pages from remote domains.
A follow-up commit will add use-after-free detection instead.

No functional change.

Signed-off-by: Edwin Török <[e-mail]>
Acked-by: Christian Lindig <[e-mail]>
---
Reason for inclusion in 4.17:
- make code follow best practice for upcoming OCaml 5.0 compiler (already in beta)

Changes since v2:
- add Acked-by line

Changes since v3:
- mention that use-after-free is fixed in another commit, and we cannot use
  finalizers here
---
 tools/ocaml/libs/mmap/mmap_stubs.h    | 4 ++++
 tools/ocaml/libs/mmap/xenmmap_stubs.c | 2 +-
 tools/ocaml/libs/xb/xs_ring_stubs.c   | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

Comments

Andrew Cooper Dec. 16, 2022, 10:40 p.m. UTC | #1

On 16/12/2022 6:25 pm, Edwin Török wrote:
> diff --git a/tools/ocaml/libs/mmap/mmap_stubs.h b/tools/ocaml/libs/mmap/mmap_stubs.h
> index 65e4239890..66f18d4406 100644
> --- a/tools/ocaml/libs/mmap/mmap_stubs.h
> +++ b/tools/ocaml/libs/mmap/mmap_stubs.h
> @@ -30,4 +30,8 @@ struct mmap_interface
>  	int len;
>  };
>  
> +#ifndef Data_abstract_val
> +#define Data_abstract_val(x) ((void*)(value*)(x))

((void *)(x))

I take it this has come from the Ocaml headers?  The cast to (value *)
in the middle is entirely cancelled out.

Can be fixed on commit.

~Andrew

Edwin Török Dec. 19, 2022, 9:06 a.m. UTC | #2

> On 16 Dec 2022, at 22:40, Andrew Cooper <[e-mail]> wrote:
> 
> On 16/12/2022 6:25 pm, Edwin Török wrote:
>> diff --git a/tools/ocaml/libs/mmap/mmap_stubs.h b/tools/ocaml/libs/mmap/mmap_stubs.h
>> index 65e4239890..66f18d4406 100644
>> --- a/tools/ocaml/libs/mmap/mmap_stubs.h
>> +++ b/tools/ocaml/libs/mmap/mmap_stubs.h
>> @@ -30,4 +30,8 @@ struct mmap_interface
>> int len;
>> };
>> 
>> +#ifndef Data_abstract_val
>> +#define Data_abstract_val(x) ((void*)(value*)(x))
> 
> ((void *)(x))
> 
> I take it this has come from the Ocaml headers?  The cast to (value *)
> in the middle is entirely cancelled out.
> 
> Can be fixed on commit.
> 


In the OCaml headers it looks like:
+#define Data_abstract_val(v) ((void*) Op_val(v))

where Op_val is:
+#define Op_val(x) ((value *) (x))

However I haven't realized that Op_val has been in OCaml for a long time (at least 1995), so we can safely use it, and do this instead:

diff --git a/tools/ocaml/libs/mmap/mmap_stubs.h b/tools/ocaml/libs/mmap/mmap_stubs.h
index 65e4239890..6c33f14138 100644
--- a/tools/ocaml/libs/mmap/mmap_stubs.h
+++ b/tools/ocaml/libs/mmap/mmap_stubs.h
@@ -30,4 +30,8 @@ struct mmap_interface
    int len;
 };

+#ifndef Data_abstract_val
+#define Data_abstract_val(v) ((void*) Op_val(v))
+#endif
+
 #endif


There is an updated commit here for convenience: https://github.com/edwintorok/xen/commit/9855ed237f3f85ac30972bfd0a601c6746ba2353

Best regards,
--Edwin

Patch

diff --git a/tools/ocaml/libs/mmap/mmap_stubs.h b/tools/ocaml/libs/mmap/mmap_stubs.h
index 65e4239890..66f18d4406 100644
--- a/tools/ocaml/libs/mmap/mmap_stubs.h
+++ b/tools/ocaml/libs/mmap/mmap_stubs.h
@@ -30,4 +30,8 @@  struct mmap_interface
 	int len;
 };
 
+#ifndef Data_abstract_val
+#define Data_abstract_val(x) ((void*)(value*)(x))
+#endif
+
 #endif
diff --git a/tools/ocaml/libs/mmap/xenmmap_stubs.c b/tools/ocaml/libs/mmap/xenmmap_stubs.c
index e2ce088e25..141dedb78c 100644
--- a/tools/ocaml/libs/mmap/xenmmap_stubs.c
+++ b/tools/ocaml/libs/mmap/xenmmap_stubs.c
@@ -28,7 +28,7 @@ 
 #include <caml/fail.h>
 #include <caml/callback.h>
 
-#define Intf_val(a) ((struct mmap_interface *) a)
+#define Intf_val(a) ((struct mmap_interface *) Data_abstract_val(a))
 
 static int mmap_interface_init(struct mmap_interface *intf,
                                int fd, int pflag, int mflag,
diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index 7a91fdee75..cc9114029f 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -35,7 +35,7 @@ 
 #include <sys/mman.h>
 #include "mmap_stubs.h"
 
-#define GET_C_STRUCT(a) ((struct mmap_interface *) a)
+#define GET_C_STRUCT(a) ((struct mmap_interface *) Data_abstract_val(a))
 
 /*
  * Bytes_val has been introduced by Ocaml 4.06.1. So define our own version