ROX-30437: resolve host path via inode tracking #158
Conversation
This patch makes it so, at start up, we walk through the configured paths we have to monitor and add the path to the inode of every file we find. This first implementation is inherently flawed and meant as a way to show how it would look once it is fully fleshed out.
Added tests will validate events generated on an overlayfs file properly shows the event on the upper layer and the access to the underlying FS. They also validate a mounted path on a container resolves to the correct host path. While developing these tests, it became painfully obvious getting the information of the process running inside the container is not straightforward. Because containers tend to be fairly static, we should be able to manually create the information statically in the test and still have everything work correctly. In order to minimize the amount of changes on existing tests, the default Process constructor now takes fields directly and there is a from_proc class method that builds a new Process object from /proc. Additionally, getting the pid of a process in a container is virtually impossible, so we make the pid check optional.
|
This is an alternative implementation to #149. Compared to that implementation, there are a few benefits to this implementation:
However, there are also some downsides:
IMO, though significant the limitations of this implementation can be improved in subsequent PRs, the current implementation should be enough for our current target of monitoring 4 system files that should never be removed/renamed. |
|
|
||
| int32_t add_path(int32_t map_fd, const char* path, const char* host_path) { | ||
| int fd = open(path, O_RDONLY); | ||
| if (fd <= 0) { |
There was a problem hiding this comment.
| if (fd <= 0) { | |
| if (fd < 0) { |
| checksum = "deec109607ca693028562ed836a5f1c4b8bd77755c4e132fc5ce11b0b6211ae7" | ||
| checksum = "b97463e1064cb1b1c1384ad0a0b9c8abd0988e2a91f52606c80ef14aadb63e36" | ||
| dependencies = [ | ||
| "find-msvc-tools", |
There was a problem hiding this comment.
What's the purpose of this dependency?
| } | ||
|
|
||
| if (!is_monitored(path)) { | ||
| if (host_path == NULL && !is_monitored(path)) { |
There was a problem hiding this comment.
Why should we ignore events if the host_path was not resolved?
|
|
||
| long res = syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr)); | ||
| if (res == -EEXIST) { | ||
| res = 0; |
There was a problem hiding this comment.
Some debug log output in this case?
| } | ||
|
|
||
| fn path_to_cstring(path: &Path) -> anyhow::Result<CString> { | ||
| let path = path.as_os_str().to_string_lossy(); |
There was a problem hiding this comment.
Looks like Path has its own to_string_lossy that does the same thing as OsStr, why convert?
Regarding lossy part -- I assume it will cause problems with non-UTF8 file names, correct?
|
|
||
| pub fn walk_path(inode_store: &mut MapData, path: &Path) -> anyhow::Result<()> { | ||
| if path.is_dir() { | ||
| for entry in (path.read_dir()?).flatten() { |
There was a problem hiding this comment.
Looks like is_dir resolves symlinks, I guess it's worth adding some loop prevention logic in the future.
| union bpf_attr attr; | ||
| memset(&attr, 0, sizeof(attr)); | ||
| attr.map_fd = map_fd; | ||
| attr.key = (unsigned long long)&fd; |
There was a problem hiding this comment.
I don't get it, we search in inode_store by the inode:
bpf_inode_storage_get(&inode_store, file->f_inode, NULL, 0);
but store by the file descriptor?
There was a problem hiding this comment.
That's how the inode storage works. When accessing from userspace via the bpf syscall you don't have a way to access the inode pointer itself (because it lives on kernel memory), so you give it the file descriptor to the file and the kernel resolves the inode under the hood:
https://github.com/torvalds/linux/blob/2061f18ad76ecaddf8ed17df81b8611ea88dbddd/kernel/bpf/bpf_inode_storage.c#L99
But on kernel side, you do have the inode pointer, so you can use that directly:
https://github.com/torvalds/linux/blob/2061f18ad76ecaddf8ed17df81b8611ea88dbddd/kernel/bpf/bpf_inode_storage.c#L128-L129
|
Superseded by #166 |
3 participants
Description
This patch makes it so, at start up, we walk through the configured
paths we have to monitor and add the path to the inode of every file we
find. This first implementation is inherently flawed and meant as a way
to show how it would look once it is fully fleshed out.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Added tests will validate events generated on an overlayfs file properly
shows the event on the upper layer and the access to the underlying FS.
They also validate a mounted path on a container resolves to the correct
host path.
While developing these tests, it became painfully obvious getting the
information of the process running inside the container is not
straightforward. Because containers tend to be fairly static, we should
be able to manually create the information statically in the test and
still have everything work correctly. In order to minimize the amount of
changes on existing tests, the default Process constructor now takes
fields directly and there is a from_proc class method that builds a new
Process object from /proc. Additionally, getting the pid of a process in
a container is virtually impossible, so we make the pid check optional.