* [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator @ 2017-08-02 15:14 Igor Stoppa 2017-08-02 17:08 ` Jerome Glisse 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-02 15:14 UTC (permalink / raw) To: Linux-MM, LKML, linux-security-module, kernel-hardening, Jerome Glisse, Michal Hocko, Kees Cook Hi, while I am working to another example of using pmalloc [1], it was pointed out to me that: 1) I had introduced a bug when I switched to using a field of the page structure [2] 2) I was also committing a layer violation in the way I was tagging the pages. I am seeking help to understand what would be the correct way to do the tagging. Here are snippets describing the problems: 1) from pmalloc.c: ... +static const unsigned long pmalloc_signature = (unsigned long)&pmalloc_mutex; ... +int __pmalloc_tag_pages(void *base, const size_t size, const bool set_tag) +{ + void *end = base + size - 1; + + do { + struct page *page; + + if (!is_vmalloc_addr(base)) + return -EINVAL; + page = vmalloc_to_page(base); + if (set_tag) { + BUG_ON(page_private(page) || page->private); + set_page_private(page, 1); + page->private = pmalloc_signature; + } else { + BUG_ON(!(page_private(page) && + page->private == pmalloc_signature)); + set_page_private(page, 0); + page->private = 0; + } + base += PAGE_SIZE; + } while ((PAGE_MASK & (unsigned long)base) <= + (PAGE_MASK & (unsigned long)end)); + return 0; +} ... +static const char msg[] = "Not a valid Pmalloc object."; +const char *pmalloc_check_range(const void *ptr, unsigned long n) +{ + unsigned long p; + + p = (unsigned long)ptr; + n = p + n - 1; + for (; (PAGE_MASK & p) <= (PAGE_MASK & n); p += PAGE_SIZE) { + struct page *page; + + if (!is_vmalloc_addr((void *)p)) + return msg; + page = vmalloc_to_page((void *)p); + if (!(page && page_private(page) && + page->private == pmalloc_signature)) + return msg; + } + return NULL; +} The problem here comes from the way I am using page->private: the fact that the page is marked as private means only that someone is using it, and the way it is used could create (spoiler: it happens) a collision with pmalloc_signature, which can generate false positives. A way to ensure that the address really belongs to pmalloc would be to pre-screen it, against either the signature or some magic number and, if such test is passed, then compare the address against those really available in the pmalloc pools. This would be slower, but it would be limited only to those cases where the signature/magic number matches and the answer is likely to be true. 2) However, both the current (incorrect) implementation and the one I am considering, are abusing something that should be used otherwise (see the following snippet): from include/linux/mm_types.h: struct page { ... union { unsigned long private; /* Mapping-private opaque data: * usually used for buffer_heads * if PagePrivate set; used for * swp_entry_t if PageSwapCache; * indicates order in the buddy * system if PG_buddy is set. */ #if USE_SPLIT_PTE_PTLOCKS #if ALLOC_SPLIT_PTLOCKS spinlock_t *ptl; #else spinlock_t ptl; #endif #endif struct kmem_cache *slab_cache; /* SL[AU]B: Pointer to slab */ }; ... } The "private" field is meant for mapping-private opaque data, which is not how I am using it. Yet it seems the least harmful field to choose. Is this acceptable? Otherwise, what would be the best course of action? thanks, igor [1] https://lkml.org/lkml/2017/7/10/400 [2] https://lkml.org/lkml/2017/7/6/573 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-02 15:14 [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator Igor Stoppa @ 2017-08-02 17:08 ` Jerome Glisse 2017-08-03 10:11 ` Igor Stoppa 0 siblings, 1 reply; 21+ messages in thread From: Jerome Glisse @ 2017-08-02 17:08 UTC (permalink / raw) To: Igor Stoppa Cc: Linux-MM, LKML, linux-security-module, kernel-hardening, Michal Hocko, Kees Cook On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: > Hi, > while I am working to another example of using pmalloc [1], > it was pointed out to me that: > > 1) I had introduced a bug when I switched to using a field of the page > structure [2] > > 2) I was also committing a layer violation in the way I was tagging the > pages. > > I am seeking help to understand what would be the correct way to do the > tagging. > > Here are snippets describing the problems: > > > 1) from pmalloc.c: > > ... > > +static const unsigned long pmalloc_signature = (unsigned > long)&pmalloc_mutex; > > ... > > +int __pmalloc_tag_pages(void *base, const size_t size, const bool set_tag) > +{ > + void *end = base + size - 1; > + > + do { > + struct page *page; > + > + if (!is_vmalloc_addr(base)) > + return -EINVAL; > + page = vmalloc_to_page(base); > + if (set_tag) { > + BUG_ON(page_private(page) || page->private); > + set_page_private(page, 1); Above line is pointless you overwrite value right below > + page->private = pmalloc_signature; > + } else { > + BUG_ON(!(page_private(page) && > + page->private == pmalloc_signature)); > + set_page_private(page, 0); Same as above > + page->private = 0; > + } > + base += PAGE_SIZE; > + } while ((PAGE_MASK & (unsigned long)base) <= > + (PAGE_MASK & (unsigned long)end)); > + return 0; > +} > > ... > > +static const char msg[] = "Not a valid Pmalloc object."; > +const char *pmalloc_check_range(const void *ptr, unsigned long n) > +{ > + unsigned long p; > + > + p = (unsigned long)ptr; > + n = p + n - 1; > + for (; (PAGE_MASK & p) <= (PAGE_MASK & n); p += PAGE_SIZE) { > + struct page *page; > + > + if (!is_vmalloc_addr((void *)p)) > + return msg; > + page = vmalloc_to_page((void *)p); > + if (!(page && page_private(page) && > + page->private == pmalloc_signature)) > + return msg; > + } > + return NULL; > +} > > > The problem here comes from the way I am using page->private: > the fact that the page is marked as private means only that someone is > using it, and the way it is used could create (spoiler: it happens) a > collision with pmalloc_signature, which can generate false positives. Is page->private use for vmalloc memory ? If so then pick another field. Thought i doubt it is use i would need to check. What was the exact objection made ? > > A way to ensure that the address really belongs to pmalloc would be to > pre-screen it, against either the signature or some magic number and, > if such test is passed, then compare the address against those really > available in the pmalloc pools. > > This would be slower, but it would be limited only to those cases where > the signature/magic number matches and the answer is likely to be true. > > 2) However, both the current (incorrect) implementation and the one I am > considering, are abusing something that should be used otherwise (see > the following snippet): > > from include/linux/mm_types.h: > > struct page { > ... > union { > unsigned long private; /* Mapping-private opaque data: > * usually used for buffer_heads > * if PagePrivate set; used for > * swp_entry_t if PageSwapCache; > * indicates order in the buddy > * system if PG_buddy is set. > */ > #if USE_SPLIT_PTE_PTLOCKS > #if ALLOC_SPLIT_PTLOCKS > spinlock_t *ptl; > #else > spinlock_t ptl; > #endif > #endif > struct kmem_cache *slab_cache; /* SL[AU]B: Pointer to slab */ > }; > ... > } > > > The "private" field is meant for mapping-private opaque data, which is > not how I am using it. As you can see this is an union and thus the meaning of that field depends on how the page is use. The private comment you see is only meaningfull for page that are in the page cache and are coming from a file system ie when a process does an mmap of a file. When page is use by sl[au]b the slab_cache field is how it is interpreted ... Context in which a page is use do matter. Here we are talking about memory that is allocated to back vmalloc area so the private field is unuse AFAICR and it is safe to use it while the page is use for vmalloc. Note that i don't think anyone is doing vmap() of pages that are in the page cache that would seem wrong from my point of view but maybe some one is. Thought someone might be doing vmap() of pages in which the private field is use for something (like a device driver private field) in which case you might still have false positive. You might want to simply add something either to vm_struct or vmap_area to know if a range of vmalloc area has been created by pmalloc or not. Maybe you don't even need to tag page and storing flag in vmap_area or vm_struct would be enough. Jérôme ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-02 17:08 ` Jerome Glisse @ 2017-08-03 10:11 ` Igor Stoppa 2017-08-03 11:48 ` Michal Hocko 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-03 10:11 UTC (permalink / raw) To: Jerome Glisse Cc: Linux-MM, LKML, linux-security-module, kernel-hardening, Michal Hocko, Kees Cook On 02/08/17 20:08, Jerome Glisse wrote: > On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: [...] >> + set_page_private(page, 1); > > Above line is pointless you overwrite value right below yes ... > >> + page->private = pmalloc_signature; >> + } else { >> + BUG_ON(!(page_private(page) && >> + page->private == pmalloc_signature)); >> + set_page_private(page, 0); > > Same as above ... and yes >> + page->private = 0; >> + } >> + base += PAGE_SIZE; >> + } while ((PAGE_MASK & (unsigned long)base) <= >> + (PAGE_MASK & (unsigned long)end)); >> + return 0; >> +} >> >> ... >> >> +static const char msg[] = "Not a valid Pmalloc object."; >> +const char *pmalloc_check_range(const void *ptr, unsigned long n) >> +{ >> + unsigned long p; >> + >> + p = (unsigned long)ptr; >> + n = p + n - 1; >> + for (; (PAGE_MASK & p) <= (PAGE_MASK & n); p += PAGE_SIZE) { >> + struct page *page; >> + >> + if (!is_vmalloc_addr((void *)p)) >> + return msg; >> + page = vmalloc_to_page((void *)p); >> + if (!(page && page_private(page) && >> + page->private == pmalloc_signature)) >> + return msg; >> + } >> + return NULL; >> +} >> >> >> The problem here comes from the way I am using page->private: >> the fact that the page is marked as private means only that someone is >> using it, and the way it is used could create (spoiler: it happens) a >> collision with pmalloc_signature, which can generate false positives. > > Is page->private use for vmalloc memory ? If so then pick another field. No, it is not in use by vmalloc, as far as I can tell, by both reading the code and empirically printing out its value in few cases. > Thought i doubt it is use i would need to check. What was the exact > objection made ? The objection made is what I tried to explain below, that the comment besides the declaration of the private field says: "Mapping-private opaque data: ..." I'll reply to your answer there. >> A way to ensure that the address really belongs to pmalloc would be to >> pre-screen it, against either the signature or some magic number and, >> if such test is passed, then compare the address against those really >> available in the pmalloc pools. >> >> This would be slower, but it would be limited only to those cases where >> the signature/magic number matches and the answer is likely to be true. >> >> 2) However, both the current (incorrect) implementation and the one I am >> considering, are abusing something that should be used otherwise (see >> the following snippet): >> >> from include/linux/mm_types.h: >> >> struct page { >> ... >> union { >> unsigned long private; /* Mapping-private opaque data: >> * usually used for buffer_heads >> * if PagePrivate set; used for >> * swp_entry_t if PageSwapCache; >> * indicates order in the buddy >> * system if PG_buddy is set. >> */ >> #if USE_SPLIT_PTE_PTLOCKS >> #if ALLOC_SPLIT_PTLOCKS >> spinlock_t *ptl; >> #else >> spinlock_t ptl; >> #endif >> #endif >> struct kmem_cache *slab_cache; /* SL[AU]B: Pointer to slab */ >> }; >> ... >> } >> >> >> The "private" field is meant for mapping-private opaque data, which is >> not how I am using it. > > As you can see this is an union and thus the meaning of that field depends > on how the page is use. The private comment you see is only meaningfull for > page that are in the page cache and are coming from a file system ie when > a process does an mmap of a file. When page is use by sl[au]b the slab_cache > field is how it is interpreted ... Context in which a page is use do matter. I am not native English speaker, but the comment seems to imply that, no matter what, it's Mapping-private. If the "Mapping-private" was dropped or somehow connected exclusively to the cases listed in the comment, then I think it would be more clear that the comment needs to be intended as related to mapping in certain cases only. But it is otherwise ok to use the "private" field for whatever purpose it might be suitable, as long as it is not already in use. > Here we are talking about memory that is allocated to back vmalloc area so > the private field is unuse AFAICR and it is safe to use it while the page > is use for vmalloc. Yes, my experience seems to confirm that. > Note that i don't think anyone is doing vmap() of pages that are in the page > cache that would seem wrong from my point of view but maybe some one is. > Thought someone might be doing vmap() of pages in which the private field is > use for something (like a device driver private field) in which case you might > still have false positive. You might want to simply add something either to > vm_struct or vmap_area to know if a range of vmalloc area has been created > by pmalloc or not. Maybe you don't even need to tag page and storing flag > in vmap_area or vm_struct would be enough. This last suggestion gives me a feeling of unease: it seems that each user of the private field has its own way to indicate that the field is in use (see the comment beside the declaration of the field). Wouldn't it make more sense to have one (sub)field, somewhere in the page structure, that would contain an unique signature (an enum?) stating who is the user? This might make the field slightly less opaque, but easier to infer its content, when it is not possible to rely on the context (like for hardened usercopy case). But, to reply more specifically to your advice, yes, I think I could add a flag to vm_struct and then retrieve its value, for the address being processed, by passing through find_vm_area(). I will try going down this path, thank you. -- thanks, igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 10:11 ` Igor Stoppa @ 2017-08-03 11:48 ` Michal Hocko 2017-08-03 12:20 ` Igor Stoppa 0 siblings, 1 reply; 21+ messages in thread From: Michal Hocko @ 2017-08-03 11:48 UTC (permalink / raw) To: Igor Stoppa Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Thu 03-08-17 13:11:45, Igor Stoppa wrote: > On 02/08/17 20:08, Jerome Glisse wrote: > > On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: [...] > >> A way to ensure that the address really belongs to pmalloc would be to > >> pre-screen it, against either the signature or some magic number and, > >> if such test is passed, then compare the address against those really > >> available in the pmalloc pools. > >> > >> This would be slower, but it would be limited only to those cases where > >> the signature/magic number matches and the answer is likely to be true. > >> > >> 2) However, both the current (incorrect) implementation and the one I am > >> considering, are abusing something that should be used otherwise (see > >> the following snippet): > >> > >> from include/linux/mm_types.h: > >> > >> struct page { > >> ... > >> union { > >> unsigned long private; /* Mapping-private opaque data: > >> * usually used for buffer_heads > >> * if PagePrivate set; used for > >> * swp_entry_t if PageSwapCache; > >> * indicates order in the buddy > >> * system if PG_buddy is set. > >> */ > >> #if USE_SPLIT_PTE_PTLOCKS > >> #if ALLOC_SPLIT_PTLOCKS > >> spinlock_t *ptl; > >> #else > >> spinlock_t ptl; > >> #endif > >> #endif > >> struct kmem_cache *slab_cache; /* SL[AU]B: Pointer to slab */ > >> }; > >> ... > >> } > >> > >> > >> The "private" field is meant for mapping-private opaque data, which is > >> not how I am using it. > > > > As you can see this is an union and thus the meaning of that field depends > > on how the page is use. The private comment you see is only meaningfull for > > page that are in the page cache and are coming from a file system ie when > > a process does an mmap of a file. When page is use by sl[au]b the slab_cache > > field is how it is interpreted ... Context in which a page is use do matter. > > I am not native English speaker, but the comment seems to imply that, no > matter what, it's Mapping-private. > > If the "Mapping-private" was dropped or somehow connected exclusively to > the cases listed in the comment, then I think it would be more clear > that the comment needs to be intended as related to mapping in certain > cases only. > But it is otherwise ok to use the "private" field for whatever purpose > it might be suitable, as long as it is not already in use. I would recommend adding a new field into the enum... [...] > But, to reply more specifically to your advice, yes, I think I could add > a flag to vm_struct and then retrieve its value, for the address being > processed, by passing through find_vm_area(). ... and you can store vm_struct pointer to the struct page there and you won't need to do the slow find_vm_area. I haven't checked very closely but this should be possible in principle. I guess other callers might benefit from this as well. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 11:48 ` Michal Hocko @ 2017-08-03 12:20 ` Igor Stoppa 2017-08-03 13:55 ` Michal Hocko 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-03 12:20 UTC (permalink / raw) To: Michal Hocko Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 03/08/17 14:48, Michal Hocko wrote: > On Thu 03-08-17 13:11:45, Igor Stoppa wrote: >> On 02/08/17 20:08, Jerome Glisse wrote: >>> On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: [...] >>>> from include/linux/mm_types.h: >>>> >>>> struct page { >>>> ... >>>> union { >>>> unsigned long private; /* Mapping-private opaque data: >>>> * usually used for buffer_heads >>>> * if PagePrivate set; used for >>>> * swp_entry_t if PageSwapCache; >>>> * indicates order in the buddy >>>> * system if PG_buddy is set. >>>> */ [...] >> If the "Mapping-private" was dropped or somehow connected exclusively to >> the cases listed in the comment, then I think it would be more clear >> that the comment needs to be intended as related to mapping in certain >> cases only. >> But it is otherwise ok to use the "private" field for whatever purpose >> it might be suitable, as long as it is not already in use. > > I would recommend adding a new field into the enum... s/enum/union/ ? If not, I am not sure what is the enum that you are talking about. [...] >> But, to reply more specifically to your advice, yes, I think I could add >> a flag to vm_struct and then retrieve its value, for the address being >> processed, by passing through find_vm_area(). > > ... and you can store vm_struct pointer to the struct page there "there" as in the new field of the union? btw, what would be a meaningful name, since "private" is already taken? For simplicity, I'll use, for now, "private2" > and you> won't need to do the slow find_vm_area. I haven't checked very closely > but this should be possible in principle. I guess other callers might > benefit from this as well. I am confused about this: if "private2" is a pointer, but when I get an address, I do not even know if the address represents a valid pmalloc page, how can i know when it's ok to dereference "private2"? Since it's just another field in a union, it can actually contain a value that should be interpreted as some other field, right? -- thanks, igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 12:20 ` Igor Stoppa @ 2017-08-03 13:55 ` Michal Hocko 2017-08-03 14:41 ` Igor Stoppa 2017-08-03 14:47 ` Jerome Glisse 0 siblings, 2 replies; 21+ messages in thread From: Michal Hocko @ 2017-08-03 13:55 UTC (permalink / raw) To: Igor Stoppa Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Thu 03-08-17 15:20:31, Igor Stoppa wrote: > On 03/08/17 14:48, Michal Hocko wrote: > > On Thu 03-08-17 13:11:45, Igor Stoppa wrote: > >> On 02/08/17 20:08, Jerome Glisse wrote: > >>> On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: > > [...] > > >>>> from include/linux/mm_types.h: > >>>> > >>>> struct page { > >>>> ... > >>>> union { > >>>> unsigned long private; /* Mapping-private opaque data: > >>>> * usually used for buffer_heads > >>>> * if PagePrivate set; used for > >>>> * swp_entry_t if PageSwapCache; > >>>> * indicates order in the buddy > >>>> * system if PG_buddy is set. > >>>> */ > > [...] > > >> If the "Mapping-private" was dropped or somehow connected exclusively to > >> the cases listed in the comment, then I think it would be more clear > >> that the comment needs to be intended as related to mapping in certain > >> cases only. > >> But it is otherwise ok to use the "private" field for whatever purpose > >> it might be suitable, as long as it is not already in use. > > > > I would recommend adding a new field into the enum... > > s/enum/union/ ? > > If not, I am not sure what is the enum that you are talking about. yeah, fat fingers on my side > > [...] > > >> But, to reply more specifically to your advice, yes, I think I could add > >> a flag to vm_struct and then retrieve its value, for the address being > >> processed, by passing through find_vm_area(). > > > > ... and you can store vm_struct pointer to the struct page there > > "there" as in the new field of the union? > btw, what would be a meaningful name, since "private" is already taken? > > For simplicity, I'll use, for now, "private2" why not explicit vm_area? > > and you> won't need to do the slow find_vm_area. I haven't checked > very closely > > but this should be possible in principle. I guess other callers might > > benefit from this as well. > > I am confused about this: if "private2" is a pointer, but when I get an > address, I do not even know if the address represents a valid pmalloc > page, how can i know when it's ok to dereference "private2"? because you can make all pages which back vmalloc mappings have vm_area pointer set. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 13:55 ` Michal Hocko @ 2017-08-03 14:41 ` Igor Stoppa 2017-08-03 14:47 ` Jerome Glisse 1 sibling, 0 replies; 21+ messages in thread From: Igor Stoppa @ 2017-08-03 14:41 UTC (permalink / raw) To: Michal Hocko Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 03/08/17 16:55, Michal Hocko wrote: > On Thu 03-08-17 15:20:31, Igor Stoppa wrote: >> On 03/08/17 14:48, Michal Hocko wrote: >>> On Thu 03-08-17 13:11:45, Igor Stoppa wrote: [...] >>>> But, to reply more specifically to your advice, yes, I think I could add >>>> a flag to vm_struct and then retrieve its value, for the address being >>>> processed, by passing through find_vm_area(). >>> >>> ... and you can store vm_struct pointer to the struct page there >> >> "there" as in the new field of the union? >> btw, what would be a meaningful name, since "private" is already taken? >> >> For simplicity, I'll use, for now, "private2" > > why not explicit vm_area? ok :-) >>> and you won't need to do the slow find_vm_area. I haven't checked >> very closely >>> but this should be possible in principle. I guess other callers might >>> benefit from this as well. >> >> I am confused about this: if "private2" is a pointer, but when I get an >> address, I do not even know if the address represents a valid pmalloc >> page, how can i know when it's ok to dereference "private2"? > > because you can make all pages which back vmalloc mappings have vm_area > pointer set. Ah, now I see, I had missed that the field would be set for *all* pages backed by vmalloc. So, given a pointer, I still have to figure out if it refers to a vmalloc area or not. However, that is something I need to do anyway, to get the reference to the corresponding page struct, in case it is indeed a vmalloc address. -- thanks, igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 13:55 ` Michal Hocko 2017-08-03 14:41 ` Igor Stoppa @ 2017-08-03 14:47 ` Jerome Glisse 2017-08-03 15:06 ` Igor Stoppa 1 sibling, 1 reply; 21+ messages in thread From: Jerome Glisse @ 2017-08-03 14:47 UTC (permalink / raw) To: Michal Hocko Cc: Igor Stoppa, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Thu, Aug 03, 2017 at 03:55:50PM +0200, Michal Hocko wrote: > On Thu 03-08-17 15:20:31, Igor Stoppa wrote: > > On 03/08/17 14:48, Michal Hocko wrote: > > > On Thu 03-08-17 13:11:45, Igor Stoppa wrote: > > >> On 02/08/17 20:08, Jerome Glisse wrote: > > >>> On Wed, Aug 02, 2017 at 06:14:28PM +0300, Igor Stoppa wrote: > > > > [...] > > > > >>>> from include/linux/mm_types.h: > > >>>> > > >>>> struct page { > > >>>> ... > > >>>> union { > > >>>> unsigned long private; /* Mapping-private opaque data: > > >>>> * usually used for buffer_heads > > >>>> * if PagePrivate set; used for > > >>>> * swp_entry_t if PageSwapCache; > > >>>> * indicates order in the buddy > > >>>> * system if PG_buddy is set. > > >>>> */ > > > > [...] > > > > >> If the "Mapping-private" was dropped or somehow connected exclusively to > > >> the cases listed in the comment, then I think it would be more clear > > >> that the comment needs to be intended as related to mapping in certain > > >> cases only. > > >> But it is otherwise ok to use the "private" field for whatever purpose > > >> it might be suitable, as long as it is not already in use. > > > > > > I would recommend adding a new field into the enum... > > > > s/enum/union/ ? > > > > If not, I am not sure what is the enum that you are talking about. > > yeah, fat fingers on my side > > > > > [...] > > > > >> But, to reply more specifically to your advice, yes, I think I could add > > >> a flag to vm_struct and then retrieve its value, for the address being > > >> processed, by passing through find_vm_area(). > > > > > > ... and you can store vm_struct pointer to the struct page there > > > > "there" as in the new field of the union? > > btw, what would be a meaningful name, since "private" is already taken? > > > > For simplicity, I'll use, for now, "private2" > > why not explicit vm_area? > > > > and you> won't need to do the slow find_vm_area. I haven't checked > > very closely > > > but this should be possible in principle. I guess other callers might > > > benefit from this as well. > > > > I am confused about this: if "private2" is a pointer, but when I get an > > address, I do not even know if the address represents a valid pmalloc > > page, how can i know when it's ok to dereference "private2"? > > because you can make all pages which back vmalloc mappings have vm_area > pointer set. Note that i think this might break some device driver that use vmap() i think some of them use private field to store device driver specific informations. But there likely is an unuse field in struct page that can be use for that. Jérôme ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 14:47 ` Jerome Glisse @ 2017-08-03 15:06 ` Igor Stoppa 2017-08-03 15:15 ` Michal Hocko 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-03 15:06 UTC (permalink / raw) To: Jerome Glisse, Michal Hocko Cc: Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 03/08/17 17:47, Jerome Glisse wrote: > On Thu, Aug 03, 2017 at 03:55:50PM +0200, Michal Hocko wrote: >> On Thu 03-08-17 15:20:31, Igor Stoppa wrote: [...] >>> I am confused about this: if "private2" is a pointer, but when I get an >>> address, I do not even know if the address represents a valid pmalloc >>> page, how can i know when it's ok to dereference "private2"? >> >> because you can make all pages which back vmalloc mappings have vm_area >> pointer set. > > Note that i think this might break some device driver that use vmap() > i think some of them use private field to store device driver specific > informations. But there likely is an unuse field in struct page that > can be use for that. This increases the unease from my side ... it looks like there is no way to fully understand if a field is really used or not, without having deep intimate knowledge of lots of code that is only marginally involved :-/ Similarly, how would I be able to specify what would be the correct way to decide the member of the union to use for handling the field? If there were either some sort of non-multiplexed tag/cookie field or a function, that would specify how to treat the various unions, then it would be easier to multiplex the remaining data, according to how the page is used. -- igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 15:06 ` Igor Stoppa @ 2017-08-03 15:15 ` Michal Hocko 2017-08-04 8:02 ` Igor Stoppa 0 siblings, 1 reply; 21+ messages in thread From: Michal Hocko @ 2017-08-03 15:15 UTC (permalink / raw) To: Igor Stoppa Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Thu 03-08-17 18:06:11, Igor Stoppa wrote: > > > On 03/08/17 17:47, Jerome Glisse wrote: > > On Thu, Aug 03, 2017 at 03:55:50PM +0200, Michal Hocko wrote: > >> On Thu 03-08-17 15:20:31, Igor Stoppa wrote: > > [...] > > >>> I am confused about this: if "private2" is a pointer, but when I get an > >>> address, I do not even know if the address represents a valid pmalloc > >>> page, how can i know when it's ok to dereference "private2"? > >> > >> because you can make all pages which back vmalloc mappings have vm_area > >> pointer set. > > > > Note that i think this might break some device driver that use vmap() > > i think some of them use private field to store device driver specific > > informations. But there likely is an unuse field in struct page that > > can be use for that. > > This increases the unease from my side ... it looks like there is no way > to fully understand if a field is really used or not, without having > deep intimate knowledge of lots of code that is only marginally involved :-/ welcome to the struct page heaven... > Similarly, how would I be able to specify what would be the correct way > to decide the member of the union to use for handling the field? I would check the one where we have mapping. It is rather unlikely vmalloc users would touch this one. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-03 15:15 ` Michal Hocko @ 2017-08-04 8:02 ` Igor Stoppa 2017-08-04 8:12 ` Michal Hocko 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-04 8:02 UTC (permalink / raw) To: Michal Hocko Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 03/08/17 18:15, Michal Hocko wrote: > I would check the one where we have mapping. It is rather unlikely > vmalloc users would touch this one. That was also the initial recommendation from Jerome Glisse, but it seemed unusable, because of the related comment. I should have asked for clarifications back then :-( But it's never too late ... struct page { /* First double word block */ unsigned long flags; /* Atomic flags, some possibly * updated asynchronously */ union { struct address_space *mapping; /* If low bit clear, points to * inode address_space, or NULL. * If page mapped as anonymous * memory, low bit is set, and * it points to anon_vma object: * see PAGE_MAPPING_ANON below. */ ... } mapping seems to be used exclusively in 2 ways, based on the value of its lower bit. Therefore I discarded it as valid option ("private", otoh was far more alluring), but maybe I could wrap it inside a union, together with vm_area? --- thanks, igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-04 8:02 ` Igor Stoppa @ 2017-08-04 8:12 ` Michal Hocko 2017-08-07 11:26 ` Igor Stoppa 0 siblings, 1 reply; 21+ messages in thread From: Michal Hocko @ 2017-08-04 8:12 UTC (permalink / raw) To: Igor Stoppa Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Fri 04-08-17 11:02:46, Igor Stoppa wrote: > > > On 03/08/17 18:15, Michal Hocko wrote: > > > I would check the one where we have mapping. It is rather unlikely > > vmalloc users would touch this one. > > That was also the initial recommendation from Jerome Glisse, but it > seemed unusable, because of the related comment. > > I should have asked for clarifications back then :-( > > But it's never too late ... > > > struct page { > /* First double word block */ > unsigned long flags; /* Atomic flags, some possibly > * updated asynchronously */ > union { > struct address_space *mapping; /* If low bit clear, points to > * inode address_space, or NULL. > * If page mapped as anonymous > * memory, low bit is set, and > * it points to anon_vma object: > * see PAGE_MAPPING_ANON below. > */ > ... > } > > mapping seems to be used exclusively in 2 ways, based on the value of > its lower bit. Not really. The above applies to LRU pages. Please note that Slab pages use s_mem and huge pages use compound_mapcount. If vmalloc pages are using none of those already you can add a new field there. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-04 8:12 ` Michal Hocko @ 2017-08-07 11:26 ` Igor Stoppa 2017-08-07 11:34 ` Michal Hocko 2017-08-07 13:31 ` Jerome Glisse 0 siblings, 2 replies; 21+ messages in thread From: Igor Stoppa @ 2017-08-07 11:26 UTC (permalink / raw) To: Michal Hocko Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 04/08/17 11:12, Michal Hocko wrote: > On Fri 04-08-17 11:02:46, Igor Stoppa wrote: [...] >> struct page { >> /* First double word block */ >> unsigned long flags; /* Atomic flags, some possibly >> * updated asynchronously */ >> union { >> struct address_space *mapping; /* If low bit clear, points to >> * inode address_space, or NULL. >> * If page mapped as anonymous >> * memory, low bit is set, and >> * it points to anon_vma object: >> * see PAGE_MAPPING_ANON below. >> */ >> ... >> } >> >> mapping seems to be used exclusively in 2 ways, based on the value of >> its lower bit. > > Not really. The above applies to LRU pages. Please note that Slab pages > use s_mem and huge pages use compound_mapcount. If vmalloc pages are > using none of those already you can add a new field there. Yes, both from reading the code and some experimentation, it seems that vmalloc is not using either field. I'll add a vm_area field as you advised. Is this something I could send as standalone patch? -- thank you, igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-07 11:26 ` Igor Stoppa @ 2017-08-07 11:34 ` Michal Hocko 2017-08-07 13:31 ` Jerome Glisse 1 sibling, 0 replies; 21+ messages in thread From: Michal Hocko @ 2017-08-07 11:34 UTC (permalink / raw) To: Igor Stoppa Cc: Jerome Glisse, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Mon 07-08-17 14:26:21, Igor Stoppa wrote: > On 04/08/17 11:12, Michal Hocko wrote: > > On Fri 04-08-17 11:02:46, Igor Stoppa wrote: > > [...] > > >> struct page { > >> /* First double word block */ > >> unsigned long flags; /* Atomic flags, some possibly > >> * updated asynchronously */ > >> union { > >> struct address_space *mapping; /* If low bit clear, points to > >> * inode address_space, or NULL. > >> * If page mapped as anonymous > >> * memory, low bit is set, and > >> * it points to anon_vma object: > >> * see PAGE_MAPPING_ANON below. > >> */ > >> ... > >> } > >> > >> mapping seems to be used exclusively in 2 ways, based on the value of > >> its lower bit. > > > > Not really. The above applies to LRU pages. Please note that Slab pages > > use s_mem and huge pages use compound_mapcount. If vmalloc pages are > > using none of those already you can add a new field there. > > Yes, both from reading the code and some experimentation, it seems that > vmalloc is not using either field. > > I'll add a vm_area field as you advised. > > Is this something I could send as standalone patch? Yes I would start that way and also look at some find_vm_area callers and maybe they can be simplified. The most obvious one being task_struct::stack_vm_area but I have to confess I haven't checked that too deeply. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-07 11:26 ` Igor Stoppa 2017-08-07 11:34 ` Michal Hocko @ 2017-08-07 13:31 ` Jerome Glisse 2017-08-07 14:13 ` Igor Stoppa 1 sibling, 1 reply; 21+ messages in thread From: Jerome Glisse @ 2017-08-07 13:31 UTC (permalink / raw) To: Igor Stoppa Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Mon, Aug 07, 2017 at 02:26:21PM +0300, Igor Stoppa wrote: > On 04/08/17 11:12, Michal Hocko wrote: > > On Fri 04-08-17 11:02:46, Igor Stoppa wrote: > > [...] > > >> struct page { > >> /* First double word block */ > >> unsigned long flags; /* Atomic flags, some possibly > >> * updated asynchronously */ > >> union { > >> struct address_space *mapping; /* If low bit clear, points to > >> * inode address_space, or NULL. > >> * If page mapped as anonymous > >> * memory, low bit is set, and > >> * it points to anon_vma object: > >> * see PAGE_MAPPING_ANON below. > >> */ > >> ... > >> } > >> > >> mapping seems to be used exclusively in 2 ways, based on the value of > >> its lower bit. > > > > Not really. The above applies to LRU pages. Please note that Slab pages > > use s_mem and huge pages use compound_mapcount. If vmalloc pages are > > using none of those already you can add a new field there. > > Yes, both from reading the code and some experimentation, it seems that > vmalloc is not using either field. > > I'll add a vm_area field as you advised. > > Is this something I could send as standalone patch? Note that vmalloc() is not the only thing that use vmalloc address space. There is also vmap() and i know one set of drivers that use vmap() and also use the mapping field of struct page namely GPU drivers. So like i said previously i would store a flag inside vm_struct to know if page you are looking at are pmalloc or not. Again do you need to store something per page ? Would storing it per vm_struct not be enough ? Cheers, Jérôme ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-07 13:31 ` Jerome Glisse @ 2017-08-07 14:13 ` Igor Stoppa 2017-08-07 19:12 ` Jerome Glisse 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-07 14:13 UTC (permalink / raw) To: Jerome Glisse Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 07/08/17 16:31, Jerome Glisse wrote: > On Mon, Aug 07, 2017 at 02:26:21PM +0300, Igor Stoppa wrote: [...] >> I'll add a vm_area field as you advised. >> >> Is this something I could send as standalone patch? > > Note that vmalloc() is not the only thing that use vmalloc address > space. There is also vmap() and i know one set of drivers that use > vmap() and also use the mapping field of struct page namely GPU > drivers. Ah, yes, you mentioned this. > So like i said previously i would store a flag inside vm_struct to > know if page you are looking at are pmalloc or not. And I was planning to follow your advice, using one of the flags. But ... > Again do you > need to store something per page ? Would storing it per vm_struct > not be enough ? ... there was this further comment, about speeding up the access to vm_area, which seemed good from performance perspective. ---8<--------------8<--------------8<--------------8<--------------8<--- On 03/08/17 14:48, Michal Hocko wrote: > On Thu 03-08-17 13:11:45, Igor Stoppa wrote: [...] >> But, to reply more specifically to your advice, yes, I think I could >> add a flag to vm_struct and then retrieve its value, for the address >> being processed, by passing through find_vm_area(). > > ... and you can store vm_struct pointer to the struct page there and > you won't need to do the slow find_vm_area. I haven't checked very > closely but this should be possible in principle. I guess other > callers might benefit from this as well. ---8<--------------8<--------------8<--------------8<--------------8<--- I do not strictly need to modify the page struct, but it seems it might harm performance, if it is added on the path of hardened usercopy. 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. -- igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-07 14:13 ` Igor Stoppa @ 2017-08-07 19:12 ` Jerome Glisse 2017-08-08 12:59 ` Igor Stoppa 0 siblings, 1 reply; 21+ messages in thread From: Jerome Glisse @ 2017-08-07 19:12 UTC (permalink / raw) To: Igor Stoppa Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Mon, Aug 07, 2017 at 05:13:00PM +0300, Igor Stoppa wrote: > > > On 07/08/17 16:31, Jerome Glisse wrote: > > On Mon, Aug 07, 2017 at 02:26:21PM +0300, Igor Stoppa wrote: > > [...] > > >> I'll add a vm_area field as you advised. > >> > >> Is this something I could send as standalone patch? > > > > Note that vmalloc() is not the only thing that use vmalloc address > > space. There is also vmap() and i know one set of drivers that use > > vmap() and also use the mapping field of struct page namely GPU > > drivers. > > Ah, yes, you mentioned this. > > > So like i said previously i would store a flag inside vm_struct to > > know if page you are looking at are pmalloc or not. > > And I was planning to follow your advice, using one of the flags. > But ... > > > Again do you > > need to store something per page ? Would storing it per vm_struct > > not be enough ? > > ... there was this further comment, about speeding up the access to > vm_area, which seemed good from performance perspective. > > ---8<--------------8<--------------8<--------------8<--------------8<--- > On 03/08/17 14:48, Michal Hocko wrote: > > On Thu 03-08-17 13:11:45, Igor Stoppa wrote: > > [...] > > >> But, to reply more specifically to your advice, yes, I think I could > >> add a flag to vm_struct and then retrieve its value, for the address > >> being processed, by passing through find_vm_area(). > > > > ... and you can store vm_struct pointer to the struct page there and > > you won't need to do the slow find_vm_area. I haven't checked very > > closely but this should be possible in principle. I guess other > > callers might benefit from this as well. > ---8<--------------8<--------------8<--------------8<--------------8<--- > > I do not strictly need to modify the page struct, but it seems it might > harm performance, if it is added on the path of hardened usercopy. > > 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. Jérôme ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-07 19:12 ` Jerome Glisse @ 2017-08-08 12:59 ` Igor Stoppa 2017-08-08 23:15 ` Jerome Glisse 0 siblings, 1 reply; 21+ messages in thread From: Igor Stoppa @ 2017-08-08 12:59 UTC (permalink / raw) To: Jerome Glisse Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook 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. Unless it's acceptable to check the private field in the page struct. It would bear the pmalloc magic number. I'm thinking to use a pointer to one of pmalloc data items, as signature. -- igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-08 12:59 ` Igor Stoppa @ 2017-08-08 23:15 ` Jerome Glisse 2017-08-09 7:27 ` Igor Stoppa 2017-08-10 7:14 ` Michal Hocko 0 siblings, 2 replies; 21+ messages in thread From: Jerome Glisse @ 2017-08-08 23:15 UTC (permalink / raw) To: Igor Stoppa Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-08 23:15 ` Jerome Glisse @ 2017-08-09 7:27 ` Igor Stoppa 2017-08-10 7:14 ` Michal Hocko 1 sibling, 0 replies; 21+ messages in thread From: Igor Stoppa @ 2017-08-09 7:27 UTC (permalink / raw) To: Jerome Glisse Cc: Michal Hocko, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On 09/08/17 02:15, Jerome Glisse wrote: > On Tue, Aug 08, 2017 at 03:59:36PM +0300, Igor Stoppa wrote: [...] >> I am tempted to add >> >> #define VM_PMALLOC 0x00000100 [...] > 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. ok, will do >> >> 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) page->private ? I thought mapping would not work in the cases you mentioned? > return false; > > vm_struct = find_vm_area(addr); > if (!vm_struct) > return false; > > return vm_struct->flags & VM_PMALLOC; > } > > Did you change your plan ? No, the code I have is almost 1:1 what you wrote. Apart from mapping <=> private In my previous mail I referred to page->private. Maybe I was not very clear in what I wrote, but I'm almost 100% aligned with your snippet. >> 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. Yes, it's not even needed in this scenario. It was just a way to ensure that it would be a value that cannot be come out accidentally as pointer value, since it is already taken. -- igor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator 2017-08-08 23:15 ` Jerome Glisse 2017-08-09 7:27 ` Igor Stoppa @ 2017-08-10 7:14 ` Michal Hocko 1 sibling, 0 replies; 21+ messages in thread From: Michal Hocko @ 2017-08-10 7:14 UTC (permalink / raw) To: Jerome Glisse Cc: Igor Stoppa, Linux-MM, LKML, linux-security-module, kernel-hardening, Kees Cook On Tue 08-08-17 19:15:36, Jerome Glisse wrote: > 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. Yes I agree. VM_PRIVATE just calls for the issues you are dealing with at struct page level where you simply do not know who might be (ab)using mapping and what not because the naming is just too generic... -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-08-10 7:14 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-02 15:14 [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator Igor Stoppa 2017-08-02 17:08 ` Jerome Glisse 2017-08-03 10:11 ` Igor Stoppa 2017-08-03 11:48 ` Michal Hocko 2017-08-03 12:20 ` Igor Stoppa 2017-08-03 13:55 ` Michal Hocko 2017-08-03 14:41 ` Igor Stoppa 2017-08-03 14:47 ` Jerome Glisse 2017-08-03 15:06 ` Igor Stoppa 2017-08-03 15:15 ` Michal Hocko 2017-08-04 8:02 ` Igor Stoppa 2017-08-04 8:12 ` Michal Hocko 2017-08-07 11:26 ` Igor Stoppa 2017-08-07 11:34 ` Michal Hocko 2017-08-07 13:31 ` Jerome Glisse 2017-08-07 14:13 ` Igor Stoppa 2017-08-07 19:12 ` Jerome Glisse 2017-08-08 12:59 ` Igor Stoppa 2017-08-08 23:15 ` Jerome Glisse 2017-08-09 7:27 ` Igor Stoppa 2017-08-10 7:14 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).