All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexei Starovoitov <ast@fb.com>
To: Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	<davem@davemloft.net>
Cc: <josef@toxicpanda.com>, <bpoirier@suse.com>,
	<akpm@linux-foundation.org>, <hannes@cmpxchg.org>,
	<netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
	<kernel-team@fb.com>
Subject: Re: [PATCH v3 bpf-next 1/5] mm/error_inject: Fix allow_error_inject function signatures.
Date: Sat, 29 Aug 2020 15:47:02 -0700	[thread overview]
Message-ID: <25d82549-c793-6e2d-d7ed-1dfb3f77e447@fb.com> (raw)
In-Reply-To: <d6eae293-5427-d5e4-73aa-4df7a493bb89@iogearbox.net>

On 8/28/20 1:27 PM, Daniel Borkmann wrote:
> On 8/28/20 12:01 AM, Alexei Starovoitov wrote:
>> From: Alexei Starovoitov <ast@kernel.org>
>>
>> 'static' and 'static noinline' function attributes make no guarantees 
>> that
>> gcc/clang won't optimize them. The compiler may decide to inline 'static'
>> function and in such case ALLOW_ERROR_INJECT becomes meaningless. The 
>> compiler
>> could have inlined __add_to_page_cache_locked() in one callsite and 
>> didn't
>> inline in another. In such case injecting errors into it would cause
>> unpredictable behavior. It's worse with 'static noinline' which won't be
>> inlined, but it still can be optimized. Like the compiler may decide 
>> to remove
>> one argument or constant propagate the value depending on the callsite.
>>
>> To avoid such issues make sure that these functions are global noinline.
> 
> Back in the days when adding 6bf37e5aa90f ("crypto: crypto_memneq - add 
> equality
> testing of memory regions w/o timing leaks") we added noinline, but also an
> explicit EXPORT_SYMBOL() to prevent this from being optimized away; I 
> wonder
> whether ALLOW_ERROR_INJECT() should have something implicit here too to 
> prevent
> from optimization .. otoh we probably don't want to expose every 
> ALLOW_ERROR_INJECT()
> function also to modules generically...

I don't quite follow the concern.
EXPORT_SYMBOL() only takes the address of the function.
Just like ALLOW_ERROR_INJECT() also takes the address.
Taking the address doesn't prevent optimizations.
The compiler is free to inline the function, but it can keep an
extra function body with the address pointing there.
Also ALLOW_ERROR_INJECT() doesn't make the symbol available to modules.

  reply	other threads:[~2020-08-29 22:47 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 22:01 [PATCH v3 bpf-next 0/5] bpf: Introduce minimal support for sleepable progs Alexei Starovoitov
2020-08-27 22:01 ` [PATCH v3 bpf-next 1/5] mm/error_inject: Fix allow_error_inject function signatures Alexei Starovoitov
2020-08-27 23:58   ` Josef Bacik
2020-08-28 20:27   ` Daniel Borkmann
2020-08-29 22:47     ` Alexei Starovoitov [this message]
2020-08-27 22:01 ` [PATCH v3 bpf-next 2/5] bpf: Introduce sleepable BPF programs Alexei Starovoitov
2020-08-27 22:28   ` KP Singh
2020-08-28  1:01   ` Josef Bacik
2020-08-31 14:51   ` Björn Töpel
2020-08-31 16:26     ` Alexei Starovoitov
2020-08-27 22:01 ` [PATCH v3 bpf-next 3/5] bpf: Add bpf_copy_from_user() helper Alexei Starovoitov
2020-08-27 22:29   ` KP Singh
2020-08-27 22:01 ` [PATCH v3 bpf-next 4/5] libbpf: support sleepable progs Alexei Starovoitov
2020-08-27 22:01 ` [PATCH v3 bpf-next 5/5] selftests/bpf: Add sleepable tests Alexei Starovoitov
2020-08-29 22:13   ` Yonghong Song
2020-08-29 22:38     ` Alexei Starovoitov
2020-08-30  0:22       ` Yonghong Song
2020-08-31 16:56         ` Alexei Starovoitov
2020-08-28 20:10 ` [PATCH v3 bpf-next 0/5] bpf: Introduce minimal support for sleepable progs Daniel Borkmann

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=25d82549-c793-6e2d-d7ed-1dfb3f77e447@fb.com \
    --to=ast@fb.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=bpoirier@suse.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=hannes@cmpxchg.org \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /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.