* [patch 1/1]pat: fix type check error on chk_conflict function
@ 2008-10-12 13:52 bibo mao
2008-10-13 16:49 ` Pallipadi, Venkatesh
0 siblings, 1 reply; 2+ messages in thread
From: bibo mao @ 2008-10-12 13:52 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel
On chk_conflict() function, if new->type does not match entry->type
and type is not NULL, this does not go through conflict path. This
patch fix this bug.
Signed-off-by: bibo.mao@gmail.com
----------------------------------------------------------------------
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 12cac5e..0632308 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -183,28 +183,29 @@ static unsigned long pat_x_mtrr_type(u64 start,
u64 end, unsigned long req_type)
static int chk_conflict(struct memtype *new, struct memtype *entry,
unsigned long *type)
{
- if (new->type != entry->type) {
- if (type) {
- new->type = entry->type;
- *type = entry->type;
- } else
- goto conflict;
- }
+ int err = -EBUSY;
+
+ if (new->type != entry->type)
+ goto conflict;
/* check overlaps with more than one entry in the list */
list_for_each_entry_continue(entry, &memtype_list, nd) {
if (new->end <= entry->start)
break;
- else if (new->type != entry->type)
+ else if (new->type != entry->type) {
+ err = -EINVAL;
goto conflict;
+ }
}
return 0;
conflict:
+ if (type)
+ *type = entry->type;
printk(KERN_INFO "%s:%d conflicting memory types "
"%Lx-%Lx %s<->%s\n", current->comm, current->pid, new->start,
new->end, cattr_name(new->type), cattr_name(entry->type));
- return -EBUSY;
+ return err;
}
static struct memtype *cached_entry;
@@ -278,9 +279,6 @@ int reserve_memtype(u64 start, u64 end, unsigned
long req_type,
new->end = end;
new->type = actual_type;
- if (new_type)
- *new_type = actual_type;
-
spin_lock(&memtype_lock);
if (cached_entry && start >= cached_start)
@@ -326,6 +324,9 @@ int reserve_memtype(u64 start, u64 end, unsigned
long req_type,
spin_unlock(&memtype_lock);
+ if (new_type)
+ *new_type = actual_type;
+
dprintk("reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s, ret %s\n",
start, end, cattr_name(new->type), cattr_name(req_type),
new_type ? cattr_name(*new_type) : "-");
@@ -474,15 +475,22 @@ void map_devmem(unsigned long pfn, unsigned long
size, pgprot_t vma_prot)
u64 addr = (u64)pfn << PAGE_SHIFT;
unsigned long flags;
unsigned long want_flags = (pgprot_val(vma_prot) & _PAGE_CACHE_MASK);
+ int err;
- reserve_memtype(addr, addr + size, want_flags, &flags);
- if (flags != want_flags) {
+ err = reserve_memtype(addr, addr + size, want_flags, &flags);
+ if (err == -EBUSY) {
printk(KERN_INFO
"%s:%d /dev/mem expected mapping type %s for %Lx-%Lx, got %s\n",
current->comm, current->pid,
cattr_name(want_flags),
addr, (unsigned long long)(addr + size),
cattr_name(flags));
+ } else if (err == -EINVAL) {
+ printk(KERN_INFO
+ "%s:%d /dev/mem has both mapping type %s and %s for %Lx-%Lx \n",
+ current->comm, current->pid,
+ cattr_name(want_flags), cattr_name(flags),
+ addr, (unsigned long long)(addr + size));
}
}
^ permalink raw reply [flat|nested] 2+ messages in thread* RE: [patch 1/1]pat: fix type check error on chk_conflict function
2008-10-12 13:52 [patch 1/1]pat: fix type check error on chk_conflict function bibo mao
@ 2008-10-13 16:49 ` Pallipadi, Venkatesh
0 siblings, 0 replies; 2+ messages in thread
From: Pallipadi, Venkatesh @ 2008-10-13 16:49 UTC (permalink / raw)
To: bibo mao, akpm
Cc: linux-kernel, Ingo Molnar, Siddha, Suresh B, Arjan van de Ven
No. This patch is not right. Inlined some comments below.
Thanks,
Venki
>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of bibo mao
>Sent: Sunday, October 12, 2008 6:52 AM
>To: akpm@linux-foundation.org
>Cc: linux-kernel@vger.kernel.org
>Subject: [patch 1/1]pat: fix type check error on chk_conflict function
>
> On chk_conflict() function, if new->type does not match entry->type
> and type is not NULL, this does not go through conflict path. This
> patch fix this bug.
No. If the type is not NULL, then new->type != entry->type is not a conflict.
In this case, the caller is looking to map some region with some type and
willing to take a type as output of reserve_memtype in "type" parameter.
So, reserve should succeed with proper type returned to the caller.
But, if type parameter is NULL, then the caller of reserve cannot really
take an output and strictly wants req_type mapping. In that case, if we have
new->type != entry->type we have to return an error. That is what the current
code is doing.
The second list_for_each_entry loop below checks for conflicting requirements
which cannot be satisfied and that will be an error in both type being NULL
and not NULL cases.
>
> Signed-off-by: bibo.mao@gmail.com
>
>----------------------------------------------------------------------
>diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
>index 12cac5e..0632308 100644
>--- a/arch/x86/mm/pat.c
>+++ b/arch/x86/mm/pat.c
>@@ -183,28 +183,29 @@ static unsigned long pat_x_mtrr_type(u64 start,
>u64 end, unsigned long req_type)
> static int chk_conflict(struct memtype *new, struct memtype *entry,
> unsigned long *type)
> {
>- if (new->type != entry->type) {
>- if (type) {
>- new->type = entry->type;
>- *type = entry->type;
>- } else
>- goto conflict;
>- }
>+ int err = -EBUSY;
>+
>+ if (new->type != entry->type)
>+ goto conflict;
>
> /* check overlaps with more than one entry in the list */
> list_for_each_entry_continue(entry, &memtype_list, nd) {
> if (new->end <= entry->start)
> break;
>- else if (new->type != entry->type)
>+ else if (new->type != entry->type) {
>+ err = -EINVAL;
> goto conflict;
>+ }
> }
> return 0;
>
> conflict:
>+ if (type)
>+ *type = entry->type;
> printk(KERN_INFO "%s:%d conflicting memory types "
> "%Lx-%Lx %s<->%s\n", current->comm,
>current->pid, new->start,
> new->end, cattr_name(new->type),
>cattr_name(entry->type));
>- return -EBUSY;
>+ return err;
> }
>
> static struct memtype *cached_entry;
>@@ -278,9 +279,6 @@ int reserve_memtype(u64 start, u64 end, unsigned
>long req_type,
> new->end = end;
> new->type = actual_type;
>
>- if (new_type)
>- *new_type = actual_type;
>-
> spin_lock(&memtype_lock);
>
> if (cached_entry && start >= cached_start)
>@@ -326,6 +324,9 @@ int reserve_memtype(u64 start, u64 end, unsigned
>long req_type,
>
> spin_unlock(&memtype_lock);
>
>+ if (new_type)
>+ *new_type = actual_type;
>+
> dprintk("reserve_memtype added 0x%Lx-0x%Lx, track %s,
>req %s, ret %s\n",
> start, end, cattr_name(new->type),
>cattr_name(req_type),
> new_type ? cattr_name(*new_type) : "-");
>@@ -474,15 +475,22 @@ void map_devmem(unsigned long pfn, unsigned long
>size, pgprot_t vma_prot)
> u64 addr = (u64)pfn << PAGE_SHIFT;
> unsigned long flags;
> unsigned long want_flags = (pgprot_val(vma_prot) &
>_PAGE_CACHE_MASK);
>+ int err;
>
>- reserve_memtype(addr, addr + size, want_flags, &flags);
>- if (flags != want_flags) {
>+ err = reserve_memtype(addr, addr + size, want_flags, &flags);
>+ if (err == -EBUSY) {
> printk(KERN_INFO
> "%s:%d /dev/mem expected mapping type %s for
>%Lx-%Lx, got %s\n",
> current->comm, current->pid,
> cattr_name(want_flags),
> addr, (unsigned long long)(addr + size),
> cattr_name(flags));
>+ } else if (err == -EINVAL) {
>+ printk(KERN_INFO
>+ "%s:%d /dev/mem has both mapping type %s and
>%s for %Lx-%Lx \n",
>+ current->comm, current->pid,
>+ cattr_name(want_flags), cattr_name(flags),
>+ addr, (unsigned long long)(addr + size));
> }
> }
>--
>To unsubscribe from this list: send the line "unsubscribe
>linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2008-10-13 16:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-12 13:52 [patch 1/1]pat: fix type check error on chk_conflict function bibo mao
2008-10-13 16:49 ` Pallipadi, Venkatesh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®