* [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype @ 2015-07-21 6:29 Pan Xinhui 2015-07-21 6:55 ` Borislav Petkov 0 siblings, 1 reply; 4+ messages in thread From: Pan Xinhui @ 2015-07-21 6:29 UTC (permalink / raw) To: linux-kernel@vger.kernel.org Cc: tglx, mingo, hpa, x86, bp, toshi.kani, jgross, mcgrof, mnipxh@163.com From: Pan Xinhui <xinhuix.pan@intel.com> It's safe and more reasonable to unlock memtype_lock right after rbt_memtype_check_insert. Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com> --- arch/x86/mm/pat.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c index 188e3e0..cb75639 100644 --- a/arch/x86/mm/pat.c +++ b/arch/x86/mm/pat.c @@ -538,20 +538,17 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type, new->type = actual_type; spin_lock(&memtype_lock); - err = rbt_memtype_check_insert(new, new_type); + spin_unlock(&memtype_lock); + if (err) { pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n", start, end - 1, cattr_name(new->type), cattr_name(req_type)); kfree(new); - spin_unlock(&memtype_lock); - return err; } - spin_unlock(&memtype_lock); - dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n", start, end - 1, cattr_name(new->type), cattr_name(req_type), new_type ? cattr_name(*new_type) : "-"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype 2015-07-21 6:29 [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype Pan Xinhui @ 2015-07-21 6:55 ` Borislav Petkov 2015-07-21 7:32 ` Pan Xinhui 0 siblings, 1 reply; 4+ messages in thread From: Borislav Petkov @ 2015-07-21 6:55 UTC (permalink / raw) To: Pan Xinhui Cc: linux-kernel@vger.kernel.org, tglx, mingo, hpa, x86, toshi.kani, jgross, mcgrof, mnipxh@163.com On Tue, Jul 21, 2015 at 02:29:35PM +0800, Pan Xinhui wrote: > From: Pan Xinhui <xinhuix.pan@intel.com> > > It's safe and more reasonable to unlock memtype_lock right after > rbt_memtype_check_insert. > > Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com> > --- > arch/x86/mm/pat.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c > index 188e3e0..cb75639 100644 > --- a/arch/x86/mm/pat.c > +++ b/arch/x86/mm/pat.c > @@ -538,20 +538,17 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type, > new->type = actual_type; > > spin_lock(&memtype_lock); > - > err = rbt_memtype_check_insert(new, new_type); > + spin_unlock(&memtype_lock); > + > if (err) { > pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n", > start, end - 1, > cattr_name(new->type), cattr_name(req_type)); > kfree(new); > - spin_unlock(&memtype_lock); > - > return err; > } > > - spin_unlock(&memtype_lock); > - > dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n", > start, end - 1, cattr_name(new->type), cattr_name(req_type), > new_type ? cattr_name(*new_type) : "-"); While you're at it, please fix a similar issue in lookup_memtype() and also improve the comments over memtype_lock to explain what exactly it protects. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype 2015-07-21 6:55 ` Borislav Petkov @ 2015-07-21 7:32 ` Pan Xinhui 2015-07-21 15:31 ` Borislav Petkov 0 siblings, 1 reply; 4+ messages in thread From: Pan Xinhui @ 2015-07-21 7:32 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel@vger.kernel.org, tglx, mingo, hpa, x86, toshi.kani, jgross, mcgrof, mnipxh@163.com hi, Borislav thanks for your reply :) On 2015年07月21日 14:55, Borislav Petkov wrote: > On Tue, Jul 21, 2015 at 02:29:35PM +0800, Pan Xinhui wrote: >> From: Pan Xinhui <xinhuix.pan@intel.com> >> >> It's safe and more reasonable to unlock memtype_lock right after >> rbt_memtype_check_insert. >> >> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com> >> --- >> arch/x86/mm/pat.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c >> index 188e3e0..cb75639 100644 >> --- a/arch/x86/mm/pat.c >> +++ b/arch/x86/mm/pat.c >> @@ -538,20 +538,17 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type, >> new->type = actual_type; >> >> spin_lock(&memtype_lock); >> - >> err = rbt_memtype_check_insert(new, new_type); >> + spin_unlock(&memtype_lock); >> + >> if (err) { >> pr_info("x86/PAT: reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n", >> start, end - 1, >> cattr_name(new->type), cattr_name(req_type)); >> kfree(new); >> - spin_unlock(&memtype_lock); >> - >> return err; >> } >> >> - spin_unlock(&memtype_lock); >> - >> dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n", >> start, end - 1, cattr_name(new->type), cattr_name(req_type), >> new_type ? cattr_name(*new_type) : "-"); > > While you're at it, please fix a similar issue in lookup_memtype() and also Let me explain why we can't unlock memtype_lock right after rbt_memtype_lookup in lookup_memtype(). CPUA CPUB spin_lock(&memtype_lock); entry = rbt_memtype_lookup(paddr); spin_unlock(&memtype_lock); ---------------------------------------------------------------------------------------- spin_lock(&memtype_lock); entry = rbt_memtype_erase(start, end); spin_unlock(&memtype_lock); if (!entry) { printk(KERN_INFO "%s:%d freeing invalid memtype [mem %#010Lx-%#010Lx]\n", current->comm, current->pid, start, end - 1); return -EINVAL; } kfree(entry); ---------------------------------------------------------------------------------------- if (entry != NULL) rettype = entry->type; else rettype = _PAGE_CACHE_UC_MINUS; yes, we may access an freed memory at that time. Because entry is stored in rb-tree. Need lock when we access it. > improve the comments over memtype_lock to explain what exactly it protects. > lock is needed when we access the data stored in rb-tree. :) I find another bug, although it's very hard to hit. just in reserve_memtype() ---------------------------------- err = rbt_memtype_check_insert(new, new_type); if (err) { printk(KERN_INFO "reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n", start, end - 1, cattr_name(new->type), cattr_name(req_type)); kfree(new); spin_unlock(&memtype_lock); return err; } spin_unlock(&memtype_lock); //this unlock may cause problems because the next dprintk access *new* dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n", start, end - 1, cattr_name(new->type), cattr_name(req_type), new_type ? cattr_name(*new_type) : "-"); ---------------------------------- if no err returned, we unlock memtype_lock, *new *is stored is rb-tree. But *new* could be freed at any possible time. race is similar with scenario above. In the second dprintk, we access *new*, *cattr_name(new->type)*. I will send patch V2 to fix this issue. I should take a more deep look at this dprintk when I send this patch. thanks xinhui > Thanks. > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype 2015-07-21 7:32 ` Pan Xinhui @ 2015-07-21 15:31 ` Borislav Petkov 0 siblings, 0 replies; 4+ messages in thread From: Borislav Petkov @ 2015-07-21 15:31 UTC (permalink / raw) To: Pan Xinhui Cc: linux-kernel@vger.kernel.org, tglx, mingo, hpa, x86, toshi.kani, jgross, mcgrof, mnipxh@163.com On Tue, Jul 21, 2015 at 03:32:50PM +0800, Pan Xinhui wrote: > yes, we may access an freed memory at that time. Because entry is > stored in rb-tree. Need lock when we access it. Ah, we touch entry, right. > > improve the comments over memtype_lock to explain what exactly it protects. > > > lock is needed when we access the data stored in rb-tree. :) I didn't ask you what it protects - I can do my own grepping and read pat_rbtree.c just fine - I asked you to update the comment. > I find another bug, although it's very hard to hit. > just in reserve_memtype() > ---------------------------------- > err = rbt_memtype_check_insert(new, new_type); > if (err) { > printk(KERN_INFO "reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n", > start, end - 1, > cattr_name(new->type), cattr_name(req_type)); > kfree(new); > spin_unlock(&memtype_lock); > > return err; > } > > spin_unlock(&memtype_lock); //this unlock may cause problems because the next dprintk access *new* Yes. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-21 15:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-21 6:29 [PATCH] x86/mm/pat: Do a small optimization in reserve_memtype Pan Xinhui 2015-07-21 6:55 ` Borislav Petkov 2015-07-21 7:32 ` Pan Xinhui 2015-07-21 15:31 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox