Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASKand HPAGE_SIZE

[David Daney]

On 11/17/2011 03:28 PM, Andrew Morton wrote:
> On Thu, 17 Nov 2011 13:57:30 -0800
> David Daney<ddaney.cavm@xxxxxxxxx>  wrote:
>
> > From: David Daney<david.daney@xxxxxxxxxx>
> >
> > It was pointed out by David Rientjes that the dummy values for
> > HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
> > used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
> > resulting code would surly not do anything sensible.
> >
> > Place BUG() in the these dummy definitions, as we do in similar
> > circumstances in other places, so any abuse can be easily detected.
> >
> > Since the only sane place to use these symbols when
> > !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
> > code to be emitted by the compiler.
>
> I assume you meant "omitted" here.

I jumbled it up.  It should read:

... the BUG() will not cause any actual code to be emitted by the 
compiler.  In fact I have verified this on both MIPS64 and x86_64 kernels.

I could re-spin the patch with a corrected changelog if desired.

> But I don't think it's true.  Any such code would occur after testing
> is_vm_hugetlb_page() or similar, and would have been omitted anyway.

The point being that we are doing:

if (is_vm_hugetlb_page(vma)) {
	/* Do something with HPAGE_MASK*/
} else {
	/* Do something with PAGE_MASK */
}

In the !CONFIG_HUGETLB_PAGE case we have:
static inline int is_vm_hugetlb_page(struct vm_area_struct *vma)
{
	return 0;
}

The compiler sees that the usage of the dummy definitions is in a dead 
code path and nothing is emitted.

> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -111,8 +111,9 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
> >   #define hugetlb_change_protection(vma, address, end, newprot)
> >
> >   #ifndef HPAGE_MASK
> > -#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
> > -#define HPAGE_SIZE	PAGE_SIZE
> > +/* Keep the compiler happy with some dummy (but BUGgy) values */
>
> That's a quite poor comment.  This?

I was trying to communicate the presence of the BUG() in the definition. 
 Perhaps it is more confusing than it was before.

> --- a/include/linux/hugetlb.h~hugetlb-provide-safer-dummy-values-for-hpage_mask-and-hpage_size-fix
> +++ a/include/linux/hugetlb.h
> @@ -111,7 +111,11 @@ static inline void copy_huge_page(struct
>   #define hugetlb_change_protection(vma, address, end, newprot)
>
>   #ifndef HPAGE_MASK
> -/* Keep the compiler happy with some dummy (but BUGgy) values */
> +/*
> + * HPAGE_MASK and friends are defined if !CONFIG_HUGETLB_PAGE as an
> + * ifdef-avoiding convenience.  However they should never be evaluated at
> + * runtime if !CONFIG_HUGETLB_PAGE.
> + */
>   #define HPAGE_MASK	({BUG(); 0; })
>   #define HPAGE_SIZE	({BUG(); 0; })
>   #define HPAGE_SHIFT	({BUG(); 0; })
> _
>
> > +#define HPAGE_MASK	({BUG(); 0; })
> > +#define HPAGE_SIZE	({BUG(); 0; })
> >   #define HPAGE_SHIFT	({BUG(); 0; })
>
> This change means that HPAGE_* cannot be evaluated at compile time.  So
>
> int foo = HPAGE_SIZE;
>
> outside functions will explode.  I guess that's OK - actually desirable
> - as such code shouldn't have been compiled anyway.

The exact point of the patch.

Thanks,
David Daney


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/