netdev - Re: [PATCH bpf-next 02/11] bpfilter: Add logging facility

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW4osuNOagPRwUB30tk3V=ECANktt9jzb+NK1mqOamouSQ@mail.gmail.com>
Date:   Wed, 19 May 2021 10:32:25 -0700
From:   Song Liu <song@...nel.org>
To:     Dmitrii Banshchikov <me@...que.spb.ru>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Networking <netdev@...r.kernel.org>, Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next 02/11] bpfilter: Add logging facility

On Tue, May 18, 2021 at 11:05 PM Dmitrii Banshchikov <me@...que.spb.ru> wrote:
>
> There are three logging levels for messages: FATAL, NOTICE and DEBUG.
> When a message is logged with FATAL level it results in bpfilter
> usermode helper termination.

Could you please explain why we choose to have 3 levels? Will we need
more levels,
like WARNING, ERROR, etc.?

>
> Introduce struct context to avoid use of global objects and store there
> the logging parameters: log level and log sink.
>
> Signed-off-by: Dmitrii Banshchikov <me@...que.spb.ru>
> ---
>  net/bpfilter/Makefile  |  2 +-
>  net/bpfilter/bflog.c   | 29 +++++++++++++++++++++++++++++
>  net/bpfilter/bflog.h   | 24 ++++++++++++++++++++++++
>  net/bpfilter/context.h | 16 ++++++++++++++++

Maybe combine bflog.h and context.h into one file? And bflog() can
probably fit in
that file too.

Thanks,
Song

>  4 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 net/bpfilter/bflog.c
>  create mode 100644 net/bpfilter/bflog.h
>  create mode 100644 net/bpfilter/context.h
>
> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
> index cdac82b8c53a..874d5ef6237d 100644
> --- a/net/bpfilter/Makefile
> +++ b/net/bpfilter/Makefile
> @@ -4,7 +4,7 @@
>  #
>
>  userprogs := bpfilter_umh
> -bpfilter_umh-objs := main.o
> +bpfilter_umh-objs := main.o bflog.o
>  userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi
>
>  ifeq ($(CONFIG_BPFILTER_UMH), y)
> diff --git a/net/bpfilter/bflog.c b/net/bpfilter/bflog.c
> new file mode 100644
> index 000000000000..2752e39060e4
> --- /dev/null
> +++ b/net/bpfilter/bflog.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include "bflog.h"
> +
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include "context.h"
> +
> +void bflog(struct context *ctx, int level, const char *fmt, ...)
> +{
> +       if (ctx->log_file &&
> +           (level == BFLOG_LEVEL_FATAL || (level & ctx->log_level))) {
> +               va_list va;
> +
> +               va_start(va, fmt);
> +               vfprintf(ctx->log_file, fmt, va);
> +               va_end(va);
> +       }
> +
> +       if (level == BFLOG_LEVEL_FATAL)
> +               exit(EXIT_FAILURE);
> +}
> diff --git a/net/bpfilter/bflog.h b/net/bpfilter/bflog.h
> new file mode 100644
> index 000000000000..4ed12791cfa1
> --- /dev/null
> +++ b/net/bpfilter/bflog.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#ifndef NET_BPFILTER_BFLOG_H
> +#define NET_BPFILTER_BFLOG_H
> +
> +struct context;
> +
> +#define BFLOG_IMPL(ctx, level, fmt, ...) bflog(ctx, level, "bpfilter: " fmt, ##__VA_ARGS__)
> +
> +#define BFLOG_LEVEL_FATAL (0)
> +#define BFLOG_LEVEL_NOTICE (1)
> +#define BFLOG_LEVEL_DEBUG (2)
> +
> +#define BFLOG_FATAL(ctx, fmt, ...)                                                                 \
> +       BFLOG_IMPL(ctx, BFLOG_LEVEL_FATAL, "fatal error: " fmt, ##__VA_ARGS__)
> +#define BFLOG_NOTICE(ctx, fmt, ...) BFLOG_IMPL(ctx, BFLOG_LEVEL_NOTICE, fmt, ##__VA_ARGS__)
> +#define BFLOG_DEBUG(ctx, fmt, ...) BFLOG_IMPL(ctx, BFLOG_LEVEL_DEBUG, fmt, ##__VA_ARGS__)
> +
> +void bflog(struct context *ctx, int level, const char *fmt, ...);
> +
> +#endif // NET_BPFILTER_BFLOG_H
> diff --git a/net/bpfilter/context.h b/net/bpfilter/context.h
> new file mode 100644
> index 000000000000..e85c97c3d010
> --- /dev/null
> +++ b/net/bpfilter/context.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#ifndef NET_BPFILTER_CONTEXT_H
> +#define NET_BPFILTER_CONTEXT_H
> +
> +#include <stdio.h>
> +
> +struct context {
> +       FILE *log_file;
> +       int log_level;
> +};
> +
> +#endif // NET_BPFILTER_CONTEXT_H
> --
> 2.25.1
>