| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Jul 2023 00:20:22 +0800
From: Leon Hwang <hffilwlqm@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc: john.fastabend@...il.com, andrii@...nel.org, martin.lau@...ux.dev,
song@...nel.org, yhs@...com, kpsingh@...nel.org, sdf@...gle.com,
haoluo@...gle.com, jolsa@...nel.org, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, hawk@...nel.org,
tangyeechou@...il.com, kernel-patches-bot@...com, bpf@...r.kernel.org,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Introduce bpf generic log
On 2023/7/5 22:39, Daniel Borkmann wrote:
> 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@...il.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?
Yes. It's possible to reuse the bpf_verifier_log infra and reuse bpf_log()
helper. I'll try this way:
#define BPF_LOG_USER BPF_LOG_LEVEL1 /* user log flag */
#define BPF_ULOG_WRITE(log_buf, log_size, fmt, ...) do { \
const char *____fmt = (fmt); \
struct bpf_verifier_log ____vlog; \
bpf_vlog_init(&____vlog, BPF_LOG_USER, log_buf, log_size); \
bpf_log(&____vlog, ____fmt,
##__VA_ARGS__); \
} while (0)
>
>> #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.
Got it. I'll change it to avoid breaking user space.
>
>> 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 {
>>
>
Powered by blists - more mailing lists