Re: [RFC PATCH] mm/vmalloc: fix vmalloc which may return null if called with __GFP_NOFAIL

[Barry Song <21cnbao@gmail.com>]

On Thu, May 9, 2024 at 2:39 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
> Hi,
>
> On 2024/5/9 10:20, Barry Song wrote:
> > On Thu, May 9, 2024 at 12:58 AM <hailong.liu@oppo.com> wrote:
> >>
> >> From: "Hailong.Liu" <hailong.liu@oppo.com>
> >>
> >> Commit a421ef303008 ("mm: allow !GFP_KERNEL allocations for kvmalloc")
> >> includes support for __GFP_NOFAIL, but it presents a conflict with
> >> commit dd544141b9eb ("vmalloc: back off when the current task is
> >> OOM-killed"). A possible scenario is as belows:
> >>
> >> process-a
> >> kvcalloc(n, m, GFP_KERNEL | __GFP_NOFAIL)
> >>      __vmalloc_node_range()
> >>          __vmalloc_area_node()
> >>              vm_area_alloc_pages()
> >>              --> oom-killer send SIGKILL to process-a
> >>              if (fatal_signal_pending(current)) break;
> >> --> return NULL;
> >>
> >> to fix this, do not check fatal_signal_pending() in vm_area_alloc_pages()
> >> if __GFP_NOFAIL set.
> >>
> >> Reported-by: Oven <liyangouwen1@oppo.com>
> >> Signed-off-by: Hailong.Liu <hailong.liu@oppo.com>
> >> ---
> >>   mm/vmalloc.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >> index 6641be0ca80b..2f359d08bf8d 100644
> >> --- a/mm/vmalloc.c
> >> +++ b/mm/vmalloc.c
> >> @@ -3560,7 +3560,7 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
> >>
> >>          /* High-order pages or fallback path if "bulk" fails. */
> >>          while (nr_allocated < nr_pages) {
> >> -               if (fatal_signal_pending(current))
> >> +               if (!(gfp & __GFP_NOFAIL) && fatal_signal_pending(current))
> >>                          break;
> >
> > why not !nofail ?
> >
> > This seems a correct fix, but it undermines the assumption made in
> > commit dd544141b9eb
> >   ("vmalloc: back off when the current task is OOM-killed")
> >
> > "
> >      This may trigger some hidden problems, when caller does not handle
> >      vmalloc failures, or when rollaback after failed vmalloc calls own
> >      vmallocs inside.  However all of these scenarios are incorrect: vmalloc
> >      does not guarantee successful allocation, it has never been called with
> >      __GFP_NOFAIL and threfore either should not be used for any rollbacks or
> >      should handle such errors correctly and not lead to critical failures.
> > "
> >
> > If a significant kvmalloc operation is performed with the NOFAIL flag, it risks
> > reverting the fix intended to address the OOM-killer issue in commit
> > dd544141b9eb.
> > Should we indeed permit the NOFAIL flag for large kvmalloc allocations?
>
> Just from my perspective, I don't really care about kmalloc, vmalloc
> or kvmalloc (__GFP_NOFAIL).  I even don't care if it returns three
> order-0 pages or a high-order page.   I just would like to need a
> virtual consecutive buffer (even it works slowly.) with __GFP_NOFAIL.
>
> Because in some cases, writing fallback code may be tough and hard to
> test if such fallback path is correct since it only triggers in extreme
> workloads, and even such buffers are just used in a very short lifetime.
> Also see other FS discussion of __GFP_NOFAIL, e.g.
> https://lore.kernel.org/all/ZcUQfzfQ9R8X0s47@tiehlicka/
>
> In the worst cases, it usually just needs < 5 order-0 pages (for many
> cases it only needs one page), but with kmalloc it will trigger WARN
> if it occurs to > order-1 allocation. as I mentioned before.
>
> With my limited understanding I don't see why it could any problem with
> kvmalloc(__GFP_NOFAIL) since it has no difference of kmalloc(GFP_NOFAIL)
> with order-0 allocation.

I completely understand that you're not concerned about the origin of
the memory,
such as whether it's organized by all zero-order pages. However, in the event
that someone else allocates a large memory, like several megabytes with the
NOFAIL flag, commit dd544141b9eb aims to halt the allocation before success
if the process being allocated is targeted for termination of OOM-killer.

With the current patch, we miss the opportunity for early allocation
termination.
However, if the size of the kvmalloc() is small, as in your case, I
believe it should
be perfectly fine. but do we have any way to prevent large size allocation with
NOFAIL?

>
>
> Thanks,
> Gao XIang

Thanks
Barry