Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <59FC9EC6.3060900@iogearbox.net>
Date:   Fri, 03 Nov 2017 17:52:22 +0100
From:   Daniel Borkmann <daniel@iogearbox.net>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To:     Josef Bacik <josef@toxicpanda.com>
CC:     rostedt@goodmis.org, mingo@redhat.com, davem@davemloft.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        ast@kernel.org, kernel-team@fb.com, Josef Bacik <jbacik@fb.com>
Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper
References: <1509633431-2184-1-git-send-email-josef@toxicpanda.com> <1509633431-2184-2-git-send-email-josef@toxicpanda.com> <59FBA64D.1050400@iogearbox.net> <20171103143135.bnlwu7hmtgmgjdri@destiny>
In-Reply-To: <20171103143135.bnlwu7hmtgmgjdri@destiny>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/03/2017 03:31 PM, Josef Bacik wrote:
> On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:
>> Hi Josef,
>>
>> one more issue I just noticed, see comment below:
>>
>> On 11/02/2017 03:37 PM, Josef Bacik wrote:
>> [...]
>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>>> index cdd78a7beaae..dfa44fd74bae 100644
>>> --- a/include/linux/filter.h
>>> +++ b/include/linux/filter.h
>>> @@ -458,7 +458,8 @@ struct bpf_prog {
>>>    				locked:1,	/* Program image locked? */
>>>    				gpl_compatible:1, /* Is filter GPL compatible? */
>>>    				cb_access:1,	/* Is control block accessed? */
>>> -				dst_needed:1;	/* Do we need dst entry? */
>>> +				dst_needed:1,	/* Do we need dst entry? */
>>> +				kprobe_override:1; /* Do we override a kprobe? */
>>>    	kmemcheck_bitfield_end(meta);
>>>    	enum bpf_prog_type	type;		/* Type of BPF program */
>>>    	u32			len;		/* Number of filter blocks */
>> [...]
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index d906775e12c1..f8f7927a9152 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>>>    			prog->dst_needed = 1;
>>>    		if (insn->imm == BPF_FUNC_get_prandom_u32)
>>>    			bpf_user_rnd_init_once();
>>> +		if (insn->imm == BPF_FUNC_override_return)
>>> +			prog->kprobe_override = 1;
>>>    		if (insn->imm == BPF_FUNC_tail_call) {
>>>    			/* If we tail call into other programs, we
>>>    			 * cannot make any assumptions since they can
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index 9660ee65fbef..0d7fce52391d 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
>>>    		return -EINVAL;
>>>    	}
>>>
>>> +	/* Kprobe override only works for kprobes, not uprobes. */
>>> +	if (prog->kprobe_override &&
>>> +	    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {
>>> +		bpf_prog_put(prog);
>>> +		return -EINVAL;
>>> +	}
>>
>> Can we somehow avoid the prog->kprobe_override flag here completely
>> and also same in the perf_event_attach_bpf_prog() handler?
>>
>> Reason is that it's not reliable for bailing out this way: Think of
>> the main program you're attaching doesn't use bpf_override_return()
>> helper, but it tail-calls into other BPF progs that make use of it
>> instead. So above check would be useless and will fail and we continue
>> to attach the prog for probes where it's not intended to be used.
>>
>> We've had similar issues in the past e.g. c2002f983767 ("bpf: fix
>> checking xdp_adjust_head on tail calls") is just one of those. Thus,
>> can we avoid the flag altogether and handle such error case differently?
>
> So if I'm reading this right there's no way to know what we'll tail call at any
> given point, so I need to go back to my previous iteration of this patch and
> always save the state of the kprobe in the per-cpu variable to make sure we
> don't use bpf_override_return in the wrong case?

Yeah.

> The tail call functions won't be in the BPF_PROG_ARRAY right?  It'll be just
> some other arbitrary function?  If that's the case then we really need something
> like this

With BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array
for the tracing/multiprog attach point? The program you're calling into
is inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time
and can have nesting as well.

> https://patchwork.kernel.org/patch/10034815/
>
> and I need to bring that back right?  Thanks,

I'm afraid so. The thing with skb cb_access which was brought up there is
that once you have a tail call in the prog you cannot make any assumptions
anymore, therefore the cb_access flag is set to 1 so we save/restore for
those cases precautionary since it could be accessed or not later on. In
your case I think this wouldn't work since legitimate bpf kprobes progs could
use tail calls today, so setting prog->kprobe_override there would prevent
attaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE
check.

From 1583055685102088614@xxx Fri Nov 03 14:32:28 +0000 2017
X-GM-THRID: 1582965462915356698
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread