On 2026/10/17 23:03, [email protected] wrote:
>> 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.
Will add a stub bpf_log() when CONFIG_BPF_SYSCALL is not set.>
> [ ... ]
>
>> @@ -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);
Will change to this.
Thanks,
Leon
> [...]
