Subject: Re: [PATCH v10 1/5] add infrastructure for tagging functions as error
 injectable
To: Masami Hiramatsu <mhiramat@kernel.org>,
        Josef Bacik <josef@toxicpanda.com>
References: <1513365176-6744-1-git-send-email-josef@toxicpanda.com>
 <1513365176-6744-2-git-send-email-josef@toxicpanda.com>
 <20171220200051.3d2adbde962e20f356b18338@kernel.org>
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>, <daniel@iogearbox.net>,
        <linux-btrfs@vger.kernel.org>, <darrick.wong@oracle.com>,
        Josef Bacik <jbacik@fb.com>
From: Alexei Starovoitov <ast@fb.com>
Message-ID: <285ac928-a7b0-b0a1-79cb-6fa5d4b24e52@fb.com>
Date: Wed, 20 Dec 2017 09:33:13 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20171220200051.3d2adbde962e20f356b18338@kernel.org>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2017 17:33:17.7658 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 8114ffe7-47ba-4658-69ad-08d547cfc2f1
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR15MB0963
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-20_07:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3805
Lines: 99

On 12/20/17 3:00 AM, Masami Hiramatsu wrote:
> On Fri, 15 Dec 2017 14:12:52 -0500
> Josef Bacik <josef@toxicpanda.com> wrote:
>
>> From: Josef Bacik <jbacik@fb.com>
>>
>> Using BPF we can override kprob'ed functions and return arbitrary
>> values.  Obviously this can be a bit unsafe, so make this feature opt-in
>> for functions.  Simply tag a function with KPROBE_ERROR_INJECT_SYMBOL in
>> order to give BPF access to that function for error injection purposes.
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> Acked-by: Ingo Molnar <mingo@kernel.org>
>> ---
>>  include/asm-generic/vmlinux.lds.h |  10 +++
>>  include/linux/bpf.h               |  11 +++
>>  include/linux/kprobes.h           |   1 +
>>  include/linux/module.h            |   5 ++
>>  kernel/kprobes.c                  | 163 ++++++++++++++++++++++++++++++++++++++
>>  kernel/module.c                   |   6 +-
>>  6 files changed, 195 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index ee8b707d9fa9..a2e8582d094a 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -136,6 +136,15 @@
>>  #define KPROBE_BLACKLIST()
>>  #endif
>>
>> +#ifdef CONFIG_BPF_KPROBE_OVERRIDE
>> +#define ERROR_INJECT_LIST()	. = ALIGN(8);						\
>> +				VMLINUX_SYMBOL(__start_kprobe_error_inject_list) = .;	\
>> +				KEEP(*(_kprobe_error_inject_list))			\
>> +				VMLINUX_SYMBOL(__stop_kprobe_error_inject_list) = .;
>> +#else
>> +#define ERROR_INJECT_LIST()
>> +#endif
>> +
>>  #ifdef CONFIG_EVENT_TRACING
>>  #define FTRACE_EVENTS()	. = ALIGN(8);					\
>>  			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
>> @@ -564,6 +573,7 @@
>>  	FTRACE_EVENTS()							\
>>  	TRACE_SYSCALLS()						\
>>  	KPROBE_BLACKLIST()						\
>> +	ERROR_INJECT_LIST()						\
>>  	MEM_DISCARD(init.rodata)					\
>>  	CLK_OF_TABLES()							\
>>  	RESERVEDMEM_OF_TABLES()						\
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index e55e4255a210..7f4d2a953173 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -576,4 +576,15 @@ extern const struct bpf_func_proto bpf_sock_map_update_proto;
>>  void bpf_user_rnd_init_once(void);
>>  u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
>>
>> +#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
>> +#ifdef CONFIG_BPF_KPROBE_OVERRIDE
>> +#define BPF_ALLOW_ERROR_INJECTION(fname)				\
>> +static unsigned long __used						\
>> +	__attribute__((__section__("_kprobe_error_inject_list")))	\
>> +	_eil_addr_##fname = (unsigned long)fname;
>> +#else
>> +#define BPF_ALLOW_ERROR_INJECTION(fname)
>> +#endif
>> +#endif
>
> This part shows this feature belongs to bpf, if it is a part of kprobes,
> it should be defined in include/asm-generic/kprobes.h as NOKPROBE_SYMBOL
> does.
>
> Why this is defined in BPF, but list is under kprobes?

because Ingo specifically requested that macro that marks the function
will be in bpf.h, so any .c file that starts adding such marks will
include that file instead of pulling stuff from kprobe.

>
> So there is no direct relationship with kprobe.
> For example, kprobe user modules can OVERRIDE any functions.
> And there is no generic error injection code in the kernel
> except for the bpf currently.

_currently_ is key word.

> Of course, I can accept this code if you accept that I make a
> generic error injection code on ftrace without BPF.

what stops other pieces of kernel to use the same technique?
The bpf verifier coupled together with opt-in
per-function marks via BPF_ALLOW_ERROR_INJECTION
give _safe_ way to do error injection.

I can imagine how you can hack kprobe text based interface to
use the same technique, but I suggest to wait and see how we
build on it in bpf land before replicating things in
pure kprobe land.