On Tue, Aug 08, 2017 at 03:59:36PM +0300, Igor Stoppa wrote:
> On 07/08/17 22:12, Jerome Glisse wrote:
> > On Mon, Aug 07, 2017 at 05:13:00PM +0300, Igor Stoppa wrote:
>
> [...]
>
> >> I have an updated version of the old proposal:
> >>
> >> * put a magic number in the private field, during initialization of
> >> pmalloc pages
> >>
> >> * during hardened usercopy verification, when I have to assess if a page
> >> is of pmalloc type, compare the private field against the magic number
> >>
> >> * if and only if the private field matches the magic number, then invoke
> >> find_vm_area(), so that the slowness affects only a possibly limited
> >> amount of false positives.
> >
> > This all sounds good to me.
>
> ok, I still have one doubt wrt defining the flag.
> Where should I do it?
>
> vmalloc.h has the following:
>
> /* bits in flags of vmalloc's vm_struct below */
> #define VM_IOREMAP 0x00000001 /* ioremap() and friends
> */
> #define VM_ALLOC 0x00000002 /* vmalloc() */
> #define VM_MAP 0x00000004 /* vmap()ed pages */
> #define VM_USERMAP 0x00000008 /* suitable for
> remap_vmalloc_range
> */
> #define VM_UNINITIALIZED 0x00000020 /* vm_struct is not
> fully initialized */
> #define VM_NO_GUARD 0x00000040 /* don't add guard page
> */
> #define VM_KASAN 0x00000080 /* has allocated kasan
> shadow memory */
> /* bits [20..32] reserved for arch specific ioremap internals */
>
>
>
> I am tempted to add
>
> #define VM_PMALLOC 0x00000100
>
> But would it be acceptable, to mention pmalloc into vmalloc?
>
> Should I name it VM_PRIVATE bit, instead?
>
> Using VM_PRIVATE would avoid contaminating vmalloc with something that
> depends on it (like VM_PMALLOC would do).
>
> But using VM_PRIVATE will likely add tracking issues, if someone else
> wants to use the same bit and it's not clear who is the user, if any.
VM_PMALLOC sounds fine to me also adding a comment there pointing to
pmalloc documentation would be a good thing to do. The above are flags
that are use only inside vmalloc context and so there is no issue
here of conflicting with other potential user.
>
> Unless it's acceptable to check the private field in the page struct.
> It would bear the pmalloc magic number.
I thought you wanted to do:
check struct page mapping field
check vmap->flags for VM_PMALLOC
bool is_pmalloc(unsigned long addr)
{
struct page *page;
struct vm_struct *vm_struct;
if (!is_vmalloc_addr(addr))
return false;
page = vmalloc_to_page(addr);
if (!page)
return false;
if (page->mapping != pmalloc_magic_key)
return false;
vm_struct = find_vm_area(addr);
if (!vm_struct)
return false;
return vm_struct->flags & VM_PMALLOC;
}
Did you change your plan ?
>
> I'm thinking to use a pointer to one of pmalloc data items, as signature.
What ever is easier for you. Note that dereferencing such pointer before
asserting this is really a pmalloc page would be hazardous.
Jérôme
