Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH bpf-next] bpf: Introduce bpf generic log
To:     Leon Hwang <hffilwlqm@gmail.com>, ast@kernel.org
Cc:     john.fastabend@gmail.com, andrii@kernel.org, martin.lau@linux.dev,
        song@kernel.org, yhs@fb.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        hawk@kernel.org, tangyeechou@gmail.com, kernel-patches-bot@fb.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
References: <20230705132058.46194-1-hffilwlqm@gmail.com>
From:   Daniel Borkmann <daniel@iogearbox.net>
Message-ID: <1a205a85-ebf2-6d90-468d-4fd63ce3dd0f@iogearbox.net>
Date:   Wed, 5 Jul 2023 16:39:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20230705132058.46194-1-hffilwlqm@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/5/23 3:20 PM, Leon Hwang wrote:
> Currently, excluding verifier, users are unable to obtain detailed error
> information when issues occur in BPF syscall.
> 
> To overcome this limitation, bpf generic log is introduced to provide
> error details similar to the verifier. This enhancement will enable the
> reporting of error details along with the corresponding errno in BPF
> syscall.
> 
> Essentially, bpf generic log functions as a mechanism similar to netlink,
> enabling the transmission of error messages to user space. This
> mechanism greatly enhances the usability of BPF syscall by allowing
> users to access comprehensive error messages instead of relying solely
> on errno.
> 
> This patch specifically addresses the error reporting in dev_xdp_attach()
> . With this patch, the error messages will be transferred to user space
> like the netlink approach. Hence, users will be able to check the error
> message along with the errno.
> 
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> ---
>   include/linux/bpf.h            | 30 ++++++++++++++++++++++++++++++
>   include/uapi/linux/bpf.h       |  6 ++++++
>   kernel/bpf/log.c               | 33 +++++++++++++++++++++++++++++++++
>   net/core/dev.c                 | 11 ++++++++++-
>   tools/include/uapi/linux/bpf.h |  6 ++++++
>   5 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f58895830..fd63f4a76 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3077,4 +3077,34 @@ static inline gfp_t bpf_memcg_flags(gfp_t flags)
>   	return flags;
>   }
>   
> +#define BPF_GENERIC_TMP_LOG_SIZE	256
> +
> +struct bpf_generic_log {
> +	char		kbuf[BPF_GENERIC_TMP_LOG_SIZE];
> +	char __user	*ubuf;
> +	u32		len_used;
> +	u32		len_total;
> +};
> +
> +__printf(2, 3) void bpf_generic_log_write(struct bpf_generic_log *log,
> +			const char *fmt, ...);
> +static inline void bpf_generic_log_init(struct bpf_generic_log *log,
> +			const struct bpf_generic_user_log *ulog)
> +{
> +	log->ubuf = (char __user *) (unsigned long) ulog->log_buf;
> +	log->len_total = ulog->log_size;
> +	log->len_used = 0;
> +}
> +
> +#define BPF_GENERIC_LOG_WRITE(log, ulog, fmt, ...)	do {	\
> +	const char *____fmt = (fmt);				\
> +	bpf_generic_log_init(log, ulog);			\
> +	bpf_generic_log_write(log, ____fmt, ##__VA_ARGS__);	\
> +} while (0)
> +
> +#define BPF_GENERIC_ULOG_WRITE(ulog, fmt, ...)	do {			\
> +	struct bpf_generic_log ____log;					\
> +	BPF_GENERIC_LOG_WRITE(&____log, ulog, fmt, ##__VA_ARGS__);	\
> +} while (0)
> +

Could we generalize the bpf_verifier_log infra and reuse bpf_log() helper
instead of adding something new?

