Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Uladzislau Rezki <urezki@gmail.com>
Date:   Mon, 15 Mar 2021 19:02:39 +0100
To:     Topi Miettinen <toiwoton@gmail.com>
Cc:     Uladzislau Rezki <urezki@gmail.com>,
        linux-hardening@vger.kernel.org, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andy Lutomirski <luto@kernel.org>,
        Jann Horn <jannh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Linux API <linux-api@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH v4] mm/vmalloc: randomize vmalloc() allocations
Message-ID: <20210315180239.GA2117@pc638.lan>
References: <20210309135757.5406-1-toiwoton@gmail.com>
 <20210314172312.GA2085@pc638.lan>
 <ba975607-9493-c78c-bbd8-6a85573114d1@gmail.com>
 <20210315153510.GA1865@pc638.lan>
 <4649f69d-b7cd-d1a6-26e0-9b8bf3b17df5@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4649f69d-b7cd-d1a6-26e0-9b8bf3b17df5@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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.

> +
>         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