Re: [PATCH bpf-next 2/6] bpf: Add syscall common attrs support for xdp link_create

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]

> 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