All of lore.kernel.org
 help / color / mirror / Atom feed
From: Edward Cree <ecree@solarflare.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>, Yonghong Song <yhs@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Jiri Olsa <jolsa@redhat.com>, Martin Lau <kafai@fb.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Linux Networking Development Mailing List
	<netdev@vger.kernel.org>
Subject: Re: Help with the BPF verifier
Date: Thu, 1 Nov 2018 20:05:07 +0000	[thread overview]
Message-ID: <deb1d2a9-25cb-da1f-32b0-53a7b07eb4ad@solarflare.com> (raw)
In-Reply-To: <20181101185217.GA20495@kernel.org>

On 01/11/18 18:52, Arnaldo Carvalho de Melo wrote:
>  R0=inv(id=0) R1=inv6 R2=inv6 R3=inv(id=0) R6=ctx(id=0,off=0,imm=0) R7=inv64 R10=fp0,call_-1
> 15: (b7) r2 = 0
> 16: (63) *(u32 *)(r10 -260) = r2
> 17: (67) r1 <<= 32
> 18: (77) r1 >>= 32
> 19: (67) r1 <<= 3
> 20: (bf) r2 = r6
> 21: (0f) r2 += r1
> 22: (79) r3 = *(u64 *)(r2 +16)
> R2 invalid mem access 'inv'
I wonder if you could run this with verifier log level 2?  (I'm not sure how
 you would go about plumbing that through the perf tooling.)  It seems very
 odd that it ends up with R2=inv, and I'm wondering whether R1 becomes unknown
 during the shifts or whether the addition in insn 21 somehow produces the
 unknown-ness.  (I know we used to have a thing[1] where doing ptr += K and
 then also having an offset in the LDX produced an error about
 ptr+const+const, but that seems to have been fixed at some point.)

Note however that even if we get past this, R1 at this point holds 6, so it
 looks like the verifier is walking the impossible path where we're inside the
 'if' even though filename_arg = 6.  This is a (slightly annoying) verifier
 limitation, that it walks paths with impossible combinations of constraints
 (we've previously had cases where assertions in the verifier would blow up
 because of this, e.g. registers with max_val less than min_val).  So if the
 check_ctx_access() is going to worry about whether you're off the end of the
 array (I'm not sure what your program type is and thus which is_valid_access
 callback is involved), then it'll complain about this.
If filename_arg came from some external source you'd have a different
 problem, because then it would have a totally unknown value, that on entering
 the 'if' becomes "unknown but < 6", which is still too variable to have as
 the offset of a ctx access.  Those have to be at a known constant offset, so
 that we can determine the type of the returned value.

As a way to fix this, how about [UNTESTED!]:
    const void *filename_arg = NULL;
    /* ... */
    switch (augmented_args.args.syscall_nr) {
        case SYS_OPEN: filename_arg = args->args[0]; break;
        case SYS_OPENAT: filename_arg = args->args[1]; break;
    }
    /* ... */
    if (filename_arg) {
        /* stuff */
        blah = probe_read_str(/* ... */, filename_arg);
    } else {
        /* the other stuff */
    }
That way, you're only ever dealing in constant pointers (although judging by
 an old thread I found[1] about ptr+const+const, the compiler might decide to
 make some optimisations that end up looking like your existing code).

As for what you want to do with the index coming from userspace, the verifier
 will not like that at all, as mentioned above, so I think you'll need to do
 something like:
    switch (filename_arg_from_userspace) {
        case 0: filename_arg = args->args[0]; break;
        case 1: filename_arg = args->args[1]; break;
        /* etc */
        default: filename_arg = NULL;
    }
 thus ensuring that you only ever have ctx pointers with constant offsets.

-Ed

[1]: https://lists.iovisor.org/g/iovisor-dev/topic/21386327#1302

  parent reply	other threads:[~2018-11-02  5:09 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-01 18:52 Help with the BPF verifier Arnaldo Carvalho de Melo
2018-11-01 19:10 ` David Miller
2018-11-01 19:13   ` Arnaldo Carvalho de Melo
2018-11-01 19:28     ` David Miller
2018-11-01 20:05 ` Edward Cree [this message]
2018-11-02 15:02   ` Arnaldo Carvalho de Melo
2018-11-02 15:42     ` Edward Cree
2018-11-02 21:27       ` Yonghong Song
2018-11-05 12:33         ` Arnaldo Carvalho de Melo
2018-11-03 11:29       ` Arnaldo Carvalho de Melo
2018-11-03 11:32         ` Arnaldo Carvalho de Melo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=deb1d2a9-25cb-da1f-32b0-53a7b07eb4ad@solarflare.com \
    --to=ecree@solarflare.com \
    --cc=acme@kernel.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=daniel@iogearbox.net \
    --cc=jolsa@redhat.com \
    --cc=kafai@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.