Use buffer with BUFFER_BACKEND_MEMFD for _export_kv_store and reuse it for db diff sync and dump operation

[prajnoha]

Currently, we have _export_kv_store for syncing changes from db snapshot from worker processes to main process and _write_kv_store_dump to dump full db content for sidctl dump command.

We could possibly reuse a single implementation here because the functionality is very similar. The _export_kv_store would need some edits though as it is currently hard-codes the functionality for the "sync diffs" case only, not being generic to send also the whole db content.

The buffer code now supports using memfd as backend with BUFFER_BACKEND_MEMFD so when editing the _export_kv_store, we should also start using this kind of buffer for converged solution using existing shared code elements to minimize duplication as well as possible bugs (...so we can concentrate more on making the shared part rock solid which will be reused even more in the future).

There's probably one (optimization) dependency here: #81. If we knew how much memory to allocate exactly in advance, we wouldn't need to expand the memfd backend by steps (calling mremap internally each time). So would be better to have #81 resolved first before resolving this issue...

[bmarzins]

So do you want to pass a memfd for the sidclt dump command as well? If so, I think we might want to limit the dump command to root only. Otherwise, even if the the memfds are sealed, it seems like a malicious user could simply keep requesting a kv store dump, and never close the file descriptors, causing SID to allocate an unbounded amount of memory (well, up to the size of the kv store dump * the limit of open files per process * the limit of running processes).

[bmarzins]

Speaking of malicious users, it might make sense to limit the number of workers that can get spawned for connections that aren't from root. So users can't flood SID with commands. This would take reorganizing _on_ubridge_interface_event(). Is there a reason why SID can't accept the connection before forking? That would also save the worker proxy from having to pass the file descriptor to the worker process.

[prajnoha]

it seems like a malicious user could simply keep requesting a kv store dump, and never close the file descriptors, causing SID to allocate an unbounded amount of memory (well, up to the size of the kv store dump * the limit of open files per process * the limit of running processes).

Yes, you're right, we should be restricting that!

Thing is the kv-store can get relatively big when used on systems with high number of devices. Honestly, I feel uneasy when thinking of serialization of the whole db to just send it over to another process, then unserializing it just to put it on report. The dump operation should really be only for debug purposes, not something that is done by common users on regular basis. So restricting it to root seems reasonable.

In the future, I believe we can get rid of the serialization part completely and just share the memory of the whole db snapshot. But for that, we'd need custom memory allocator so we know which part(s) of the memory to share. Right now, with the usual malloc, I simply don't see what's behind, where the memory regions are exactly and I suppose they're also not one huge region, but several chunks of them.

[prajnoha]

Speaking of malicious users, it might make sense to limit the number of workers that can get spawned for connections that aren't from root. So users can't flood SID with commands. This would take reorganizing _on_ubridge_interface_event(). Is there a reason why SID can't accept the connection before forking? That would also save the worker proxy from having to pass the file descriptor to the worker process.

Yup, I'm counting with that too that we'll be limiting the number of workers. The reason I'm calling accept before forking (and then passing the FD over to worker) is that this way, I'm able to keep the worker around for another job (in the same "session"). This is also something to be completed (#27). For example, calling usid scan, usid checkpoint... and have the same worker (with the same db snapshot) to process several commands in sequence.

Also, when we find better way of creating a db snapshot than relying on the fork itself, we can keep a pool of workers even after processing current uevent. Then when any new request comes, we can simply pass the accepted connection to the worker to process it.

[bmarzins]

Yup, I'm counting with that too that we'll be limiting the number of workers. The reason I'm calling accept before forking (and then passing the FD over to worker) is that this way, I'm able to keep the worker around for another job (in the same "session"). This is also something to be completed (#27). For example, calling usid scan, usid checkpoint... and have the same worker (with the same db snapshot) to process several commands in sequence.

I'm confused here. _on_ubridge_interface_event() calls worker_control_get_new_worker(), which forks, before calling accept(). That is the ordering I think needs to change. Otherwise there is no way to stop a malicious user from forcing SID to create endless children. With abstract namespace sockets, IIRC there is no permission control until you examine the connection after accepting it. And like I mentioned, if the ordering does change, then new workers would fork with a copy of the socket ready to use. When sid starts reusing idle workers, the new socket would need to be passed to them, of course.

Also, when we find better way of creating a db snapshot than relying on the fork itself, we can keep a pool of workers even after processing current uevent. Then when any new request comes, we can simply pass the accepted connection to the worker to process it.

[prajnoha]

I'm confused here. _on_ubridge_interface_event() calls worker_control_get_new_worker(), which forks, before calling accept().

Sorry for the confusion! You're right, as you say, the accept is after forking right now, not before! In case we swap the order, we'll have the FD for accepted connection ready which will get inherited to worker, sure!

Now, I'm thinking how we'll get to that FD, which is actually within ubridge resource, while the worker is represented with worker resource on top. There's _worker_init_fn which is called at worker's start. That fn does have access to ubridge resource. So then we need a way how to tell the new worker resource:

• "Hey, worker resource, here's the inherited FD for the accepted connection I found inside ubridge resource, please remember it in your own internal structure"

Then, in that _worker_init_fn, we destroy any unneeded resources for the worker, including the ubridge resource. So we need to take care not to close the FD withing ubridge resource destroy operation. Also we'll be creating a special case - on worker init, the FD is passed with different method when compared to situation where we are reusing existing worker and passing the FD through the socket, which is becoming a bit clumsy then (we have two different "channels" where we pass the same information). And that can be a source of confusion when maintaining the code later.

But I have to admit, that in case we already have the FD inherited, it would be (at first sight) extra unneeded work if we passed the FD through the socket and not just find and use the inherited FD... So that's a tiny dilemma now for me :)

[bmarzins]

I don't understand why vector buffers would support BUFFER_BACKEND_MEMFD. I don't see a reason to use a memfd for data that we aren't trying to share between processes. If we are trying to share data between processes via a memfd, then the vector buffer is useless, since all that's stored in the memfd are iovecs that point to memory which the other process doesn't have access to. Is there some use case I'm not thinking of here?

[prajnoha]

Right, for vector buffer to be really usable with BUFFER_BACKEND_MEMFD, each vector item (the data it points to) would neeed to be:

• an offset instead of direct pointer
• the offset would need to be offset inside a shared memory

So yes, the memfd backend is directly usable with linear buffers. With vectors, there are extra conditions where it makes sense...

[prajnoha]

We could possibly reuse a single implementation here because the functionality is very similar. The _export_kv_store would need some edits though as it is currently hard-codes the functionality for the "sync diffs" case only, not being generic to send also the whole db content.

We have common _build_cmd_kv_buffers in place which is used by both sync and dump action.

The buffer code now supports using memfd as backend with BUFFER_BACKEND_MEMFD so when editing the _export_kv_store, we should also start using this kind of buffer for converged solution using existing shared code elements to minimize duplication as well as possible bugs (...so we can concentrate more on making the shared part rock solid which will be reused even more in the future).

The _build_cmd_kv_buffers uses BUFFER_BACKEND_MEMFD for both sync and dump action (the dump can be also redirected to a file, that is BUFFER_BACKEND_FILE which is similar to BUFFER_BACKEND_MEMFD (also shares most of the code), just instead of memory, we're writing to a file.

There's probably one (optimization) dependency here: #81. If we knew how much memory to allocate exactly in advance, we wouldn't need to expand the memfd backend by steps (calling mremap internally each time). So would be better to have #81 resolved first before resolving this issue...

This is not that important at the moment, we can improve this later in the future if we find that to be a performance issue.