> bpf: Add syscall common attrs support for xdp link_create
TL;DR:
- bpf_log(log, extack._msg) in bpf_xdp_link_attach() may dereference NULL when
no extack message is set
- bpf_log() may be an undefined reference when CONFIG_BPF_SYSCALL=n
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 18dc88990510..fdc1de21f358 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[ ... ]
> @@ -10784,6 +10786,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr,
> struct bpf_prog *prog)
> dev = dev_get_by_index(net, attr->link_create.target_ifindex);
> if (!dev) {
> rtnl_unlock();
> + bpf_log(log, "Invalid target_ifindex.\n");
> return -EINVAL;
> }
>
Can this fail to link when CONFIG_BPF_SYSCALL is not set?
bpf_xdp_link_attach() is built whenever CONFIG_NET=y, but bpf_log() lives in
kernel/bpf/log.c, which kernel/bpf/Makefile only builds under
CONFIG_BPF_SYSCALL:
obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o ... log.o ...
include/linux/bpf_verifier.h declares bpf_log() with no stub for the
!CONFIG_BPF_SYSCALL case, and CONFIG_NET only does "select BPF". A config
with CONFIG_NET=y and CONFIG_BPF_SYSCALL=n looks like it would end with an
undefined reference to bpf_log from both of the new calls in this function.
include/linux/bpf.h provides static inline stubs for bpf_link_init(),
bpf_link_prime(), bpf_link_settle() and bpf_link_cleanup() so that this
function builds without the bpf() syscall.
Should bpf_log() get a similar stub, or should the calls be guarded with
IS_ENABLED(CONFIG_BPF_SYSCALL)? None of the later patches in the series add
a stub or change the Makefile.
[ ... ]
> @@ -10812,6 +10815,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr,
> struct bpf_prog *prog)
> if (err) {
> link->dev = NULL;
> bpf_link_cleanup(&link_primer);
> + bpf_log(log, extack._msg);
> trace_bpf_xdp_link_attach_failed(extack._msg);
> goto out_put_dev;
> }
Can extack._msg be NULL here?
extack is zero-initialised (struct netlink_ext_ack extack = {};), and _msg
stays NULL whenever dev_xdp_attach_link() fails without anyone calling
NL_SET_ERR_MSG(). Many ndo_bpf implementations do exactly that. For
example, ixgbe_xdp() -> ixgbe_xdp_setup() has several -EINVAL and -ENOMEM
returns that don't set an extack message, and mlx5e_xdp_set() ->
mlx5e_xdp_allowed() returns -EINVAL with only a netdev_warn().
If the caller passed BPF_COMMON_ATTRS with log_level != 0
(bpf_verifier_log_attr_valid() accepts log_level=1 even with a NULL log_buf
and a log_size of 0), bpf_verifier_log_needed() is true and the call chain
becomes:
bpf_log(log, NULL)
-> bpf_verifier_vlog(log, NULL, args)
-> vscnprintf(log->kbuf, BPF_VERIFIER_TMP_LOG_SIZE, NULL, args)
-> vsnprintf() /* while (*fmt.str) with fmt.str == NULL */
which dereferences NULL. The tracepoint on the next line copes with NULL
because __assign_str() substitutes a placeholder string, and the netlink ack
path also checks extack->_msg, so this new bpf_log() call looks like the only
consumer without a NULL check.
Passing the message as the format string also means a '%' in an extack
message would be interpreted as a conversion, and unlike the other bpf_log()
messages in link_create(), this one has no trailing newline. Would
something like this be better?
if (extack._msg)
bpf_log(log, "%s\n", extack._msg);
This isn't fixed later in the series. Commit 2a63a314e533 ("bpf: Retire
tracepoint bpf_xdp_link_attach_failed") removes the tracepoint but keeps
bpf_log(log, extack._msg), so no NULL-safe consumer is left on this path.
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39042048581
