| Message ID | 20200827220114.69225-2-alexei.starovoitov@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Introduce minimal support for sleepable progs | expand |
On 8/27/20 6:01 PM, 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. > > Fixes: af3b854492f3 ("mm/page_alloc.c: allow error injection") > Fixes: cfcbfb1382db ("mm/filemap.c: enable error injection at add_to_page_cache()") > Signed-off-by: Alexei Starovoitov <ast@kernel.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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... Thanks, Daniel
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.
diff --git a/mm/filemap.c b/mm/filemap.c index 1aaea26556cc..054d93a86f8a 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -827,10 +827,10 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask) } EXPORT_SYMBOL_GPL(replace_page_cache_page); -static int __add_to_page_cache_locked(struct page *page, - struct address_space *mapping, - pgoff_t offset, gfp_t gfp_mask, - void **shadowp) +noinline int __add_to_page_cache_locked(struct page *page, + struct address_space *mapping, + pgoff_t offset, gfp_t gfp_mask, + void **shadowp) { XA_STATE(xas, &mapping->i_pages, offset); int huge = PageHuge(page); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 0e2bab486fea..cd8d8f0326fc 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3477,7 +3477,7 @@ static inline bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) #endif /* CONFIG_FAIL_PAGE_ALLOC */ -static noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) +noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) { return __should_fail_alloc_page(gfp_mask, order); }