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

[Leon Hwang <leon.hwang@linux.dev>]

On 2026/10/17 23:03, bot+bpf-ci@kernel.org 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

> [...]