Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Matthew Wilcox]

On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> >         int i;
> >
> >         for (i = 0; i < XBUFSIZE; i++) {
> > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > +                                                 order);
> 
> Is there a reason __GFP_COMP isn't automatically included in all page
> allocations? (Or rather, it seems like the exception is when things
> should NOT be considered part of the same allocation, so something
> like __GFP_SINGLE should exist?.)

The question is not whether or not things should be considered part of the
same allocation.  The question is whether the allocation is of a compound
page or of N consecutive pages.  Now you're asking what the difference is,
and it's whether you need to be able to be able to call compound_head(),
compound_order(), PageTail() or use a compound_dtor.  If you don't, then
you can save some time at allocation & free by not specifying __GFP_COMP.

I'll agree this is not documented well, and maybe most multi-page
allocations do want __GFP_COMP and we should invert that bit, but
__GFP_SINGLE doesn't seem like the right antonym to __GFP_COMP to me.

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Herbert Xu]

On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote:
> On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> > >         int i;
> > >
> > >         for (i = 0; i < XBUFSIZE; i++) {
> > > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > > +                                                 order);
> > 
> > Is there a reason __GFP_COMP isn't automatically included in all page
> > allocations? (Or rather, it seems like the exception is when things
> > should NOT be considered part of the same allocation, so something
> > like __GFP_SINGLE should exist?.)
> 
> The question is not whether or not things should be considered part of the
> same allocation.  The question is whether the allocation is of a compound
> page or of N consecutive pages.  Now you're asking what the difference is,
> and it's whether you need to be able to be able to call compound_head(),
> compound_order(), PageTail() or use a compound_dtor.  If you don't, then
> you can save some time at allocation & free by not specifying __GFP_COMP.

Thanks for clarifying Matthew.

Eric, this means that we should not use __GFP_COMP here just to
silent what is clearly a broken warning.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Matthew Wilcox]

On Mon, Apr 15, 2019 at 10:46:15AM +0800, Herbert Xu wrote:
> On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote:
> > On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> > > >         int i;
> > > >
> > > >         for (i = 0; i < XBUFSIZE; i++) {
> > > > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > > > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > > > +                                                 order);
> > > 
> > > Is there a reason __GFP_COMP isn't automatically included in all page
> > > allocations? (Or rather, it seems like the exception is when things
> > > should NOT be considered part of the same allocation, so something
> > > like __GFP_SINGLE should exist?.)
> > 
> > The question is not whether or not things should be considered part of the
> > same allocation.  The question is whether the allocation is of a compound
> > page or of N consecutive pages.  Now you're asking what the difference is,
> > and it's whether you need to be able to be able to call compound_head(),
> > compound_order(), PageTail() or use a compound_dtor.  If you don't, then
> > you can save some time at allocation & free by not specifying __GFP_COMP.
> 
> Thanks for clarifying Matthew.
> 
> Eric, this means that we should not use __GFP_COMP here just to
> silent what is clearly a broken warning.

I agree; if the crypto code is never going to try to go from the address of
a byte in the allocation back to the head page, then there's no need to
specify GFP_COMP.

But that leaves us in the awkward situation where
HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
'ptr + n - 1' lies within the same allocation as ptr.  Without using
a compound page, there's no indication in the VM structures that these
two pages were allocated as part of the same allocation.

We could force all multi-page allocations to be compound pages if
HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
something.  We could make it catch fewer problems by succeeding if the
page is not compound.  I don't know, these all seem like bad choices
to me.

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Kees Cook]

On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> I agree; if the crypto code is never going to try to go from the address of
> a byte in the allocation back to the head page, then there's no need to
> specify GFP_COMP.
>
> But that leaves us in the awkward situation where
> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> a compound page, there's no indication in the VM structures that these
> two pages were allocated as part of the same allocation.
>
> We could force all multi-page allocations to be compound pages if
> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> something.  We could make it catch fewer problems by succeeding if the
> page is not compound.  I don't know, these all seem like bad choices
> to me.

If GFP_COMP is _not_ the correct signal about adjacent pages being
part of the same allocation, then I agree: we need to drop this check
entirely from PAGESPAN. Is there anything else that indicates this
property? (Or where might we be able to store that info?)

There are other pagespan checks, though, so those could stay. But I'd
really love to gain page allocator allocation size checking ...

-- 
Kees Cook

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Matthew Wilcox]

