[PATCH] drop the deprecated malloc/free hooks in hurd/mach-defpager

[Flavio Cruz]

---

mach-defpager: Drop the deprecated malloc/free hooks.

* mach-defpager/kalloc.c: Define malloc and free directly since those are weak
symbols.

diff --git a/mach-defpager/kalloc.c b/mach-defpager/kalloc.c
index ef844ac..35ddf9a 100644
--- a/mach-defpager/kalloc.c
+++ b/mach-defpager/kalloc.c
@@ -34,23 +34,9 @@

#include <mach.h>
#include <pthread.h> /* for spin locks */
-#include <malloc.h> /* for malloc_hook/free_hook */

#include "wiring.h"

-static void init_hook (void);
-static void *malloc_hook (size_t size, const void *caller);
-static void free_hook (void *ptr, const void *caller);
-
-/* GNU libc 2.14 defines this macro to declare hook variables as volatile.
- Define it as empty for older libc versions. */
-#ifndef __MALLOC_HOOK_VOLATILE
-# define __MALLOC_HOOK_VOLATILE
-#endif
-
-void (*__MALLOC_HOOK_VOLATILE __malloc_initialize_hook) (void) = init_hook;
-
-
/* #define DEBUG */

/*
@@ -264,21 +250,14 @@ kfree( void *data,
}
}

-static void
-init_hook (void)
-{
- __malloc_hook = malloc_hook;
- __free_hook = free_hook;
-}
-
-static void *
-malloc_hook (size_t size, const void *caller)
+void *
+malloc (size_t size)
{
return (void *) kalloc ((vm_size_t) size);
}

-static void
-free_hook (void *ptr, const void *caller)
+void
+free (void *ptr)
{
/* Just ignore harmless attempts at cleanliness. */
/* panic("free not implemented"); */

[Samuel Thibault]

Applied, thanks!

[Thomas Schwinge]

Hi!

On Tue, 29 Dec 2015 23:09:54 +0100, Flavio Cruz <flavi...@gmail.com> wrote:
> mach-defpager: Drop the deprecated malloc/free hooks.

(After approval by Samuel, this became commit
8c49801c8f7e3f800cabedf8fca8ccec3cf35a22.)

> * mach-defpager/kalloc.c: Define malloc and free directly since those are weak
> symbols.

The idea here is to just change the mechanism (override glibc's weak
symbols instead of using glibc's deprecated malloc hooks), while not
changing any semantics, right?

From a quick look, I have some doubts about this generally (so, not only
related to your change).

> --- a/mach-defpager/kalloc.c
> +++ b/mach-defpager/kalloc.c

> -static void *
> -malloc_hook (size_t size, const void *caller)
> +void *
> +malloc (size_t size)
> {
> return (void *) kalloc ((vm_size_t) size);
> }
>
> -static void
> -free_hook (void *ptr, const void *caller)
> +void
> +free (void *ptr)
> {
> /* Just ignore harmless attempts at cleanliness. */
> /* panic("free not implemented"); */

So, here we now only provide the malloc and free symbols. What happens
if, for example, calloc, realloc, posix_memalign et al. are called?

For example, looking at [glibc]/malloc/malloc.c:__libc_calloc (weak alias
for calloc). Before your change, it called mach-defpager's malloc_hook,
and then zeroed that region. After your change, it is implemented using
the real glibc calloc -- which actually does change the semantics in an
unintended way?

Backing up one step: what is actually being done here in mach-defpager?
Again from just a quick look, is the idea to use wired memory for all of
mach-defpager's process memory?

So, for example, if now some mach-defpager code calls calloc, it will now
get non-wired "glibc" memory instead of wired "kalloc" memory. The
problem here is not mach-defpager's own code (which we can easily
verify/control), but the problems is the libraries it links against,
which currently are: libihash, libpthread, glibc itself, and any
dependencies these may have. Can we be sure these are not internally
using calloc, for example?

Now, this is not a new problem that you introduced ;-) -- previously we
also didn't provide malloc's memalign and realloc hooks, and realloc is
used quite commonly, I suppose?

