- 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:
Martin KaFai Lau <martin.lau@kernel.org>
-
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:
Quentin Monnet <quentin@isovalent.com> Link: https://lore.kernel.org/bpf/20230110014504.3120711-1-james.hilliard1@gmail.com
-
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 35 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:
-
Jakub Kicinski authored
Create a dump context structure instead of using cb->args as an unsigned long array. This is a pure conversion which is intended to be as much of a noop as possible. Subsequent changes will use this to simplify the code. The two non-trivial parts are: - devlink_nl_cmd_health_reporter_dump_get_dumpit() checks args[0] to see if devlink_fmsg_dumpit() has already been called (whether this is the first msg), but doesn't use the exact value, so we can drop the local variable there already - devlink_nl_cmd_region_read_dumpit() uses args[0] for address but we'll use args[1] now, shouldn't matter Reviewed-by:
-
Jakub Kicinski authored
We encourage casting struct netlink_callback::ctx to a local struct (in a comment above the field). Provide a convenience macro for checking if the local struct fits into the ctx. Reviewed-by:
-
Jakub Kicinski authored
Move out the netlink glue into a separate file. Leave the ops in the old file because we'd have to export a ton of functions. Going forward we should switch to split ops which will let us to put the new ops in the netlink.c file. Pure code move, no functional changes. Reviewed-by:
-
Jakub Kicinski authored
Move core code into a separate file. It's spread around the main file which makes refactoring and figuring out how devlink works harder. Move the xarray, all the most core devlink instance code out like locking, ref counting, alloc, register, etc. Leave port stuff in leftover.c, if we want to move port code it'd probably be to its own file. Reviewed-by:
-
Jakub Kicinski authored
To make the upcoming change a pure(er?) code move rename devlink_netdevice_event -> devlink_port_netdevice_event. This makes it clear that it only touches ports and doesn't belong cleanly in the core. Reviewed-by:
-
Jakub Kicinski authored
The devlink code is hard to navigate with 13kLoC in one file. I really like the way Michal split the ethtool into per-command files and core. It'd probably be too much to split it all up, but we can at least separate the core parts out of the per-cmd implementations and put it in a directory so that new commands can be separate files. Move the code, subsequent commit will do a partial split. Reviewed-by:
-
Jakub Kicinski authored
Alex Elder says: ==================== net: ipa: simplify IPA interrupt handling One of the IPA's two IRQs fires when data on a suspended channel is available (to request that the channel--or system--be resumed to recieve the pending data). This interrupt also handles a few conditions signaled by the embedded microcontroller. For this "IPA interrupt", the current code requires a handler to be dynamically registered for each interrupt condition. Any condition that has no registered handler is quietly ignored. This design is derived from the downstream IPA driver implementation. There isn't any need for this complexity. Even in the downstream code, only four of the available 30 or so IPA interrupt conditions are ever handled. So these handlers can pretty easily just be called directly in the main IRQ handler function. This series simplifies the interrupt handling code by having the small number of IPA interrupt handlers be called directly, rather than having them be registered dynamically. Version 2 just adds a missing forward-reference, as suggested by Caleb. ==================== Link: https://lore.kernel.org/r/20230104175233.2862874-1-elder@linaro.orgSigned-off-by:
-
Alex Elder authored
We can call the two IPA interrupt handler functions directly; there's no need to maintain the array of handler function pointers any more. Signed-off-by:
Alex Elder <elder@linaro.org> Signed-off-by:
-
Alex Elder authored
The dynamic assignment of IPA interrupt handlers isn't needed; we only handle three IPA interrupt types, and their handler functions are now assigned directly. We can get rid of ipa_interrupt_add() and ipa_interrupt_remove() now, because they serve no purpose. Signed-off-by:
-
Alex Elder authored
Declare the microcontroller IPA interrupt handler publicly, and assign it directly in ipa_interrupt_config(). Make the SUSPEND IPA interrupt handler public, and rename it ipa_power_suspend_handler(). Assign it directly in ipa_interrupt_config() as well. This makes it unnecessary to do this in ipa_interrupt_add(). Make similar changes for removing IPA interrupt handlers. The next two patches will finish the cleanup, removing the add/remove functions and the handler array entirely. Signed-off-by:
-
Alex Elder authored
Expose ipa_interrupt_enable() and have functions that register IPA interrupt handlers enable them directly, rather than having the registration process do that. Do the same for disabling IPA interrupt handlers. Signed-off-by:
-
Alex Elder authored
Create new function ipa_interrupt_enable() to encapsulate enabling one of the IPA interrupt types. Introduce ipa_interrupt_disable() to reverse that operation. Add a helper function to factor out the common register update used by both. Use these in ipa_interrupt_add() and ipa_interrupt_remove(). Signed-off-by:
-
Alex Elder authored
The prototype for an IPA interrupt handler supplies the IPA interrupt ID, so it's possible to use a single function to handle any type of microcontroller interrupt. Introduce ipa_uc_interrupt_handler(), which calls the event or the response handler depending on the IRQ ID provided. Register the new function as the handler for both microcontroller IPA interrupt types. The called functions don't use their "irq_id" arguments, so remove them. Signed-off-by:
-
