x86/mm/pat: Do a small optimization in reserve_memtype

From: Pan Xinhui <[email protected]>

It's safe and more reasonable to unlock memtype_lock right after rbt_memtype_check_insert.

---
 arch/x86/mm/pat.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

index 188e3e0..cb75639 100644
@@ -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

hi, Borislav
thanks for your reply :)

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.

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

Ah, we touch entry, right.

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.

Yes.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--