| Message ID | 20190619011450.28048-1-aarcange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] fork,memcg: alloc_thread_stack_node needs to set tsk->stack | expand |
On Tue, 2019-06-18 at 21:14 -0400, Andrea Arcangeli wrote: > Commit 5eed6f1dff87bfb5e545935def3843edf42800f2 corrected two > instances, but there was a third instance of this bug. > > Without setting tsk->stack, if memcg_charge_kernel_stack fails, it'll > execute free_thread_stack() on a dangling pointer. > > Enterprise kernels are compiled with VMAP_STACK=y so this isn't > critical, but custom VMAP_STACK=n builds should have some performance > advantage, with the drawback of risking to fail fork because > compaction didn't succeed. So as long as VMAP_STACK=n is a supported > option it's worth fixing it upstream. > > Fixes: 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> Reviewed-by: Rik van Riel <riel@surriel.com>
Acked-by: Roman Gushchin <guro@fb.com> Thank you, Andrea! > On Jun 18, 2019, at 18:15, Andrea Arcangeli <aarcange@redhat.com> wrote: > > Commit 5eed6f1dff87bfb5e545935def3843edf42800f2 corrected two > instances, but there was a third instance of this bug. > > Without setting tsk->stack, if memcg_charge_kernel_stack fails, it'll > execute free_thread_stack() on a dangling pointer. > > Enterprise kernels are compiled with VMAP_STACK=y so this isn't > critical, but custom VMAP_STACK=n builds should have some performance > advantage, with the drawback of risking to fail fork because > compaction didn't succeed. So as long as VMAP_STACK=n is a supported > option it's worth fixing it upstream. > > Fixes: 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> > --- > kernel/fork.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index d6c324b1b29e..9ee28dfe7c21 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -248,7 +248,11 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node) > struct page *page = alloc_pages_node(node, THREADINFO_GFP, > THREAD_SIZE_ORDER); > > - return page ? page_address(page) : NULL; > + if (likely(page)) { > + tsk->stack = page_address(page); > + return tsk->stack; > + } > + return NULL; > #endif > } > >
On Tue 18-06-19 21:14:50, Andrea Arcangeli wrote: > Commit 5eed6f1dff87bfb5e545935def3843edf42800f2 corrected two > instances, but there was a third instance of this bug. Sigh. I should have noticed when reviewing the above. My bad. > Without setting tsk->stack, if memcg_charge_kernel_stack fails, it'll > execute free_thread_stack() on a dangling pointer. > > Enterprise kernels are compiled with VMAP_STACK=y so this isn't > critical, but custom VMAP_STACK=n builds should have some performance > advantage, with the drawback of risking to fail fork because > compaction didn't succeed. So as long as VMAP_STACK=n is a supported > option it's worth fixing it upstream. > > Fixes: 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> Acked-by: Michal Hocko <mhocko@suse.com> I have double checked now and it seems we have covered all the cases finally. Thanks! > --- > kernel/fork.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index d6c324b1b29e..9ee28dfe7c21 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -248,7 +248,11 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node) > struct page *page = alloc_pages_node(node, THREADINFO_GFP, > THREAD_SIZE_ORDER); > > - return page ? page_address(page) : NULL; > + if (likely(page)) { > + tsk->stack = page_address(page); > + return tsk->stack; > + } > + return NULL; > #endif > } >
diff --git a/kernel/fork.c b/kernel/fork.c index d6c324b1b29e..9ee28dfe7c21 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -248,7 +248,11 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node) struct page *page = alloc_pages_node(node, THREADINFO_GFP, THREAD_SIZE_ORDER); - return page ? page_address(page) : NULL; + if (likely(page)) { + tsk->stack = page_address(page); + return tsk->stack; + } + return NULL; #endif }
Commit 5eed6f1dff87bfb5e545935def3843edf42800f2 corrected two instances, but there was a third instance of this bug. Without setting tsk->stack, if memcg_charge_kernel_stack fails, it'll execute free_thread_stack() on a dangling pointer. Enterprise kernels are compiled with VMAP_STACK=y so this isn't critical, but custom VMAP_STACK=n builds should have some performance advantage, with the drawback of risking to fail fork because compaction didn't succeed. So as long as VMAP_STACK=n is a supported option it's worth fixing it upstream. Fixes: 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> --- kernel/fork.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)