Canonicalize pin paths #4274

dthaler · 2025-03-10T19:36:30Z

Description

Pinning related APIs should treat pin paths the same as filesystem paths and canonicalize them accordingly
Fix documentation error with ebpf_get_next_pinned_object_path() where the start_path cannot be null.
Added a test. Note that in doing so, I found and filed issue pinned_path_count is always 0 #4273 and added a TODO for that issue in the test where it should be tested.

Note: on Linux, pin paths are case sensitive, and so any programs ported from Linux may assume case sensitivity.
On Windows, it varies by filesystem, where Windows itself supports both case sensitive filesystems and case insensitive filesystems. The latter is the normal filesystem on Windows, but if we later support a BPF filesystem, we can make it be case sensitive, for ease of porting from Linux. As such, this PR uses case sensitive pin paths. This is not a change for ebpf-for-windows, which already had case-sensitive pin paths.

Testing

This PR contains associated tests.

Documentation

This PR includes a design doc added to the docs directory.

Installation

No impact.

scripts/check_binary_dependencies_ebpfapi_dll_regular_debug.txt

tests/end_to_end/end_to_end.cpp

docs/PinPaths.md

lmb · 2025-03-19T09:13:15Z

docs/PinPaths.md

+* Pins can be enumerated or removed via normal filesystem APIs/utilities
+* Pin paths are case-sensitive
+* The path component separator is a forward slash ('/')
+* Pin paths begin with "/sys/fs/bpf/" and relative paths like "mymap" are relative to that prefix.


Not necessarily, this is just a convention that systemd and the popular user space libraries follow. It is possible to mount a separate BPF fs at say /var/run/myapp/bpffs and then pin into that.

Updated text

lmb · 2025-03-19T09:16:01Z

docs/PinPaths.md

+the driver implements it as such.
+
+As for portability, canonicalization could ensure that Linux style paths like
+"/sys/fs/bpf/my/pin/path" would be canonicalized to "BPF:\my\pin\path" allowing


My gut feeling would be to not remove the sys/fs/bpf prefix. If you do want to remove it, what should happen to paths that do not have it?

Updated to not remove it.

lmb · 2025-03-19T09:16:38Z

docs/PinPaths.md

+
+As for portability, canonicalization could ensure that Linux style paths like
+"/sys/fs/bpf/my/pin/path" would be canonicalized to "BPF:\my\pin\path" allowing
+portability in that respect.  But querying the pin path on an object


Incomplete sentence?

docs/PinPaths.md

lmb · 2025-03-19T09:58:21Z

docs/PinPaths.md

+
+### UNC (UNC path)
+
+Example: \\BPF\my\pin\path


There is also \\?\ which would disable all path parsing and would forward the path to the filesystem verbatim. And \\.\ which points into the object namespace, but I don't know whether we could hook that somehow?

Added a brief note.

lmb · 2025-03-19T10:00:36Z

docs/PinPaths.md

+
+### Virtual (Windows file system path with another virtual drive for BPF)
+
+Example: BPF:\my\pin\path


Sorry for being a windows noob, is it possible to have multi letter drives? Never seen one of those.

Also, would this drive show up in the explorer?

It's not a drive, but a virtual device (aka DOS device name). There are a bunch of these in windows, like CON:, PRN:, NUL:, COM1:, etc. And drivers can add new ones.

Updated wording to explain.

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

lmb

I still want to run ebpf-go unit tests against this, I'll get back to you!

lmb · 2025-03-31T16:08:41Z

libs/execution_context/ebpf_native.c

-            native_maps[i].pin_path.value =
-                ebpf_allocate_with_tag(prefix_length + name_length + 1, EBPF_POOL_TAG_NATIVE);
+            size_t length = strlen(canonical_path);
+            native_maps[i].pin_path.value = ebpf_allocate_with_tag(length, EBPF_POOL_TAG_NATIVE);


Off by one here for NUL?

The NUL isn't needed here since the length is stored. This isn't a change from before, which also didn't include space for a NUL. The +1 before was for the "/", as can be seen in old lines 1042-1045.

lmb · 2025-03-31T16:09:52Z

libs/shared/shared_common.c