If my theories are correct, do we need to use a different mechanism to
get the whole mach-defpager process into wired memory?


Grüße
Thomas

[Justus Winter]

Hey :)

Quoting Thomas Schwinge (2016-05-16 18:49:47)

Uh, we actually don't free memory here...

> So, here we now only provide the malloc and free symbols. What happens
> if, for example, calloc, realloc, posix_memalign et al. are called?
>
> For example, looking at [glibc]/malloc/malloc.c:__libc_calloc (weak alias
> for calloc). Before your change, it called mach-defpager's malloc_hook,
> and then zeroed that region. After your change, it is implemented using
> the real glibc calloc -- which actually does change the semantics in an
> unintended way?
>
> Backing up one step: what is actually being done here in mach-defpager?
> Again from just a quick look, is the idea to use wired memory for all of
> mach-defpager's process memory?
>
> So, for example, if now some mach-defpager code calls calloc, it will now
> get non-wired "glibc" memory instead of wired "kalloc" memory. The
> problem here is not mach-defpager's own code (which we can easily
> verify/control), but the problems is the libraries it links against,
> which currently are: libihash, libpthread, glibc itself, and any

And indeed libihash uses calloc.

> dependencies these may have. Can we be sure these are not internally
> using calloc, for example?
>
> Now, this is not a new problem that you introduced ;-) -- previously we
> also didn't provide malloc's memalign and realloc hooks, and realloc is
> used quite commonly, I suppose?
>
> If my theories are correct, do we need to use a different mechanism to
> get the whole mach-defpager process into wired memory?

As Richard said in #hurd, implement mlockall and MCL_FUTURE and just
use the default allocator.

I'd suggest reverting that change, or is the removal of that
deprecated interface imminent?

Cheers,
Justus

[Justus Winter]

Quoting Justus Winter (2016-05-18 13:27:13)

> > Backing up one step: what is actually being done here in mach-defpager?
> > Again from just a quick look, is the idea to use wired memory for all of
> > mach-defpager's process memory?

Yes, wiring the memory is required to avoid deadlocks [0].

0: http://darnassus.sceen.net/~rbraun/hurd_related_papers/mach/moving_the_default_memory_manager_out_of_the_mach_kernel.pdf

> > So, for example, if now some mach-defpager code calls calloc, it will now
> > get non-wired "glibc" memory instead of wired "kalloc" memory. The
> > problem here is not mach-defpager's own code (which we can easily
> > verify/control), but the problems is the libraries it links against,
> > which currently are: libihash, libpthread, glibc itself, and any
>
> And indeed libihash uses calloc.
>
> > dependencies these may have. Can we be sure these are not internally
> > using calloc, for example?
> >
> > Now, this is not a new problem that you introduced ;-) -- previously we
> > also didn't provide malloc's memalign and realloc hooks, and realloc is
> > used quite commonly, I suppose?
> >
> > If my theories are correct, do we need to use a different mechanism to
> > get the whole mach-defpager process into wired memory?
>
> As Richard said in #hurd, implement mlockall and MCL_FUTURE and just
> use the default allocator.
>
> I'd suggest reverting that change, or is the removal of that
> deprecated interface imminent?

Wdyt?

Cheers,
Justus

[Samuel Thibault]

Justus Winter, on Wed 18 May 2016 13:27:13 +0200, wrote:
> As Richard said in #hurd, implement mlockall and MCL_FUTURE and just
> use the default allocator.

Indeed.

> I'd suggest reverting that change,

Better be safe for now, yes. I have also implemented the other hooks,
just to be on the safe side.

> or is the removal of that deprecated interface imminent?

I don't think it's that imminent.

Samuel

[Samuel Thibault]

Samuel Thibault, on Wed 31 Aug 2016 17:59:02 +0200, wrote:
> Debian is uploading 2.24, so we need to implement something now.

2.24-2 is now uploaded.

Samuel