Re: [PATCH v4] mm/vmalloc: randomize vmalloc() allocations

[Topi Miettinen]

On 15.3.2021 20.02, Uladzislau Rezki wrote:
> On Mon, Mar 15, 2021 at 06:23:37PM +0200, Topi Miettinen wrote:
> > On 15.3.2021 17.35, Uladzislau Rezki wrote:
> > > > On 14.3.2021 19.23, Uladzislau Rezki wrote:
> > > > > Also, using vmaloc test driver i can trigger a kernel BUG:
> > > > >
> > > > > <snip>
> > > > > [   24.627577] kernel BUG at mm/vmalloc.c:1272!
> > > >
> > > > It seems that most tests indeed fail. Perhaps the vmalloc subsystem isn't
> > > > very robust in face of fragmented virtual memory. What could be done to fix
> > > > that?
> > > Your patch is broken in context of checking "vend" when you try to
> > > allocate next time after first attempt. Passed "vend" is different
> > > there comparing what is checked later to figure out if an allocation
> > > failed or not:
> > >
> > > <snip>
> > >       if (unlikely(addr == vend))
> > >           goto overflow;
> > > <snip>
> >
> >
> > Thanks, I'll fix that.
> >
> > > > In this patch, I could retry __alloc_vmap_area() with the whole region after
> > > > failure of both [random, vend] and [vstart, random] but I'm not sure that
> > > > would help much. Worth a try of course.
> > > There is no need in your second [vstart, random]. If a first bigger range
> > > has not been successful, the smaller one will never be success anyway. The
> > > best way to go here is to repeat with real [vsart:vend], if it still fails
> > > on a real range, then it will not be possible to accomplish an allocation
> > > request with given parameters.
> > >
> > > > By the way, some of the tests in test_vmalloc.c don't check for vmalloc()
> > > > failure, for example in full_fit_alloc_test().
> > > Where?
> >
> > Something like this:
> >
> > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> > index 5cf2fe9aab9e..27e5db9a96b4 100644
> > --- a/lib/test_vmalloc.c
> > +++ b/lib/test_vmalloc.c
> > @@ -182,9 +182,14 @@ static int long_busy_list_alloc_test(void)
> >          if (!ptr)
> >                  return rv;
> >
> > -       for (i = 0; i < 15000; i++)
> > +       for (i = 0; i < 15000; i++) {
> >                  ptr[i] = vmalloc(1 * PAGE_SIZE);
> >
> > +               if (!ptr[i])
> > +                       goto leave;
> > +       }
> > +
> Hmm. That is for creating a long list of allocated areas before running
> a test. For example if one allocation among 15 000 fails, some index will
> be set to NULL. Later on after "leave" label vfree() will bypass NULL freeing.
>
> Either we have 15 000 extra elements or 10 000 does not really matter
> and is considered as a corner case that is probably never happens. Yes,
> you can simulate such precondition, but then a regular vmalloc()s will
> likely also fails, thus the final results will be screwed up.

I'd argue that if the allocations fail, the test should be aborted 
immediately since the results are not representative.

-Topi

> > +
> >          for (i = 0; i < test_loop_count; i++) {
> >                  ptr_1 = vmalloc(100 * PAGE_SIZE);
> >                  if (!ptr_1)
> > @@ -236,7 +241,11 @@ static int full_fit_alloc_test(void)
> >
> >          for (i = 0; i < junk_length; i++) {
> >                  ptr[i] = vmalloc(1 * PAGE_SIZE);
> > +               if (!ptr[i])
> > +                       goto error;
> >                  junk_ptr[i] = vmalloc(1 * PAGE_SIZE);
> > +               if (!junk_ptr[i])
> > +                       goto error;
> >          }
> >
> >          for (i = 0; i < junk_length; i++)
> > @@ -256,8 +265,10 @@ static int full_fit_alloc_test(void)
> >          rv = 0;
> >
> >   error:
> > -       for (i = 0; i < junk_length; i++)
> > +       for (i = 0; i < junk_length; i++) {
> >                  vfree(ptr[i]);
> > +               vfree(junk_ptr[i]);
> > +       }
> >
> >          vfree(ptr);
> >          vfree(junk_ptr);
> Same here.
>
> --
> Vlad Rezki