- 12 Jan, 2023 11 commits
-
-
Magnus Karlsson authored
Remove unnecessary code in the control path. This is located in the file xsk.c that was moved from libbpf when the xsk support there was deprecated. Some of the code there is not needed anymore as the selftests are only guaranteed to run on the kernel it is shipped with. Therefore, all the code that has to deal with compatibility of older kernels can be dropped and also any other function that is not of any use for the tests. Signed-off-by:
Magnus Karlsson <magnus.karlsson@intel.com> Acked-by:
Maciej Fijalkowski <maciej.fijalkowski@intel.com> Link: https://lore.kernel.org/r/20230111093526.11682-11-magnus.karlsson@gmail.comSigned-off-by:
Alexei Starovoitov <ast@kernel.org>
-
Magnus Karlsson authored
Load and attach the XDP program only once per XDP mode that is being executed. Today, the XDP program is loaded and attached for every test, then unloaded, which takes a long time on real NICs, since they have to reconfigure their HW, in contrast to veth. The test suite now completes in 21 seconds, instead of 207 seconds previously on my machine. This is a speed-up of around 10x. This is accomplished by moving the XDP loading from the worker threads to the main thread and replacing the XDP loading interfaces of xsk.c that was taken from the xsk support in libbpf, with something more explicit that is more useful for these tests. Instead, the relevant file descriptors and ifindexes are just passed down to the new functions. Signed-off-by:
-
Magnus Karlsson authored
Remove the namespaces used as they fill no function. This will simplify the code for speeding up the tests in the following commits. Signed-off-by:
-
Magnus Karlsson authored
Replace our own homegrown assembly store/release and load/acquire implementations with the HW agnositic atomic APIs C11 offers. This to make the code more portable, easier to read, and reduce the maintenance burden. The original code used load-acquire and store-release barriers hand-coded in assembly. Since C11, these kind of operations are offered as built-ins in gcc and llvm. The load-acquire operation prevents hoisting of non-atomic memory operations to before this operation and it corresponds to the __ATOMIC_ACQUIRE operation in the built-in atomics. The store-release operation prevents hoisting of non-atomic memory operations to after this operation and it corresponds to the __ATOMIC_RELEASE operation in the built-in atomics. Signed-off-by:
-
Magnus Karlsson authored
Add a new option to the test_xsk.sh script that only creates the two veth netdevs and the extra namespace, then exits without running any tests. The failed test can then be executed in the debugger without having to create the netdevs and namespace manually. For ease-of-use, the veth netdevs to use are printed so they can be copied into the debugger. Here is an example how to use it: > sudo ./test_xsk.sh -d veth10 veth11 > gdb xskxceiver In gdb: run -i veth10 -i veth11 And now the test cases can be debugged with gdb. If you want to debug the test suite on a real NIC in loopback mode, there is no need to use this feature as you already know the netdev of your NIC. Signed-off-by:
-
Magnus Karlsson authored
Remove the unused variable outstanding_tx. Signed-off-by:
-
Magnus Karlsson authored
Print the correct error codes when exiting the test suite due to some terminal error. Some of these had a switched sign and some of them printed zero instead of errno. Fixes: facb7cb2 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL") Signed-off-by:
-
Magnus Karlsson authored
Submit the correct number of frames in the function xsk_populate_fill_ring(). For the tests that set the flag use_addr_for_fill, uninitialized buffers were sent to the fill ring following the correct ones. This has no impact on the tests, since they only use the ones that were initialized. But for correctness, this should be fixed. Signed-off-by:
-
Magnus Karlsson authored
Do not close descriptors that have never been used. File descriptor fields that are not in use are erroneously marked with the number 0, which is a valid fd. Mark unused fds with -1 instead and do not close these when deleting the socket. Signed-off-by:
-
Magnus Karlsson authored
Print the correct payload when the packet dump option is selected. The network to host conversion was forgotten and the payload was erronously declared to be an int instead of an unsigned int. Fixes: facb7cb2 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL") Signed-off-by:
-
Michal Suchanek authored
The ability to subscript match result as an array is only available since python 3.6. Existing code in bpf_doc uses the older group() interface but commit 8a76145a adds code using the new interface. Use the old interface consistently to avoid build error on older distributions like the below: + make -j48 -s -C /dev/shm/kbuild/linux.33946/current ARCH=powerpc HOSTCC=gcc CROSS_COMPILE=powerpc64-suse-linux- clean TypeError: '_sre.SRE_Match' object is not subscriptable Fixes: 8a76145a ("bpf: explicitly define BPF_FUNC_xxx integer values") Signed-off-by:
Michal Suchanek <msuchanek@suse.de> Signed-off-by:
Andrii Nakryiko <andrii@kernel.org> Acked-by:
Quentin Monnet <quentin@isovalent.com> Link: https://lore.kernel.org/bpf/20230109113442.20946-1-msuchanek@suse.de
-
- 11 Jan, 2023 2 commits
-
-
Ludovic L'Hours authored
As BPF_F_MMAPABLE flag is now conditionnaly set (by map_is_mmapable), it should not be toggled but disabled if not supported by kernel. Fixes: 4fcac46c ("libbpf: only add BPF_F_MMAPABLE flag for data maps with global vars") Signed-off-by:
Ludovic L'Hours <ludovic.lhours@gmail.com> Acked-by:
Jiri Olsa <jolsa@kernel.org> Link: https://lore.kernel.org/r/20230108182018.24433-1-ludovic.lhours@gmail.comSigned-off-by:
Martin KaFai Lau <martin.lau@kernel.org>
-
Chethan Suresh authored
When bpftool feature does not find kernel config files under default path or wrong format, do not output CONFIG_XYZ is not set. Skip kernel config check and continue. Signed-off-by:
Chethan Suresh <chethan.suresh@sony.com> Signed-off-by:
Kenta Tada <Kenta.Tada@sony.com> Acked-by:
-
- 10 Jan, 2023 5 commits
-
-
Connor O'Brien authored
Enabling CONFIG_MODULE_ALLOW_BTF_MISMATCH is an indication that BTF mismatches are expected and module loading should proceed anyway. Logging with pr_warn() on every one of these "benign" mismatches creates unnecessary noise when many such modules are loaded. Instead, handle this case with a single log warning that BTF info may be unavailable. Mismatches also result in calls to __btf_verifier_log() via __btf_verifier_log_type() or btf_verifier_log_member(), adding several additional lines of logging per mismatched module. Add checks to these paths to skip logging for module BTF mismatches in the "allow mismatch" case. All existing logging behavior is preserved in the default CONFIG_MODULE_ALLOW_BTF_MISMATCH=n case. Signed-off-by:
Connor O'Brien <connoro@google.com> Acked-by:
Yonghong Song <yhs@fb.com> Link: https://lore.kernel.org/r/20230107025331.3240536-1-connoro@google.comSigned-off-by:
-
Pu Lehui authored
Extra_nregs of structure parameters and nr_args can be added directly at the beginning, and using a flip flag to identifiy structure parameters. Meantime, renaming some variables to make them more sense. Signed-off-by:
Pu Lehui <pulehui@huawei.com> Acked-by:
-
Kees Cook authored
Zero-length arrays are deprecated [1]. Replace struct bpf_array's union of 0-length arrays with flexible arrays. Detected with GCC 13, by using -fstrict-flex-arrays=3: arch/x86/net/bpf_jit_comp.c: In function 'bpf_tail_call_direct_fixup': arch/x86/net/bpf_jit_comp.c:606:37: warning: array subscript <unknown> is outside array bounds of 'void *[0]' [-Warray-bounds=] 606 | target = array->ptrs[poke->tail_call.key]; | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/filter.h:9, from arch/x86/net/bpf_jit_comp.c:9: include/linux/bpf.h:1527:23: note: while referencing 'ptrs' 1527 | void *ptrs[0] __aligned(8); | ^~~~ [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arraysSigned-off-by:Kees Cook <keescook@chromium.org> Signed-off-by:
Daniel Borkmann <daniel@iogearbox.net> Acked-by:
Stanislav Fomichev <sdf@google.com> Link: https://lore.kernel.org/bpf/20230105192646.never.154-kees@kernel.org
-
James Hilliard authored
When passing compiler variables like CC=$(HOSTCC) to a submake we must ensure the variable is quoted in order to handle cases where $(HOSTCC) may be multiple binaries. For example when using ccache $HOSTCC may be: "/usr/bin/ccache /usr/bin/gcc" If we pass CC without quotes like CC=$(HOSTCC) only the first "/usr/bin/ccache" part will be assigned to the CC variable which will cause an error due to dropping the "/usr/bin/gcc" part of the variable in the submake invocation. This fixes errors such as: /usr/bin/ccache: invalid option -- 'd' Signed-off-by:
James Hilliard <james.hilliard1@gmail.com> Signed-off-by:
-
Haiyue Wang authored
The variable 'insn' is initialized to 'insn_buf' without being changed, only some helper macros are defined, so the insn buffer comparison is unnecessary. Just remove it. This missed removal back in 2377b81d ("bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation"). Signed-off-by:
Haiyue Wang <haiyue.wang@intel.com> Signed-off-by:
-
- 06 Jan, 2023 22 commits
-
-
Rong Tao authored
Since commit 9fc205b4("libbpf: Add sane strncpy alternative and use it internally") introduce libbpf_strlcpy(), thus add strlcpy() to a poison list to prevent accidental use of it. Signed-off-by:
Rong Tao <rongtao@cestc.cn> Signed-off-by:
-
David S. Miller authored
Jakub Kicinski says: ==================== devlink: remove the wait-for-references on unregister Move the registration and unregistration of the devlink instances under their instance locks. Don't perform the netdev-style wait for all references when unregistering the instance. Instead the devlink instance refcount will only ensure that the memory of the instance is not freed. All places which acquire access to devlink instances via a reference must check that the instance is still registered under the instance lock. This fixes the problem of the netdev code accessing devlink instances before they are registered. RFC: https://lore.kernel.org/all/20221217011953.152487-1-kuba@kernel.org/ - rewrite the cover letter - rewrite the commit message for patch 1 - un-export and rename devl_is_alive - squash the netdevsim patches ==================== Signed-off-by:
David S. Miller <davem@davemloft.net>
-
Jakub Kicinski authored
To prevent races with netdev code accessing free devlink instances move the registration under the devlink instance lock. Core now waits for the instance to be registered before accessing it. Signed-off-by:
Jakub Kicinski <kuba@kernel.org> Signed-off-by:
-
Jakub Kicinski authored
err_dl_unregister should unregister the devlink instance. Looks like renaming it was missed in one of the reshufflings. Reviewed-by:
Jacob Keller <jacob.e.keller@intel.com> Signed-off-by:
-
Jakub Kicinski authored
It's most natural to register the instance first and then its subobjects. Now that we can use the instance lock to protect the atomicity of all init - it should also be safe. Signed-off-by:
-
Jakub Kicinski authored
Requiring devlink_set_features() to be run before devlink is registered is overzealous. devlink_set_features() itself is a leftover from old workarounds which were trying to prevent initiating reload before probe was complete. Signed-off-by:
Jiri Pirko <jiri@nvidia.com> Signed-off-by:
-
Jakub Kicinski authored
The objective of exposing the devlink instance locks to drivers was to let them use these locks to prevent user space from accessing the device before it's fully initialized. This is difficult because devlink_unregister() waits for all references to be released, meaning that devlink_unregister() can't itself be called under the instance lock. To avoid this issue devlink_register() was moved after subobject registration a while ago. Unfortunately the netdev paths get a hold of the devlink instances _before_ they are registered. Ideally netdev should wait for devlink init to finish (synchronizing on the instance lock). This can't work because we don't know if the instance will _ever_ be registered (in case of failures it may not). The other option of returning an error until devlink_register() is called is unappealing (user space would get a notification netdev exist but would have to wait arbitrary amount of time before accessing some of its attributes). Weaken the guarantees of the devlink references. Holding a reference will now only guarantee that the memory of the object is around. Another way of looking at it is that the reference now protects the object not its "registered" status. Use devlink instance lock to synchronize unregistration. This implies that releasing of the "main" reference of the devlink instance moves from devlink_unregister() to devlink_free(). Reviewed-by:
-
Jakub Kicinski authored
Always check under the instance lock whether the devlink instance is still / already registered. This is a no-op for the most part, as the unregistration path currently waits for all references. On the init path, however, we may temporarily open up a race with netdev code, if netdevs are registered before the devlink instance. This is temporary, the next change fixes it, and this commit has been split out for the ease of review. Note that in case of iterating over sub-objects which have their own lock (regions and line cards) we assume an implicit dependency between those objects existing and devlink unregistration. Signed-off-by:
-
Jakub Kicinski authored
devlink->dev is assumed to be always valid as long as any outstanding reference to the devlink instance exists. In prep for weakening of the references take the instance lock. Signed-off-by:
-
Jakub Kicinski authored
devlink_pernet_pre_exit() is the only obvious place which takes the instance lock without using the devl_ helpers. Update the code and move the error print after releasing the reference (having unlock and put together feels slightly idiomatic). Reviewed-by:
-
Jakub Kicinski authored
xa_find_after() is designed to handle multi-index entries correctly. If a xarray has two entries one which spans indexes 0-3 and one at index 4 xa_find_after(0) will return the entry at index 4. Having to juggle the two callbacks, however, is unnecessary in case of the devlink xarray, as there is 1:1 relationship with indexes. Always use xa_find() and increment the index manually. Signed-off-by:
-
Mahesh Bandewar authored
All were not visible to the non-priv users inside netns. However, with 4ecb9009 ("sysctl: allow override of /proc/sys/net with CAP_NET_ADMIN"), these vars are protected from getting modified. A proc with capable(CAP_NET_ADMIN) can change the values so not having them visible inside netns is just causing nuisance to process that check certain values (e.g. net.core.somaxconn) and see different behavior in root-netns vs. other-netns Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Soheil Hassas Yeganeh <soheil@google.com> Signed-off-by:
Mahesh Bandewar <maheshb@google.com> Acked-by:
Soheil Hassas Yeganeh <soheil@google.com> Signed-off-by:
-
Jakub Kicinski authored
Jakub Kicinski says: ==================== devlink: code split and structured instance walk Split devlink.c into a handful of files, trying to keep the "core" code away from all the command-specific implementations. The core code has been quite scattered until now. Going forward we can consider using a source file per-subobject, I think that it's quite beneficial to newcomers (based on relative ease with which folks contribute to ethtool vs devlink). But this series doesn't split everything out, yet - partially due to backporting concerns, but mostly due to lack of time. Bulk of the netlink command handling is left in a leftover.c file. Introduce a context structure for dumps, and use it to store the devlink instance ID of the last dumped devlink instance. This means we don't have to restart the walk from 0 each time. Finally - introduce a "structured walk". A centralized dump handler in devlink/netlink.c which walks the devlink instances, deals with refcounting/locking, simplifying the per-object implementations quite a bit. Inspired by the ethtool code. v1: https://lore.kernel.org/all/20230104041636.226398-1-kuba@kernel.org/ RFC: https://lore.kernel.org/all/20221215020155.1619839-1-kuba@kernel.org/ ==================== Link: https://lore.kernel.org/r/20230105040531.353563-1-kuba@kernel.orgSigned-off-by:
-
Jakub Kicinski authored
Soon we'll have to check if a devlink instance is alive after locking it. Convert to the by-instance dumping scheme to make refactoring easier. Most of the subobject code no longer has to worry about any devlink locking / lifetime rules (the only ones that still do are the two subject types which stubbornly use their own locking). Both dump and do callbacks are given a devlink instance which is already locked and good-to-access (do from the .pre_doit handler, dump from the new dump indirection). Note that we'll now check presence of an op (e.g. for sb_pool_get) under the devlink instance lock, that will soon be necessary anyway, because we don't hold refs on the driver modules so the memory in which ops live may be gone for a dead instance, after upcoming locking changes. Reviewed-by:
-
Jakub Kicinski authored
Most dumpit implementations walk the devlink instances. This requires careful lock taking and reference dropping. Factor the loop out and provide just a callback to handle a single instance dump. Convert one user as an example, other users converted in the next change. Slightly inspired by ethtool netlink code. Reviewed-by:
-
Jakub Kicinski authored
Move the lock taking out of devlink_nl_cmd_region_get_devlink_dumpit(). This way all dumps will take the instance lock in the main iteration loop directly, making refactoring and reading the code easier. Reviewed-by:
-
Jakub Kicinski authored
Use xarray id for cases of sub-objects which are iterated in a function. Reviewed-by:
-
Jakub Kicinski authored
Use xarray id for cases of simple sub-object iteration. We'll now use the state->instance for the devlink instances and state->idx for subobject index. Moving the definition of idx into the inner loop makes sense, so while at it also move other sub-object local variables into the loop. Reviewed-by:
-
Jakub Kicinski authored
xarray gives each devlink instance an id and allows us to restart walk based on that id quite neatly. This is nice both from the perspective of code brevity and from the stability of the dump (devlink instances disappearing from before the resumption point will not cause inconsistent dumps). This patch takes care of simple cases where state->idx counts devlink instances only. Reviewed-by:
-
Jakub Kicinski authored
Walk devlink instances only once. Dump the instance reporters and port reporters before moving to the next instance. User space should not depend on ordering of messages. This will make improving stability of the walk easier. Reviewed-by:
-
Jakub Kicinski authored
Looks like devlinks_xa_find_get() was intended to get the mark from the @filter argument. It doesn't actually use @filter, passing DEVLINK_REGISTERED to xa_find_fn() directly. Walking marks other than registered is unlikely so drop @filter argument completely. Reviewed-by:
-
Jakub Kicinski authored
The start variables made the code clearer when we had to access cb->args[0] directly, as the name args doesn't explain much. Now that we use a structure to hold state this seems no longer needed. Reviewed-by:
-
