On 21/11/2022 16.29, Jason A. Donenfeld wrote:
Cc += linux-api
if (!new_block)
goto out;
new_cap = grnd_allocator.cap + num;
new_states = reallocarray(grnd_allocator.states, new_cap, sizeof(*grnd_allocator.states));
if (!new_states) {
munmap(new_block, num * size_per_each);
Hm. This does leak an implementation detail of vgetrandom_alloc(),
namely that it is based on mmap() of that size rounded up to page size.
Do we want to commit to this being the proper way of disposing of a
succesful vgetrandom_alloc(), or should there also be a
vgetrandom_free(void *states, long num, long size_per_each)?
And if so, what color should the bikeshed really have. I.e.,
- does it need to take that size_per_each parameter which the kernel knows
- should it rather take the product so it can for now be a simple alias
for munmap
- should it also have a flags argument just because that's what all
well-behaving syscalls have these days...
Also, should vgetrandom_alloc() take a void *hint argument that
would/could be passed through to mmap() to give userspace some control
over where the memory is located - possibly only in the future, i.e.
insist on it being NULL for now, but it could open the possibility for
adding e.g. VGRND_MAP_FIXED[_NOREPLACE] that would translate to the
corresponding MAP_ flags.
Rasmus
Hi Rasmus,
On Wed, Nov 23, 2022 at 09:51:04AM +0100, Rasmus Villemoes wrote:
On 21/11/2022 16.29, Jason A. Donenfeld wrote:
Cc += linux-api
quoted
if (!new_block)
goto out;
new_cap = grnd_allocator.cap + num;
new_states = reallocarray(grnd_allocator.states, new_cap, sizeof(*grnd_allocator.states));
if (!new_states) {
munmap(new_block, num * size_per_each);
Hm. This does leak an implementation detail of vgetrandom_alloc(),
namely that it is based on mmap() of that size rounded up to page size.
Do we want to commit to this being the proper way of disposing of a
succesful vgetrandom_alloc(), or should there also be a
vgetrandom_free(void *states, long num, long size_per_each)?
Yes, this is intentional, and this is exactly what I wanted to do. There
are various wrappers of vm_mmap() throughout, mmap being one of them,
and they typically then resort to munmap to unmap it. This is how
userspace handles memory - maps, always maps. So I think doing that is
fine and consistent.
However, your point about it relying on it being a rounded up size isn't
correct. `munmap` will unmap the whole page if the size you pass lies
within a page. So `num * size_of_each` will always do the right thing,
without needing to have userspace code round anything up. (From the man
page: "The address addr must be a multiple of the page size (but length
need not be). All pages containing a part of the indicated range are
unmapped.") And as you can see in my example code, nothing is rounded
up. So I don't know why you made that comment.
And if so, what color should the bikeshed really have. I.e.,
No color, thanks.
Also, should vgetrandom_alloc() take a void *hint argument that
would/could be passed through to mmap() to give userspace some control
over where the memory is located - possibly only in the future, i.e.
insist on it being NULL for now, but it could open the possibility for
adding e.g. VGRND_MAP_FIXED[_NOREPLACE] that would translate to the
corresponding MAP_ flags.
I think adding more control is exactly what this is trying to avoid.
It's very intentionally *not* a general allocator function, but
something specific for vDSO getrandom(). However, it does already, in
this very patchset here, take a (currently unused) flags argument, in
case we have the need for later extension.
In the meantime, however, I'm not very interested in complicating this
interface into oblivion. Firstly, it ensures nothing will get done. But
moreover, this interface needs to be somewhat future-proof, yes, but it
does not need to be a general syscall; rather, it's a specific syscall
for a specific task.
Jason
On 24/11/2022 02.18, Jason A. Donenfeld wrote:
Hi Rasmus,
On Wed, Nov 23, 2022 at 09:51:04AM +0100, Rasmus Villemoes wrote:
quoted
On 21/11/2022 16.29, Jason A. Donenfeld wrote:
Cc += linux-api
quoted
if (!new_block)
goto out;
new_cap = grnd_allocator.cap + num;
new_states = reallocarray(grnd_allocator.states, new_cap, sizeof(*grnd_allocator.states));
if (!new_states) {
munmap(new_block, num * size_per_each);
Hm. This does leak an implementation detail of vgetrandom_alloc(),
namely that it is based on mmap() of that size rounded up to page size.
Do we want to commit to this being the proper way of disposing of a
succesful vgetrandom_alloc(), or should there also be a
vgetrandom_free(void *states, long num, long size_per_each)?
Yes, this is intentional, and this is exactly what I wanted to do. There
are various wrappers of vm_mmap() throughout, mmap being one of them,
and they typically then resort to munmap to unmap it. This is how
userspace handles memory - maps, always maps. So I think doing that is
fine and consistent.
OK. Perhaps for the benefit of future libc implementors drop a comment
somewhere as to how to dealloc the blob.
However, your point about it relying on it being a rounded up size isn't
correct. `munmap` will unmap the whole page if the size you pass lies
within a page. So `num * size_of_each` will always do the right thing,
without needing to have userspace code round anything up. (From the man
page: "The address addr must be a multiple of the page size (but length
need not be).
I know, and I never said userspace needed to round anything up.
All pages containing a part of the indicated range are
unmapped.") And as you can see in my example code, nothing is rounded
up. So I don't know why you made that comment.
I made that comment because it's clear from what this does that you get
something back that is _at least_ num*size_per_each in size, but what is
not clear is that the actual allocation is exactly and will always be
that size rounded up to a page size (and no more), so that
munmap(num*size_per_each), with its well-known and documented semantics,
will DTRT.
I think adding more control is exactly what this is trying to avoid.
It's very intentionally *not* a general allocator function, but
something specific for vDSO getrandom(). However, it does already, in
this very patchset here, take a (currently unused) flags argument, in
case we have the need for later extension.
OK.
Perhaps you can spend a few more words on why this allocation _needs_ to
be MAP_LOCKED? That seems somewhat of a policy thing imposed by the
kernel, something that would be better left to the libc or distro or
whatnot to request via a flag. I could imagine applications that
currently run at the mlock limit start failing after a libc upgrade -
which could of course be considered a libc problem, and perhaps it's too
unlikely to worry about.
Rasmus