Re: [PATCH bpf-next] bpf: Pass extack to bpf_xdp_link_attach

[bot+bpf-ci@kernel.org]

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

> bpf: Pass extack to bpf_xdp_link_attach

TL;DR:
- link_create() now logs extack message even when XDP attach succeeds (e.g.
  vmxnet3 LRO), changing behaviour; success path doesn't finalize the log
- Nit: braces in column 0 in the new case block

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 87cd122d1024..26917a1ecbb2 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5944,8 +5944,14 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr, struct bpf_common_a
>  		break;
>  #ifdef CONFIG_NET
>  	case BPF_PROG_TYPE_XDP:
> -		ret = bpf_xdp_link_attach(attr, prog, log);
> +{

This isn't a bug, but the opening and closing braces of this new block scope
sit in column 0 on lines of their own inside the function body.  Could they
follow the usual 'case X: {' form instead?

	case BPF_PROG_TYPE_XDP: {
		struct netlink_ext_ack extack = {};

		ret = bpf_xdp_link_attach(attr, prog, &extack);
		if (extack._msg)
			bpf_log(log, "%s\n", extack._msg);
		break;
	}

A '{' or '}' in column 0 inside a function body can also confuse diff hunk
headers, ctags and vim's [[ motions.

> +		struct netlink_ext_ack extack = {};
> +
> +		ret = bpf_xdp_link_attach(attr, prog, &extack);
> +		if (extack._msg)
> +			bpf_log(log, "%s\n", extack._msg);

Is it intended that this now logs extack._msg regardless of the value of
ret?

Before this patch the copy sat inside the failure branch of
bpf_xdp_link_attach():

	if (err) {
		link->dev = NULL;
		bpf_link_cleanup(&link_primer);
		if (extack._msg)
			bpf_log(log, "%s\n", extack._msg);
		goto out_put_dev;
	}

Some ndo_bpf handlers set extack on a successful install.  For example,
vmxnet3_xdp_set() does:

	if (adapter->netdev->features & NETIF_F_LRO) {
		NL_SET_ERR_MSG_MOD(extack, "LRO is not supported with XDP");
		adapter->netdev->features &= ~NETIF_F_LRO;
	}

and then installs the program and returns 0.  The path is:

__sys_bpf()
  link_create()
    bpf_xdp_link_attach()
      dev_xdp_attach_link()
        dev_xdp_attach()
          dev_xdp_install()
            vmxnet3_xdp()
              vmxnet3_xdp_set()

On that path BPF_LINK_CREATE succeeds and returns a link fd, yet
"vmxnet3: LRO is not supported with XDP\n" is now copied into the user's
log_buf by bpf_verifier_vlog() through copy_to_user().  Before this patch
nothing was written on success.

The success path is also only partly handled.  link_create() calls
bpf_log_attr_finalize() only when ret < 0:

	out:
		if (ret < 0) {
			int err = bpf_log_attr_finalize(&attr_log, log);

so on success the text lands in the user buffer, but log_true_size is never
reported and the rotating-mode buffer is never linearized by
bpf_vlog_finalize().

The commit message only talks about transferring "the error message".  If
the old behaviour is wanted, should the condition be 'if (ret < 0 &&
extack._msg)'?  If logging warnings on success is intended, should the
commit message say so, and should the success path finalize the log as well?

>  		break;
> +}
>  	case BPF_PROG_TYPE_SCHED_CLS:
>  		if (attr->link_create.attach_type == BPF_TCX_INGRESS ||
>  		    attr->link_create.attach_type == BPF_TCX_EGRESS)


---
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/39627153683