netdev - Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier

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: <CAMEtUuwNL2ngOfMA7HMc6agRnSwYMTkvEqxoy_AFwDVnb-o_Ug@mail.gmail.com>
Date:	Wed, 2 Jul 2014 15:43:26 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	Daniel Borkmann <dborkman@...hat.com>,
	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Chema Gonzalez <chema@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Jiri Olsa <jolsa@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kees Cook <keescook@...omium.org>,
	Linux API <linux-api@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier

On Wed, Jul 2, 2014 at 1:11 AM, David Laight <David.Laight@...lab.com> wrote:
> From: Alexei Starovoitov
> ...
>> >> +#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })
> ...
>> >> +       _(get_map_info(env, map_id, &map));
>> >
>> > Nit: such macros should be removed, please.
>>
>> It may surely look unconventional, but alternative is to replace
>> every usage of _ macro with:
>> err =
>> if (err)
>>   return err;
>>
>> and since this macro is used 38 times, it will add ~120 unnecessary
>> lines that will only make code much harder to follow.
>> I tried not using macro and results were not pleasing.
>
> The problem is that they are hidden control flow.
> As such they make flow analysis harder for the casual reader.

In the abstract context macros with gotos and returns are bad,
but in this case extra verbosity is the bigger evil.

Consider this piece of code:
#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })

if (opcode == BPF_END || opcode == BPF_NEG) {
    if (BPF_SRC(insn->code) != BPF_X)
        return -EINVAL;

    /* check src operand */
     _(check_reg_arg(regs, insn->dst_reg, 1));

    /* check dest operand */
    _(check_reg_arg(regs, insn->dst_reg, 0));

} else if (opcode == BPF_MOV) {

    if (BPF_SRC(insn->code) == BPF_X)
        /* check src operand */
        _(check_reg_arg(regs, insn->src_reg, 1));

    /* check dest operand */
    _(check_reg_arg(regs, insn->dst_reg, 0));

where casual reader can easily see what the purpose of the code
and what it's doing.

Now rewrite it without '_' macro:
if (opcode == BPF_END || opcode == BPF_NEG) {
    if (BPF_SRC(insn->code) != BPF_X)
            return -EINVAL;

    /* check src operand */
    err = check_reg_arg(regs, insn->dst_reg, 1);
    if (err)
        return err;

    /* check dest operand */
    err = check_reg_arg(regs, insn->dst_reg, 0);
    if (err)
        return err;

} else if (opcode == BPF_MOV) {

    if (BPF_SRC(insn->code) == BPF_X) {
        /* check src operand */
        err = check_reg_arg(regs, insn->src_reg, 1);
        if (err)
            return err;
    }

    /* check dest operand */
    err = check_reg_arg(regs, insn->dst_reg, 0);
    if (err)
        return err;

see how your eyes are now picking up endless control flow of
if conditions and returns, instead of focusing on the code itself.
It's much easier to understand the semantics when if (err) is out
of the way. Note that replacing _ with real name will ruin
the reading experience, since CAPITAL letters of the macro
will be screaming: "look at me", instead of letting reviewer
focus on the code.

I believe that this usage of _ as a macro specifically as defined,
would be a great addition to kernel coding style in general.
I don't want to see _ to be redefined differently.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html