+    strcpy_s(output, output_size, "BPF:\\");
+
+    // Add the BPF: prefix if not already present.
+    if (_strnicmp(input, "BPF:\\", PREFIX_SIZE) == 0 || _strnicmp(input, "BPF:/", PREFIX_SIZE) == 0) {


Nit: would this be easier after transforming slashes?

Not really, a mutable buffer is used to transform slashes, and this is the code that copies the string into the mutable buffer.

lmb · 2025-03-31T16:12:59Z

libs/shared/shared_common.c

+ebpf_result_t
+ebpf_canonicalize_path(_Out_writes_(output_size) char* output, size_t output_size, _In_z_ const char* input)
+{
+    const int PREFIX_SIZE = 5; // Length of "BPF:\\".


Isn't that 4?

No, it's 5. BPF is 3, then a colon, then a backslash.

Updated code to be easier to follow (hopefully), and define it as 4 instead of 5 based on the new code.

Alan-Jowett · 2025-03-31T20:36:17Z

libs/shared/shared_common.c

+ebpf_canonicalize_path(_Out_writes_(output_size) char* output, size_t output_size, _In_z_ const char* input)
+{
+    const int PREFIX_SIZE = 5; // Length of "BPF:\\".
+    strcpy_s(output, output_size, "BPF:\\");


Shouldn't we check the return code from strcpy_s? This fails if the destination buffer is too small.

Alan-Jowett · 2025-03-31T20:37:09Z

libs/shared/shared_common.c

+            memmove(output + i + 1, next, strlen(next) + 1);
+        } else if (strncmp(output + i, "\\..\\", 4) == 0) {
+            int previous_index = i - 1;
+            for (; previous_index >= 4; previous_index--) {


Why 4 as the boundary value?

"BPF:" is length 4.

Updated to be easier to follow (I hope).

libs/shared/shared_common.c

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

lmb · 2025-04-02T16:20:05Z

Ran the ebpf-go testsuite against this PR, and it turns out that ebpf_get_next_pinned_object_path needs canonicalization as well.

dthaler · 2025-04-02T20:51:50Z

Ran the ebpf-go testsuite against this PR, and it turns out that ebpf_get_next_pinned_object_path needs canonicalization as well.

@lmb What is the specific test case(s) you are using with this API?

lmb · 2025-04-14T09:06:48Z

Test case: https://github.com/cilium/ebpf/pull/1736/files#diff-5daa97a3a95cd1516d793eeff2ceb72f8d8fe926f38e0ec584e6ae21f7e5e4c8
Function being tested: https://github.com/cilium/ebpf/pull/1736/files#diff-d3c75b3feceb3d403da6f23bcb72e3374767dbbab89f80a014a0b0c5fc0bb417

P.S. Actually I think canonicalization doesn't work / help. The reason is the HasPrefix call which immediately bails out when using a path like "/sys/fs/bpf/foo" as input because the output of the function includes the BPF: drive letter, or has its slashes normalised. We need the HasPrefix check to determine when to end iteration. This could be worked around by moving that check into ebpf_get_next_pinned_object_path, and operate on normalized paths there. Doesn't solve the problem outright of course.

lmb · 2025-04-14T16:48:01Z

Assuming the pin table contains:

BPF:\foo\bar
BPF:\foo\baz
BPF:\beep

Here is what the library wants to do:

root	cursor	next path	continue?
/foo	/foo	BPF:\foo\bar	yes
/foo	/foo/baz	BPF:\bloop	no
/	/	BPF:\foo\bar	yes

At the moment this doesn't work, since the next path is always returned in its canonicalized form, but the root which we are comparing against isn't canonicalized.

Possible work arounds:

Require that the library passes a canonicalized path to the function.
Pass root to the function and do the continue check in the runtime, on the canonicalized version.

dthaler · 2025-04-19T19:02:46Z

Assuming the pin table contains:

BPF:\foo\bar

BPF:\foo\baz

BPF:\beep

Here is what the library wants to do:

root cursor next path continue?
/foo /foo BPF:\foo\bar yes
/foo /foo/baz BPF:\bloop no
/ / BPF:\foo\bar yes
At the moment this doesn't work, since the next path is always returned in its canonicalized form, but the root which we are comparing against isn't canonicalized.

Possible work arounds:

Require that the library passes a canonicalized path to the function.

Pass root to the function and do the continue check in the runtime, on the canonicalized version.

@lmb ebpfapi has:

/**
     * @brief Retrieve the next pinned path of an eBPF object.
     *
     * @param[in] start_path Path to look for an entry greater than.
     * @param[out] next_path Returns the next path in lexicographical order, if one exists.
     * @param[in] next_path_len Length of the next path buffer.
     * @param[in, out] type On input, the type of object to retrieve or EBPF_OBJECT_UNKNOWN.
     *                      On output, the type of the object.
     *
     * @retval EBPF_SUCCESS The operation was successful.
     * @retval other An error occurred.
     */
    _Must_inspect_result_ ebpf_result_t
    ebpf_get_next_pinned_object_path(
        _In_z_ const char* start_path,
        _Out_writes_z_(next_path_len) char* next_path,
        size_t next_path_len,
        _Inout_ ebpf_object_type_t* type) EBPF_NO_EXCEPT;

In your table, you have "next path" as a column header. Is that what you're expecting to get back? How could you get back "BPF:\bloop" when that isn't in the pin path table?

What does "root" mean? does that mean the library wants to enumerate only the paths under that root?

What does "cursor" mean? does that mean it's the value you want to pass to start_path to look for an entry greater than?

If we export a function to get the canonical path from the non-canonical path, would that solve the problem?

And how is that different than on linux if the root is "/bar/../foo", where that wouldn't be a substring of the next path returned by an enumeration? Don't you already have to canonicalize the path to compare against in your library?

dthaler requested review from Alan-Jowett, dv-msft, LakshK98, matthewige, mikeagun, mtfriesen, poornagmsft, saxena-anurag, shankarseal and vpidatala94 as code owners March 10, 2025 19:36

dthaler requested a review from lmb March 10, 2025 19:36

dthaler mentioned this pull request Mar 10, 2025

pinned_path_count is always 0 #4273

Open

dthaler changed the title ~~Canonicalize paths~~ Canonicalize pin paths Mar 10, 2025

Alan-Jowett previously approved these changes Mar 11, 2025

View reviewed changes

dthaler dismissed Alan-Jowett’s stale review via 3c38a22 March 11, 2025 18:27

Alan-Jowett reviewed Mar 12, 2025

View reviewed changes

scripts/check_binary_dependencies_ebpfapi_dll_regular_debug.txt Outdated Show resolved Hide resolved

dthaler force-pushed the canonicalize-paths branch from 888ff28 to 8717508 Compare March 14, 2025 15:55

lmb reviewed Mar 17, 2025

View reviewed changes

tests/end_to_end/end_to_end.cpp Outdated Show resolved Hide resolved

saxena-anurag reviewed Mar 17, 2025

View reviewed changes

docs/PinPaths.md Outdated Show resolved Hide resolved

dthaler force-pushed the canonicalize-paths branch 5 times, most recently from 81b2d93 to a20d9c8 Compare March 18, 2025 15:54

dthaler requested review from lmb, Alan-Jowett and saxena-anurag March 18, 2025 15:54

lmb reviewed Mar 19, 2025

View reviewed changes

dthaler requested a review from lmb March 31, 2025 15:24

Canonicalize pin paths using filesystem canonicalization rules

c514d47

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

dthaler added 3 commits March 31, 2025 08:27

Update some bpftool_tests

8394bb0

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Add decision to PinPaths.md

42efa7b

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Address PR feedback

b5fc392

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

dthaler force-pushed the canonicalize-paths branch from bb70b64 to b5fc392 Compare March 31, 2025 15:28

lmb reviewed Mar 31, 2025

View reviewed changes

Alan-Jowett reviewed Mar 31, 2025

View reviewed changes

libs/shared/shared_common.c Show resolved Hide resolved

Address PR feedback

39d8b70

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

dthaler requested review from Alan-Jowett and lmb April 2, 2025 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonicalize pin paths #4274

Canonicalize pin paths #4274

dthaler commented Mar 10, 2025 •

edited

Loading

lmb Mar 19, 2025

dthaler Mar 31, 2025

lmb Mar 19, 2025

dthaler Mar 31, 2025

lmb Mar 19, 2025

dthaler Mar 31, 2025

lmb Mar 19, 2025

dthaler Mar 31, 2025

lmb Mar 19, 2025

dthaler Mar 19, 2025

dthaler Mar 31, 2025

lmb left a comment

lmb Mar 31, 2025

dthaler Mar 31, 2025

lmb Mar 31, 2025

dthaler Mar 31, 2025 •

edited

Loading

lmb Mar 31, 2025

dthaler Mar 31, 2025

dthaler Apr 2, 2025

Alan-Jowett Mar 31, 2025

dthaler Apr 2, 2025

Alan-Jowett Mar 31, 2025

dthaler Apr 1, 2025

dthaler Apr 2, 2025

lmb commented Apr 2, 2025

dthaler commented Apr 2, 2025 •

edited

Loading

lmb commented Apr 14, 2025 •

edited

Loading

lmb commented Apr 14, 2025

dthaler commented Apr 19, 2025


		### Virtual (Windows file system path with another virtual drive for BPF)

		Example: BPF:\my\pin\path

Canonicalize pin paths #4274

Are you sure you want to change the base?

Canonicalize pin paths #4274

Conversation

dthaler commented Mar 10, 2025 • edited Loading

Description

Testing

Documentation

Installation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb commented Apr 2, 2025

dthaler commented Apr 2, 2025 • edited Loading

lmb commented Apr 14, 2025 • edited Loading

lmb commented Apr 14, 2025

dthaler commented Apr 19, 2025

dthaler commented Mar 10, 2025 •

edited

Loading

dthaler Mar 31, 2025 •

edited

Loading

dthaler commented Apr 2, 2025 •

edited

Loading

lmb commented Apr 14, 2025 •

edited

Loading