>   #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 60a9d59be..34fa33493 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1318,6 +1318,11 @@ struct bpf_stack_build_id {
>   	};
>   };
>   
> +struct bpf_generic_user_log {
> +	__aligned_u64	log_buf;    /* user supplied buffer */
> +	__u32		log_size;   /* size of user buffer */
> +};
> +
>   #define BPF_OBJ_NAME_LEN 16U
>   
>   union bpf_attr {
> @@ -1544,6 +1549,7 @@ union bpf_attr {
>   		};
>   		__u32		attach_type;	/* attach type */
>   		__u32		flags;		/* extra flags */
> +		struct bpf_generic_user_log log; /* user log */

You cannot add this here, this breaks user space, you would have to
ad this under a xdp specific section inside the union.

>   		union {
>   			__u32		target_btf_id;	/* btf_id of target to attach to */
>   			struct {
> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> index 850494423..be56b153b 100644
> --- a/kernel/bpf/log.c
> +++ b/kernel/bpf/log.c
> @@ -325,3 +325,36 @@ __printf(2, 3) void bpf_log(struct bpf_verifier_log *log,
>   	va_end(args);
>   }
>   EXPORT_SYMBOL_GPL(bpf_log);
> +
> +static inline void __bpf_generic_log_write(struct bpf_generic_log *log, const char *fmt,
> +				      va_list args)
> +{
> +	unsigned int n;
> +
> +	n = vscnprintf(log->kbuf, BPF_GENERIC_TMP_LOG_SIZE, fmt, args);
> +
> +	WARN_ONCE(n >= BPF_GENERIC_TMP_LOG_SIZE - 1,
> +		  "bpf generic log truncated - local buffer too short\n");
> +
> +	n = min(log->len_total - log->len_used - 1, n);
> +	log->kbuf[n] = '\0';
> +
> +	if (!copy_to_user(log->ubuf + log->len_used, log->kbuf, n + 1))
> +		log->len_used += n;
> +	else
> +		log->ubuf = NULL;
> +}
> +
> +__printf(2, 3) void bpf_generic_log_write(struct bpf_generic_log *log,
> +				     const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	if (!log->ubuf || !log->len_total)
> +		return;
> +
> +	va_start(args, fmt);
> +	__bpf_generic_log_write(log, fmt, args);
> +	va_end(args);
> +}
> +EXPORT_SYMBOL_GPL(bpf_generic_log_write);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 69a3e5446..e933809c0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9409,12 +9409,20 @@ static const struct bpf_link_ops bpf_xdp_link_lops = {
>   	.update_prog = bpf_xdp_link_update,
>   };
>   
> +static inline void bpf_xdp_link_log(const union bpf_attr *attr, struct netlink_ext_ack *extack)
> +{
> +	const struct bpf_generic_user_log *ulog = &attr->link_create.log;
> +
> +	BPF_GENERIC_ULOG_WRITE(ulog, extack->_msg);
> +}
> +
>   int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>   {
>   	struct net *net = current->nsproxy->net_ns;
>   	struct bpf_link_primer link_primer;
>   	struct bpf_xdp_link *link;
>   	struct net_device *dev;
> +	struct netlink_ext_ack extack;
>   	int err, fd;
>   
>   	rtnl_lock();
> @@ -9440,12 +9448,13 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>   		goto unlock;
>   	}
>   
> -	err = dev_xdp_attach_link(dev, NULL, link);
> +	err = dev_xdp_attach_link(dev, &extack, link);
>   	rtnl_unlock();
>   
>   	if (err) {
>   		link->dev = NULL;
>   		bpf_link_cleanup(&link_primer);
> +		bpf_xdp_link_log(attr, &extack);
>   		goto out_put_dev;
>   	}

Agree that this is a useful facility to have and propagate back here.

> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 60a9d59be..34fa33493 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1318,6 +1318,11 @@ struct bpf_stack_build_id {
>   	};
>   };
>   
> +struct bpf_generic_user_log {
> +	__aligned_u64	log_buf;    /* user supplied buffer */
> +	__u32		log_size;   /* size of user buffer */
> +};
> +
>   #define BPF_OBJ_NAME_LEN 16U
>   
>   union bpf_attr {
> @@ -1544,6 +1549,7 @@ union bpf_attr {
>   		};
>   		__u32		attach_type;	/* attach type */
>   		__u32		flags;		/* extra flags */
> +		struct bpf_generic_user_log log; /* user log */
>   		union {
>   			__u32		target_btf_id;	/* btf_id of target to attach to */
>   			struct {
>