On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > I agree; if the crypto code is never going to try to go from the address of
> > a byte in the allocation back to the head page, then there's no need to
> > specify GFP_COMP.
> >
> > But that leaves us in the awkward situation where
> > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> > a compound page, there's no indication in the VM structures that these
> > two pages were allocated as part of the same allocation.
> >
> > We could force all multi-page allocations to be compound pages if
> > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > something.  We could make it catch fewer problems by succeeding if the
> > page is not compound.  I don't know, these all seem like bad choices
> > to me.
> 
> If GFP_COMP is _not_ the correct signal about adjacent pages being
> part of the same allocation, then I agree: we need to drop this check
> entirely from PAGESPAN. Is there anything else that indicates this
> property? (Or where might we be able to store that info?)

As far as I know, the page allocator does not store size information
anywhere, unless you use GFP_COMP.  That's why you have to pass
the 'order' to free_pages() and __free_pages().  It's also why
alloc_pages_exact() works (follow all the way into split_page()).

> There are other pagespan checks, though, so those could stay. But I'd
> really love to gain page allocator allocation size checking ...

I think that's a great idea, but I'm not sure how you'll be able to
do that.

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Russell King - ARM Linux admin]

On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> > On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > I agree; if the crypto code is never going to try to go from the address of
> > > a byte in the allocation back to the head page, then there's no need to
> > > specify GFP_COMP.
> > >
> > > But that leaves us in the awkward situation where
> > > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > > 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> > > a compound page, there's no indication in the VM structures that these
> > > two pages were allocated as part of the same allocation.
> > >
> > > We could force all multi-page allocations to be compound pages if
> > > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > > something.  We could make it catch fewer problems by succeeding if the
> > > page is not compound.  I don't know, these all seem like bad choices
> > > to me.
> > 
> > If GFP_COMP is _not_ the correct signal about adjacent pages being
> > part of the same allocation, then I agree: we need to drop this check
> > entirely from PAGESPAN. Is there anything else that indicates this
> > property? (Or where might we be able to store that info?)
> 
> As far as I know, the page allocator does not store size information
> anywhere, unless you use GFP_COMP.  That's why you have to pass
> the 'order' to free_pages() and __free_pages().  It's also why
> alloc_pages_exact() works (follow all the way into split_page()).
> 
> > There are other pagespan checks, though, so those could stay. But I'd
> > really love to gain page allocator allocation size checking ...
> 
> I think that's a great idea, but I'm not sure how you'll be able to
> do that.

However, we have had code (maybe historically now) that has allocated
a higher order page and then handed back pages that it doesn't need -
for example, when the code requires multiple contiguous pages but does
not require a power-of-2 size of contiguous pages.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP

[Robin Murphy]

On 17/04/2019 09:09, Russell King - ARM Linux admin wrote:
> On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
>>> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
>>>> I agree; if the crypto code is never going to try to go from the address of
>>>> a byte in the allocation back to the head page, then there's no need to
>>>> specify GFP_COMP.
>>>>
>>>> But that leaves us in the awkward situation where
>>>> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
>>>> 'ptr + n - 1' lies within the same allocation as ptr.  Without using
>>>> a compound page, there's no indication in the VM structures that these
>>>> two pages were allocated as part of the same allocation.
>>>>
>>>> We could force all multi-page allocations to be compound pages if
>>>> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
>>>> something.  We could make it catch fewer problems by succeeding if the
>>>> page is not compound.  I don't know, these all seem like bad choices
>>>> to me.
>>>
>>> If GFP_COMP is _not_ the correct signal about adjacent pages being
>>> part of the same allocation, then I agree: we need to drop this check
>>> entirely from PAGESPAN. Is there anything else that indicates this
>>> property? (Or where might we be able to store that info?)
>>
>> As far as I know, the page allocator does not store size information
>> anywhere, unless you use GFP_COMP.  That's why you have to pass
>> the 'order' to free_pages() and __free_pages().  It's also why
>> alloc_pages_exact() works (follow all the way into split_page()).
>>
>>> There are other pagespan checks, though, so those could stay. But I'd
>>> really love to gain page allocator allocation size checking ...
>>
>> I think that's a great idea, but I'm not sure how you'll be able to
>> do that.
> 
> However, we have had code (maybe historically now) that has allocated
> a higher order page and then handed back pages that it doesn't need -
> for example, when the code requires multiple contiguous pages but does
> not require a power-of-2 size of contiguous pages.

'git grep alloc_pages_exact' suggests it's not historical yet...

Robin.