Re: [RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
On Tue, Sep 30, 2014 at 07:27:51AM -0700, Dave Hansen wrote: > On 09/30/2014 05:41 AM, Frantisek Hrbata wrote: > > Does it make sense to replace "count" with "size" so it's consistent with > > the > > rest or do you prefer "nr_bytes" or as Dave proposed "len_bytes"? > > I don't care what it is as long as it has a unit in it. Hi Dave/Thomas, I sent v2 of this patch set, which uses the "len_bytes". Again, I'm sorry I forgot to fix this the first time. Many thanks -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 4/4] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 77a13f8..788f2b7 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count <= __pa(high_memory);
+ return arch_pfn_possible((addr + count) >> PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/4] x86: add phys addr validity check for /dev/mem mmap
t main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die("cannot get page size");
fd = open("/dev/mem", O_RDONLY|O_LARGEFILE);
if (fd == -1)
die("cannot open /dev/mem");
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die("cannot mmap");
c = map[0];
if (munmap(map, ps) == -1)
die("cannot munmap");
if (close(fd) == -1)
die("cannot close");
return 0;
}
-8<--
V3: use len_bytes instead of count, thanks to Dave Hansen and Thomas Gleixner
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..49ede3c 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 919b912..77a13f8 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include
#include
+#include "physaddr.h"
+
struct va_alignment __read_mostly va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count <= __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes)
+{
+ return arch_pfn_possible(pfn + (len_bytes >> PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/4] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys & PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start >> PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start >> PAGE_SHIFT) && phys <= __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys >> PAGE_SHIFT))
+ if (page_is_ram(phys >> PAGE_SHIFT) && phys <= __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/4] x86: /dev/mem fixes
This is a second version of the patch set. The only change is in 2/4 x86: add phys addr validity check for /dev/mem mmap where the "count" was replaced with "len_bytes" for better readability. The rest is just a refresh because of this change. The original message follows ... Hi all, this is a combination of two patch sets I sent a while ago 1) Prevent possible PTE corruption with /dev/mem mmap 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory The original thread with both patch sets can be found here https://lkml.org/lkml/2014/8/14/229 lkml: <1408025927-16826-1-git-send-email-fhrb...@redhat.com> 1) Prevent possible PTE corruption with /dev/mem mmap x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap Many thanks goes to Dave Hansen, who helped with the "final" check. Other than that it did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not "directly" related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What this patch set does is using the existing interface to implement x86 specific check in the least invasive way. Anyway I tried to remove the high_memory check with a follow-up patch set 2) 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (4): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/include/asm/io.h | 4 arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 4 files changed, 29 insertions(+), 5 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/4] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr >> boot_cpu_data.x86_phys_bits);
+ return !(pfn >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr >> PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
On Mon, Sep 29, 2014 at 10:15:28PM +0200, Thomas Gleixner wrote:
> On Mon, 29 Sep 2014, Frantisek Hrbata wrote:
> > V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
>
> AFAICT, Dave also asked you to change size_t count into something more
> intuitive, i.e. nr_bytes or such.
Hi,
mea culpa, I for unknown reason changed it from "size" to "count". I guess some
cut mess. The correct prototype used elsewhere in kernel is
int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
Does it make sense to replace "count" with "size" so it's consistent with the
rest or do you prefer "nr_bytes" or as Dave proposed "len_bytes"?
I will fix this and I'm sorry Dave I did not change it as discussed. It totally
slipped my mind.
Many thanks Thomas.
>
> > +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
>
> And right he is. I really had to look twice to see that count is
> actually number of bytes and not number of pages, which is what you
> expect after pfn.
>
> > +{
> > + return arch_pfn_possible(pfn + (count >> PAGE_SHIFT));
> > +}
>
> Thanks,
>
> tglx
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
On Mon, Sep 29, 2014 at 10:15:28PM +0200, Thomas Gleixner wrote:
On Mon, 29 Sep 2014, Frantisek Hrbata wrote:
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
AFAICT, Dave also asked you to change size_t count into something more
intuitive, i.e. nr_bytes or such.
Hi,
mea culpa, I for unknown reason changed it from size to count. I guess some
cutpaste mess. The correct prototype used elsewhere in kernel is
int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
Does it make sense to replace count with size so it's consistent with the
rest or do you prefer nr_bytes or as Dave proposed len_bytes?
I will fix this and I'm sorry Dave I did not change it as discussed. It totally
slipped my mind.
Many thanks Thomas.
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
And right he is. I really had to look twice to see that count is
actually number of bytes and not number of pages, which is what you
expect after pfn.
+{
+ return arch_pfn_possible(pfn + (count PAGE_SHIFT));
+}
Thanks,
tglx
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/4] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include asm/processor.h
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr boot_cpu_data.x86_phys_bits);
+ return !(pfn (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/4] x86: /dev/mem fixes
This is a second version of the patch set. The only change is in 2/4 x86: add phys addr validity check for /dev/mem mmap where the count was replaced with len_bytes for better readability. The rest is just a refresh because of this change. The original message follows ... Hi all, this is a combination of two patch sets I sent a while ago 1) Prevent possible PTE corruption with /dev/mem mmap 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory The original thread with both patch sets can be found here https://lkml.org/lkml/2014/8/14/229 lkml: 1408025927-16826-1-git-send-email-fhrb...@redhat.com 1) Prevent possible PTE corruption with /dev/mem mmap x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap Many thanks goes to Dave Hansen, who helped with the final check. Other than that it did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not directly related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What this patch set does is using the existing interface to implement x86 specific check in the least invasive way. Anyway I tried to remove the high_memory check with a follow-up patch set 2) 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (4): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/include/asm/io.h | 4 arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 4 files changed, 29 insertions(+), 5 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/4] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start PAGE_SHIFT) phys = __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys PAGE_SHIFT))
+ if (page_is_ram(phys PAGE_SHIFT) phys = __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 4/4] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 77a13f8..788f2b7 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count = __pa(high_memory);
+ return arch_pfn_possible((addr + count) PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes)
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/4] x86: add phys addr validity check for /dev/mem mmap
OFFSET 0x200
int main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die(cannot get page size);
fd = open(/dev/mem, O_RDONLY|O_LARGEFILE);
if (fd == -1)
die(cannot open /dev/mem);
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die(cannot mmap);
c = map[0];
if (munmap(map, ps) == -1)
die(cannot munmap);
if (close(fd) == -1)
die(cannot close);
return 0;
}
-8--
V3: use len_bytes instead of count, thanks to Dave Hansen and Thomas Gleixner
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..49ede3c 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 919b912..77a13f8 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include linux/sched.h
#include asm/elf.h
+#include physaddr.h
+
struct va_alignment __read_mostly va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm-get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t len_bytes)
+{
+ return arch_pfn_possible(pfn + (len_bytes PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
On Tue, Sep 30, 2014 at 07:27:51AM -0700, Dave Hansen wrote: On 09/30/2014 05:41 AM, Frantisek Hrbata wrote: Does it make sense to replace count with size so it's consistent with the rest or do you prefer nr_bytes or as Dave proposed len_bytes? I don't care what it is as long as it has a unit in it. Hi Dave/Thomas, I sent v2 of this patch set, which uses the len_bytes. Again, I'm sorry I forgot to fix this the first time. Many thanks -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 1/4] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr >> boot_cpu_data.x86_phys_bits);
+ return !(pfn >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr >> PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
t main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die("cannot get page size");
fd = open("/dev/mem", O_RDONLY|O_LARGEFILE);
if (fd == -1)
die("cannot open /dev/mem");
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die("cannot mmap");
c = map[0];
if (munmap(map, ps) == -1)
die("cannot munmap");
if (close(fd) == -1)
die("cannot close");
return 0;
}
-----8<--
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 919b912..c8acb10 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include
#include
+#include "physaddr.h"
+
struct va_alignment __read_mostly va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count <= __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ return arch_pfn_possible(pfn + (count >> PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 3/4] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys & PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start >> PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start >> PAGE_SHIFT) && phys <= __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys >> PAGE_SHIFT))
+ if (page_is_ram(phys >> PAGE_SHIFT) && phys <= __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 4/4] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index c8acb10..7fa0242 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count <= __pa(high_memory);
+ return arch_pfn_possible((addr + count) >> PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 0/4] x86: /dev/mem fixes
Hi all, this is a combination of two patch sets I sent a while ago 1) Prevent possible PTE corruption with /dev/mem mmap 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory The original thread with both patch sets can be found here https://lkml.org/lkml/2014/8/14/229 lkml: <1408025927-16826-1-git-send-email-fhrb...@redhat.com> 1) Prevent possible PTE corruption with /dev/mem mmap x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap Many thanks goes to Dave Hansen, who helped with the "final" check. Other than that it did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not "directly" related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What this patch set does is using the existing interface to implement x86 specific check in the least invasive way. Anyway I tried to remove the high_memory check with a follow-up patch set 2) 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (4): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/include/asm/io.h | 4 arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 4 files changed, 29 insertions(+), 5 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 0/4] x86: /dev/mem fixes
Hi all, this is a combination of two patch sets I sent a while ago 1) Prevent possible PTE corruption with /dev/mem mmap 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory The original thread with both patch sets can be found here https://lkml.org/lkml/2014/8/14/229 lkml: 1408025927-16826-1-git-send-email-fhrb...@redhat.com 1) Prevent possible PTE corruption with /dev/mem mmap x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap Many thanks goes to Dave Hansen, who helped with the final check. Other than that it did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not directly related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What this patch set does is using the existing interface to implement x86 specific check in the least invasive way. Anyway I tried to remove the high_memory check with a follow-up patch set 2) 2) x86: allow read/write /dev/mem to access non-system RAM above high_memory x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (4): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/include/asm/io.h | 4 arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 4 files changed, 29 insertions(+), 5 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 4/4] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index c8acb10..7fa0242 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count = __pa(high_memory);
+ return arch_pfn_possible((addr + count) PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 3/4] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start PAGE_SHIFT) phys = __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys PAGE_SHIFT))
+ if (page_is_ram(phys PAGE_SHIFT) phys = __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 2/4] x86: add phys addr validity check for /dev/mem mmap
OFFSET 0x200
int main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die(cannot get page size);
fd = open(/dev/mem, O_RDONLY|O_LARGEFILE);
if (fd == -1)
die(cannot open /dev/mem);
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die(cannot mmap);
c = map[0];
if (munmap(map, ps) == -1)
die(cannot munmap);
if (close(fd) == -1)
die(cannot close);
return 0;
}
-8--
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 919b912..c8acb10 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include linux/sched.h
#include asm/elf.h
+#include physaddr.h
+
struct va_alignment __read_mostly va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm-get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ return arch_pfn_possible(pfn + (count PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH 1/4] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include asm/processor.h
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr boot_cpu_data.x86_phys_bits);
+ return !(pfn (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys & PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start >> PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start >> PAGE_SHIFT) && phys <= __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys >> PAGE_SHIFT))
+ if (page_is_ram(phys >> PAGE_SHIFT) && phys <= __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] x86: allow read/write /dev/mem to access non-system RAM above high_memory
Hi, this patch set depends on the following patches posted earlier. [PATCH V2 1/2] x86: add arch_pfn_possible helper lkml: Message-Id: <1408103043-31015-2-git-send-email-fhrb...@redhat.com> [PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap lkml: Message-Id: <1408103043-31015-3-git-send-email-fhrb...@redhat.com> This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (2): x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 110473e..16f222d 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count <= __pa(high_memory);
+ return arch_pfn_possible((addr + count) >> PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] x86: remove high_memory check from valid_phys_addr_range
There is no need to block read/write access to /dev/mem for phys. addr. above
high_memory for non-system RAM. The only limitation should be
boot_cpu_data.x86_phys_bits(max phys. addr. size).
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 110473e..16f222d 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -127,7 +127,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
int valid_phys_addr_range(phys_addr_t addr, size_t count)
{
- return addr + count = __pa(high_memory);
+ return arch_pfn_possible((addr + count) PAGE_SHIFT);
}
int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr
So far (xlate|unxlate)_dev_mem_ptr for read/write /dev/mem relies on a generic
high_memory check in valid_phys_addr_range(), which does not allow to access any
memory above high_memory whatsoever. By adding the high_memory check to
(xlate|unxlate)_dev_mem_ptr, it still will be possible to use __va safely for
kernel mapped memory and it will also allow read/write to access non-system RAM
above high_memory once the high_memory check is removed from
valid_phys_addr_range.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/ioremap.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..1ae7323 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -320,8 +320,11 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void *addr;
unsigned long start = phys PAGE_MASK;
- /* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start PAGE_SHIFT))
+ /*
+* If page is RAM and is mapped by kernel, we can use __va.
+* Otherwise ioremap and unmap.
+*/
+ if (page_is_ram(start PAGE_SHIFT) phys = __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +336,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys PAGE_SHIFT))
+ if (page_is_ram(phys PAGE_SHIFT) phys = __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr PAGE_MASK));
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] x86: allow read/write /dev/mem to access non-system RAM above high_memory
Hi, this patch set depends on the following patches posted earlier. [PATCH V2 1/2] x86: add arch_pfn_possible helper lkml: Message-Id: 1408103043-31015-2-git-send-email-fhrb...@redhat.com [PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap lkml: Message-Id: 1408103043-31015-3-git-send-email-fhrb...@redhat.com This is an attempt to remove the high_memory limit for the read/write access to /dev/mem. IMHO there is no reason for this limit on x86. It is presented in the generic valid_phys_addr_range, which is used only by (read|write)_mem. IIUIC it's main purpose is for the generic xlate_dev_mem_ptr, which is using only the direct kernel mapping __va translation. Since the valid_phys_addr_range is called as the first check in (read|write)_mem, it basically does not allow to access anything above high_memory on x86. The first patch adds high_memory check to x86's (xlate|unxlate)_dev_mem_ptr, so the direct kernel mapping can be safely used for system RAM bellow high_memory. This is IMHO the only valid reason to use high_memory check in (read|write)_mem. The second patch removes the high_memory check from valid_phys_addr_range, allowing read/write to access non-system RAM above high_memory. So far this was possible only by using mmap. I hope I haven't overlooked something. Many thanks Frantisek Hrbata (2): x86: add high_memory check to (xlate|unxlate)_dev_mem_ptr x86: remove high_memory check from valid_phys_addr_range arch/x86/mm/ioremap.c | 9 ++--- arch/x86/mm/mmap.c| 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap
On Fri, Aug 15, 2014 at 11:10:25AM -0700, Dave Hansen wrote:
> On 08/15/2014 04:44 AM, Frantisek Hrbata wrote:
> > +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> > +{
> > + return addr + count <= __pa(high_memory);
> > +}
> > +
> > +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> > +{
> > + return arch_pfn_possible(pfn + (count >> PAGE_SHIFT));
> > +}
>
> It definitely fixes the issue as you described it.
Hi Dave,
many thanks for your time and help with this!
>
> It's a bit unfortunate that the highmem check isn't tied in to the
> _existing_ /dev/mem limitations in some way, but it's not a deal breaker
> for me.
Agreed, I will do some more testing with the "patch" I proposed earlier in our
discussion. Meaning the one moving the high_memory check out of the
valid_phys_addr_range() to the xlate_dev_mem_ptr() for x86. IMHO this should
work fine and it should remove the high_memory limitation. But I for sure can be
missing something. If the testing goes well I will post the patch.
>
> The only other thing is to make sure this doesn't add some limitation to
> 64-bit where we can't map things above the end of memory (end of memory
> == high_memory on 64-bit). As long as you've done this, I can't see a
> downside.
Yes, from what I have tested, this patch should not introduce any new
limitation, except fixing the PTE problem. Also please note
that this kind of check is already done in ioremap by calling the
phys_addr_valid(). Again, I hope I haven't overlooked something.
Peter and others: Could you please consider including this fix? Of course only
if you do not have any other objections or problems with it.
Many thanks!
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap
On Fri, Aug 15, 2014 at 11:10:25AM -0700, Dave Hansen wrote:
On 08/15/2014 04:44 AM, Frantisek Hrbata wrote:
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ return arch_pfn_possible(pfn + (count PAGE_SHIFT));
+}
It definitely fixes the issue as you described it.
Hi Dave,
many thanks for your time and help with this!
It's a bit unfortunate that the highmem check isn't tied in to the
_existing_ /dev/mem limitations in some way, but it's not a deal breaker
for me.
Agreed, I will do some more testing with the patch I proposed earlier in our
discussion. Meaning the one moving the high_memory check out of the
valid_phys_addr_range() to the xlate_dev_mem_ptr() for x86. IMHO this should
work fine and it should remove the high_memory limitation. But I for sure can be
missing something. If the testing goes well I will post the patch.
The only other thing is to make sure this doesn't add some limitation to
64-bit where we can't map things above the end of memory (end of memory
== high_memory on 64-bit). As long as you've done this, I can't see a
downside.
Yes, from what I have tested, this patch should not introduce any new
limitation, except fixing the PTE problem. Also please note
that this kind of check is already done in ioremap by calling the
phys_addr_valid(). Again, I hope I haven't overlooked something.
Peter and others: Could you please consider including this fix? Of course only
if you do not have any other objections or problems with it.
Many thanks!
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap
t main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die("cannot get page size");
fd = open("/dev/mem", O_RDONLY|O_LARGEFILE);
if (fd == -1)
die("cannot open /dev/mem");
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die("cannot mmap");
c = map[0];
if (munmap(map, ps) == -1)
die("cannot munmap");
if (close(fd) == -1)
die("cannot close");
return 0;
}
-----8<--
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..110473e 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include
#include
+#include "physaddr.h"
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count <= __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ return arch_pfn_possible(pfn + (count >> PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 0/2] Prevent possible PTE corruption with /dev/mem mmap
Thanks to Dave Hansen for pointing out problems related to the pfn check in the first version. I tried to fix it by adding a new arch_pfn_possible helper to arch/x86/mm/physaddr.h. Please note that I'm not quite sure about the name and the location(physaddr.h). Maybe we can keep the check directly in the valid_mmap_phys_addr_range. I will leave this to a discussion and fix it if required. Question: Do we need the CONFIG_PHYS_ADDR_T_64BIT ifdef? The boot_cpu_data.x86_phys_bits is set for all x86. So at this point it seems to me more like an "optimization" for x86_32 or something kept from historic reasons. I'm just curious and I of course may be missing something. Many thanks Frantisek Hrbata (2): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap arch/x86/include/asm/io.h | 4 arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 3 files changed, 23 insertions(+), 2 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 1/2] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr >> boot_cpu_data.x86_phys_bits);
+ return !(pfn >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr >> PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
self-nack As pointed by Dave Hansen, the check is just wrong. I will post V2. Many thanks Dave! -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
self-nack As pointed by Dave Hansen, the check is just wrong. I will post V2. Many thanks Dave! -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 0/2] Prevent possible PTE corruption with /dev/mem mmap
Thanks to Dave Hansen for pointing out problems related to the pfn check in the first version. I tried to fix it by adding a new arch_pfn_possible helper to arch/x86/mm/physaddr.h. Please note that I'm not quite sure about the name and the location(physaddr.h). Maybe we can keep the check directly in the valid_mmap_phys_addr_range. I will leave this to a discussion and fix it if required. Question: Do we need the CONFIG_PHYS_ADDR_T_64BIT ifdef? The boot_cpu_data.x86_phys_bits is set for all x86. So at this point it seems to me more like an optimization for x86_32 or something kept from historic reasons. I'm just curious and I of course may be missing something. Many thanks Frantisek Hrbata (2): x86: add arch_pfn_possible helper x86: add phys addr validity check for /dev/mem mmap arch/x86/include/asm/io.h | 4 arch/x86/mm/mmap.c| 12 arch/x86/mm/physaddr.h| 9 +++-- 3 files changed, 23 insertions(+), 2 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 1/2] x86: add arch_pfn_possible helper
Add helper to check maximum possible pfn on x86. Also make the current
phys_addr_valid helper using it internally.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/mm/physaddr.h | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/physaddr.h b/arch/x86/mm/physaddr.h
index a3cd5a0..9df8e3a 100644
--- a/arch/x86/mm/physaddr.h
+++ b/arch/x86/mm/physaddr.h
@@ -1,10 +1,15 @@
#include asm/processor.h
-static inline int phys_addr_valid(resource_size_t addr)
+static inline int arch_pfn_possible(unsigned long pfn)
{
#ifdef CONFIG_PHYS_ADDR_T_64BIT
- return !(addr boot_cpu_data.x86_phys_bits);
+ return !(pfn (boot_cpu_data.x86_phys_bits - PAGE_SHIFT));
#else
return 1;
#endif
}
+
+static inline int phys_addr_valid(resource_size_t addr)
+{
+ return arch_pfn_possible(addr PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 2/2] x86: add phys addr validity check for /dev/mem mmap
OFFSET 0x200
int main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die(cannot get page size);
fd = open(/dev/mem, O_RDONLY|O_LARGEFILE);
if (fd == -1)
die(cannot open /dev/mem);
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die(cannot mmap);
c = map[0];
if (munmap(map, ps) == -1)
die(cannot munmap);
if (close(fd) == -1)
die(cannot close);
return 0;
}
-8--
V2: fix pfn check in valid_mmap_phys_addr_range, thanks to Dave Hansen
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 12
2 files changed, 16 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..110473e 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include linux/sched.h
#include asm/elf.h
+#include physaddr.h
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,13 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm-get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ return arch_pfn_possible(pfn + (count PAGE_SHIFT));
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
On Thu, Aug 14, 2014 at 10:20:53AM -0700, H. Peter Anvin wrote:
> On 08/14/2014 09:36 AM, Dave Hansen wrote:
> > Thanks for dredging this back up!
> >
> > On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
> >> +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> >> +{
> >> + return addr + count <= __pa(high_memory);
> >> +}
> >
> > Is this correct on 32-bit? It would limit /dev/mem to memory below 896MB.
> >
>
> Last I checked, /dev/mem was already broken for highmem... which is
> actually a problem. I would prefer to see it fixed.
>
> -hpa
>
Hi Peter,
thank you for jumping in again. I'm not going to pretent I fully understand this
code, as proven by Dave :), but wouldn't it help just to move the high_memory
check directly to the xlate_dev_mem_ptr. Meaning no high_memory check in
valid_phys_addr_range for x86. I sent more info in my reply to Dave's mail.
Again many thanks.
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
On Thu, Aug 14, 2014 at 09:36:03AM -0700, Dave Hansen wrote:
> Thanks for dredging this back up!
>
> On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
> > +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> > +{
> > + return addr + count <= __pa(high_memory);
> > +}
>
> Is this correct on 32-bit? It would limit /dev/mem to memory below 896MB.
Unfortunatelly this is how it works right now. Please note that at this moment
x86 is using the default checks from drivers/char/mem.c. The
valid_phys_addr_range is used just for read/write. Meaning yes, you cannot
access
/dev/mem above high_memory via read/write, which is 896MB on x86_32.
I simply copied this generic check. There is no change compared to the current
behaviour.
BTW I think this can be simply fixed by moving the high_memory check directly to
the xlate_dev_mem_ptr function.
Something like the following. Please note this is not even compile tested.
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..7ebc241 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -321,7 +321,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
unsigned long start = phys & PAGE_MASK;
/* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start >> PAGE_SHIFT))
+ if (page_is_ram(start >> PAGE_SHIFT) && phys <= __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +333,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys >> PAGE_SHIFT))
+ if (page_is_ram(phys >> PAGE_SHIFT) && phys <= __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
IIUIC the whole high_memory check is here only because of the kernel identity
mapping and as a generic check because of the generic xlate_dev_mem_ptr.
include/asm-generic/io.h: #define xlate_dev_mem_ptr(p)__va(p)
>
> > +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> > +{
>
> Nit: please add units to things like "count". len_bytes would be nice
> for this kind of thing, especially since it's passed *with* a pfn it
> would be easy to think it is a count in pages.
Sure I have no problem with this. But please note that I just took the already
used/presented interface from drivers/char/mem.c.
>
> > + /* pgoff + count overflow is checked in do_mmap_pgoff */
> > + pfn += count >> PAGE_SHIFT;
> > +
> > + if (pfn >> BITS_PER_LONG - PAGE_SHIFT)
> > + return -EOVERFLOW;
>
> Is this -EOVERFLOW correct? It is called like this:
>
> > static int mmap_mem(struct file *file, struct vm_area_struct *vma)
> > {
> > if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
> > return -EINVAL;
>
> So I think we need to return true/false:0/1. -EOVERFLOW would be true,
> and that if() would pass.
Facepalm, sure this is completely wrong. We of course need to return zero. I
thought it would be more descriptive to use -EOVERFLOW, even though we get
-EINVAL in the end. I will fix this. Many thanks for pointing this out.
>
> > + return phys_addr_valid(pfn << PAGE_SHIFT);
> > +}
>
> Maybe I'm dumb, but it took me a minute to figure out what you were
> trying to do with the: "(pfn >> BITS_PER_LONG - PAGE_SHIFT)". In any
> case, I think it is wrong on 32-bit.
>
> On 32-bit, BITS_PER_LONG=32, and PAGE_SIZE=12, and a paddr=0x1
> or pfn=0x10 (4GB) is perfectly valid with PAE enabled. But, this
> code pfn>>(32-12) would result in 0x1 and return -EOVERFLOW.
Right, I did not realized this.
>
> I think something like this would be easier to read and actually work on
> 32-bit:
>
> static inline int arch_pfn_possible(unsigned long pfn)
> {
> unsigned long max_arch_pfn = 1UL << (boot_cpu_data.x86_phys_bits -
> PAGE_SHIFT);
> return pfn < max_arch_pfn;
> }
Actually I wanted to use exactly this instead of calling phys_addr_valid,
because
we can avoid the whole overflow check, but it seemed natural to use what is
already available. But you are right for sure. This needs to be fixed also.
Dave, many thanks for your feedback, it's really appreciated!
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/1] Prevent possible PTE corruption with /dev/mem mmap
Hi all, after some time this issue popped up again. Please note that the patch was send to lkml two times. https://lkml.org/lkml/2013/4/2/297 lkml: <1364905733-23937-1-git-send-email-fhrb...@redhat.com> https://lkml.org/lkml/2013/10/2/359 lkml: <20131002160514.GA25471@localhost.localdomain> It did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not "directly" related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What the patch does is using the existing interface to implement x86 specific check in the least invasive way. Peter: I by no means want to be pushy. Just that after I looked into this a little bit more, I don't see a better and more straightforward way how to fix this. I will be grateful for any suggestions and help. If we want/need to fix this in a different way, I can for sure try, but I will need at least some guidance. So I'm posting this once more with a hope it will get more attention or at least to start the discussion how/if this should be fixed. The patch is the same except I added a check for phys addr overflow before calling phys_addr_valid. Maybe this check should be in do_mmap_pgoff. Many thanks Frantisek Hrbata (1): x86: add phys addr validity check for /dev/mem mmap arch/x86/include/asm/io.h | 4 arch/x86/mm/mmap.c| 18 ++ 2 files changed, 22 insertions(+) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
t main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die("cannot get page size");
fd = open("/dev/mem", O_RDONLY|O_LARGEFILE);
if (fd == -1)
die("cannot open /dev/mem");
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die("cannot mmap");
c = map[0];
if (munmap(map, ps) == -1)
die("cannot munmap");
if (close(fd) == -1)
die("cannot close");
return 0;
}
-8<--
Signed-off-by: Frantisek Hrbata
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 18 ++
2 files changed, 22 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..b5be2ad 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include
#include
+#include "physaddr.h"
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,19 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count <= __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ /* pgoff + count overflow is checked in do_mmap_pgoff */
+ pfn += count >> PAGE_SHIFT;
+
+ if (pfn >> BITS_PER_LONG - PAGE_SHIFT)
+ return -EOVERFLOW;
+
+ return phys_addr_valid(pfn << PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
OFFSET 0x200
int main(int argc, char *argv[])
{
int fd;
long ps;
char *map;
char c;
ps = sysconf(_SC_PAGE_SIZE);
if (ps == -1)
die(cannot get page size);
fd = open(/dev/mem, O_RDONLY|O_LARGEFILE);
if (fd == -1)
die(cannot open /dev/mem);
map = (char *)syscall(SYS_mmap2, NULL, ps, PROT_READ, MAP_SHARED, fd,
OFFSET);
if (map == MAP_FAILED)
die(cannot mmap);
c = map[0];
if (munmap(map, ps) == -1)
die(cannot munmap);
if (close(fd) == -1)
die(cannot close);
return 0;
}
-8--
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 18 ++
2 files changed, 22 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..55c59d5 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -243,6 +243,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..b5be2ad 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include linux/sched.h
#include asm/elf.h
+#include physaddr.h
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,19 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm-get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ /* pgoff + count overflow is checked in do_mmap_pgoff */
+ pfn += count PAGE_SHIFT;
+
+ if (pfn BITS_PER_LONG - PAGE_SHIFT)
+ return -EOVERFLOW;
+
+ return phys_addr_valid(pfn PAGE_SHIFT);
+}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/1] Prevent possible PTE corruption with /dev/mem mmap
Hi all, after some time this issue popped up again. Please note that the patch was send to lkml two times. https://lkml.org/lkml/2013/4/2/297 lkml: 1364905733-23937-1-git-send-email-fhrb...@redhat.com https://lkml.org/lkml/2013/10/2/359 lkml: 20131002160514.GA25471@localhost.localdomain It did not get much attention, except H. Peter Anvin's complain that having two checks for mmap and read/write for /dev/mem access is ridiculous. I for sure do not object to this, but AFAICT it's not that simple to unify them and it's not directly related to the PTE corruption. Please note that there are other archs(ia64, arm) using these check. But I for sure can be missing something. What the patch does is using the existing interface to implement x86 specific check in the least invasive way. Peter: I by no means want to be pushy. Just that after I looked into this a little bit more, I don't see a better and more straightforward way how to fix this. I will be grateful for any suggestions and help. If we want/need to fix this in a different way, I can for sure try, but I will need at least some guidance. So I'm posting this once more with a hope it will get more attention or at least to start the discussion how/if this should be fixed. The patch is the same except I added a check for phys addr overflow before calling phys_addr_valid. Maybe this check should be in do_mmap_pgoff. Many thanks Frantisek Hrbata (1): x86: add phys addr validity check for /dev/mem mmap arch/x86/include/asm/io.h | 4 arch/x86/mm/mmap.c| 18 ++ 2 files changed, 22 insertions(+) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
On Thu, Aug 14, 2014 at 09:36:03AM -0700, Dave Hansen wrote:
Thanks for dredging this back up!
On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
Is this correct on 32-bit? It would limit /dev/mem to memory below 896MB.
Unfortunatelly this is how it works right now. Please note that at this moment
x86 is using the default checks from drivers/char/mem.c. The
valid_phys_addr_range is used just for read/write. Meaning yes, you cannot
access
/dev/mem above high_memory via read/write, which is 896MB on x86_32.
I simply copied this generic check. There is no change compared to the current
behaviour.
BTW I think this can be simply fixed by moving the high_memory check directly to
the xlate_dev_mem_ptr function.
Something like the following. Please note this is not even compile tested.
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index baff1da..7ebc241 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -321,7 +321,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
unsigned long start = phys PAGE_MASK;
/* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
- if (page_is_ram(start PAGE_SHIFT))
+ if (page_is_ram(start PAGE_SHIFT) phys = __pa(high_memory))
return __va(phys);
addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
@@ -333,7 +333,7 @@ void *xlate_dev_mem_ptr(unsigned long phys)
void unxlate_dev_mem_ptr(unsigned long phys, void *addr)
{
- if (page_is_ram(phys PAGE_SHIFT))
+ if (page_is_ram(phys PAGE_SHIFT) phys = __pa(high_memory))
return;
iounmap((void __iomem *)((unsigned long)addr PAGE_MASK));
IIUIC the whole high_memory check is here only because of the kernel identity
mapping and as a generic check because of the generic xlate_dev_mem_ptr.
include/asm-generic/io.h: #define xlate_dev_mem_ptr(p)__va(p)
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
Nit: please add units to things like count. len_bytes would be nice
for this kind of thing, especially since it's passed *with* a pfn it
would be easy to think it is a count in pages.
Sure I have no problem with this. But please note that I just took the already
used/presented interface from drivers/char/mem.c.
+ /* pgoff + count overflow is checked in do_mmap_pgoff */
+ pfn += count PAGE_SHIFT;
+
+ if (pfn BITS_PER_LONG - PAGE_SHIFT)
+ return -EOVERFLOW;
Is this -EOVERFLOW correct? It is called like this:
static int mmap_mem(struct file *file, struct vm_area_struct *vma)
{
if (!valid_mmap_phys_addr_range(vma-vm_pgoff, size))
return -EINVAL;
So I think we need to return true/false:0/1. -EOVERFLOW would be true,
and that if() would pass.
Facepalm, sure this is completely wrong. We of course need to return zero. I
thought it would be more descriptive to use -EOVERFLOW, even though we get
-EINVAL in the end. I will fix this. Many thanks for pointing this out.
+ return phys_addr_valid(pfn PAGE_SHIFT);
+}
Maybe I'm dumb, but it took me a minute to figure out what you were
trying to do with the: (pfn BITS_PER_LONG - PAGE_SHIFT). In any
case, I think it is wrong on 32-bit.
On 32-bit, BITS_PER_LONG=32, and PAGE_SIZE=12, and a paddr=0x1
or pfn=0x10 (4GB) is perfectly valid with PAE enabled. But, this
code pfn(32-12) would result in 0x1 and return -EOVERFLOW.
Right, I did not realized this.
I think something like this would be easier to read and actually work on
32-bit:
static inline int arch_pfn_possible(unsigned long pfn)
{
unsigned long max_arch_pfn = 1UL (boot_cpu_data.x86_phys_bits -
PAGE_SHIFT);
return pfn max_arch_pfn;
}
Actually I wanted to use exactly this instead of calling phys_addr_valid,
because
we can avoid the whole overflow check, but it seemed natural to use what is
already available. But you are right for sure. This needs to be fixed also.
Dave, many thanks for your feedback, it's really appreciated!
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap
On Thu, Aug 14, 2014 at 10:20:53AM -0700, H. Peter Anvin wrote:
On 08/14/2014 09:36 AM, Dave Hansen wrote:
Thanks for dredging this back up!
On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
Is this correct on 32-bit? It would limit /dev/mem to memory below 896MB.
Last I checked, /dev/mem was already broken for highmem... which is
actually a problem. I would prefer to see it fixed.
-hpa
Hi Peter,
thank you for jumping in again. I'm not going to pretent I fully understand this
code, as proven by Dave :), but wouldn't it help just to move the high_memory
check directly to the xlate_dev_mem_ptr. Meaning no high_memory check in
valid_phys_addr_range for x86. I sent more info in my reply to Dave's mail.
Again many thanks.
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
On Wed, Oct 02, 2013 at 11:36:09AM -0700, H. Peter Anvin wrote:
> On 10/02/2013 11:31 AM, Frantisek Hrbata wrote:
> > On Wed, Oct 02, 2013 at 10:46:35AM -0700, H. Peter Anvin wrote:
> >> On 10/02/2013 09:05 AM, Frantisek Hrbata wrote:
> >>> +
> >>> +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> >>> +{
> >>> + return addr + count <= __pa(high_memory);
> >>> +}
> >>> +
> >>> +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> >>> +{
> >>> + resource_size_t addr = (pfn << PAGE_SHIFT) + count;
> >>> + return phys_addr_valid(addr);
> >>> +}
> >>>
> >>
> >> The latter has overflow problems.
> >
> > Could you please specify what overflow problems do you mean?
>
> Consider if pfn + count overflows and wraps around, or if (pfn <<
> PAGE_SHIFT) pushes bits out to the left.
Ok, maybe I'm missing something, but isn't this handled in
do_mmap_pgoff?
/* offset overflow? */
if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
return -EOVERFLOW;
Anyway I can take a closer look and if this can really happen I can fix it.
>
> >> The former I realize matches the current /dev/mem, but it is still just
> >> plain wrong in multiple ways.
> >
> > I guess that you are talking about /dev/mem implementation generelly,
> > because
> > this patch is exactly the same as the first one. All I'm trying to do here
> > is to
> > fix this simple problem, which was reported by a customer, using IMHO the
> > least
> > invasive way. Anyway is there any description what is wrong with /dev/mem
> > implementation? Maybe I can try to take a look.
> >
>
> The bottom line is that read/write to /dev/mem should be able to access
> the same memory that we can mmap(). Having two different tests is
> ridiculous.
Ok, I can try to look into this. I just want to point out that some other archs
like arm are doing it the same way. I simply replaced the generic check
functions
in drivers/char/mem.c with x86 specific ones.
Thanks
>
> -hpa
>
>
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
On Wed, Oct 02, 2013 at 10:46:35AM -0700, H. Peter Anvin wrote:
> On 10/02/2013 09:05 AM, Frantisek Hrbata wrote:
> > +
> > +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> > +{
> > + return addr + count <= __pa(high_memory);
> > +}
> > +
> > +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> > +{
> > + resource_size_t addr = (pfn << PAGE_SHIFT) + count;
> > + return phys_addr_valid(addr);
> > +}
> >
>
> The latter has overflow problems.
Could you please specify what overflow problems do you mean?
>
> The former I realize matches the current /dev/mem, but it is still just
> plain wrong in multiple ways.
I guess that you are talking about /dev/mem implementation generelly, because
this patch is exactly the same as the first one. All I'm trying to do here is to
fix this simple problem, which was reported by a customer, using IMHO the least
invasive way. Anyway is there any description what is wrong with /dev/mem
implementation? Maybe I can try to take a look.
Many thanks
>
> -hpa
>
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
07fff2d34da50 R08:
0003 R09: 0008000a
Oct 2 11:24:02 amd-pence-01 kernel: R10: 0001 R11:
0202 R12: 00400590
Oct 2 11:24:02 amd-pence-01 kernel: R13: 7fff2d34db30 R14:
R15:
Oct 2 11:24:02 amd-pence-01 kernel: FS: 7f911ffce700()
GS:880136cc() knlGS:
Oct 2 11:24:02 amd-pence-01 kernel: CS: 0010 DS: ES: CR0:
8005003b
Oct 2 11:24:02 amd-pence-01 kernel: CR2: 7f911ffc8000 CR3:
c39ec000 CR4: 000407e0
Oct 2 11:24:02 amd-pence-01 kernel:
Oct 2 11:24:02 amd-pence-01 kernel: RIP [<00400776>] 0x400775
Oct 2 11:24:02 amd-pence-01 kernel: RSP <7fff2d34da10>
Oct 2 11:24:02 amd-pence-01 kernel: ---[ end trace 8a123cd70049eebc ]---
---------8<--
Please note the PTE value 8008000a0225.
Signed-off-by: Frantisek Hrbata
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 13 +
2 files changed, 17 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 34f69cb..af9f363 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -242,6 +242,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..ef02a93 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include
#include
+#include "physaddr.h"
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,14 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count <= __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ resource_size_t addr = (pfn << PAGE_SHIFT) + count;
+ return phys_addr_valid(addr);
+}
--
1.8.3.1
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
:02 amd-pence-01 kernel: R10: 0001 R11:
0202 R12: 00400590
Oct 2 11:24:02 amd-pence-01 kernel: R13: 7fff2d34db30 R14:
R15:
Oct 2 11:24:02 amd-pence-01 kernel: FS: 7f911ffce700()
GS:880136cc() knlGS:
Oct 2 11:24:02 amd-pence-01 kernel: CS: 0010 DS: ES: CR0:
8005003b
Oct 2 11:24:02 amd-pence-01 kernel: CR2: 7f911ffc8000 CR3:
c39ec000 CR4: 000407e0
Oct 2 11:24:02 amd-pence-01 kernel:
Oct 2 11:24:02 amd-pence-01 kernel: RIP [00400776] 0x400775
Oct 2 11:24:02 amd-pence-01 kernel: RSP 7fff2d34da10
Oct 2 11:24:02 amd-pence-01 kernel: ---[ end trace 8a123cd70049eebc ]---
-8--
Please note the PTE value 8008000a0225.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
arch/x86/include/asm/io.h | 4
arch/x86/mm/mmap.c| 13 +
2 files changed, 17 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 34f69cb..af9f363 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -242,6 +242,10 @@ static inline void flush_write_buffers(void)
#endif
}
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t count);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t count);
+
#endif /* __KERNEL__ */
extern void native_io_delay(void);
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index 25e7e13..ef02a93 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -31,6 +31,8 @@
#include linux/sched.h
#include asm/elf.h
+#include physaddr.h
+
struct __read_mostly va_alignment va_align = {
.flags = -1,
};
@@ -122,3 +124,14 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
mm-get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ resource_size_t addr = (pfn PAGE_SHIFT) + count;
+ return phys_addr_valid(addr);
+}
--
1.8.3.1
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
On Wed, Oct 02, 2013 at 10:46:35AM -0700, H. Peter Anvin wrote:
On 10/02/2013 09:05 AM, Frantisek Hrbata wrote:
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ resource_size_t addr = (pfn PAGE_SHIFT) + count;
+ return phys_addr_valid(addr);
+}
The latter has overflow problems.
Could you please specify what overflow problems do you mean?
The former I realize matches the current /dev/mem, but it is still just
plain wrong in multiple ways.
I guess that you are talking about /dev/mem implementation generelly, because
this patch is exactly the same as the first one. All I'm trying to do here is to
fix this simple problem, which was reported by a customer, using IMHO the least
invasive way. Anyway is there any description what is wrong with /dev/mem
implementation? Maybe I can try to take a look.
Many thanks
-hpa
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] x86: add phys addr validity check for /dev/mem mmap
On Wed, Oct 02, 2013 at 11:36:09AM -0700, H. Peter Anvin wrote:
On 10/02/2013 11:31 AM, Frantisek Hrbata wrote:
On Wed, Oct 02, 2013 at 10:46:35AM -0700, H. Peter Anvin wrote:
On 10/02/2013 09:05 AM, Frantisek Hrbata wrote:
+
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+ return addr + count = __pa(high_memory);
+}
+
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
+{
+ resource_size_t addr = (pfn PAGE_SHIFT) + count;
+ return phys_addr_valid(addr);
+}
The latter has overflow problems.
Could you please specify what overflow problems do you mean?
Consider if pfn + count overflows and wraps around, or if (pfn
PAGE_SHIFT) pushes bits out to the left.
Ok, maybe I'm missing something, but isn't this handled in
do_mmap_pgoff?
/* offset overflow? */
if ((pgoff + (len PAGE_SHIFT)) pgoff)
return -EOVERFLOW;
Anyway I can take a closer look and if this can really happen I can fix it.
The former I realize matches the current /dev/mem, but it is still just
plain wrong in multiple ways.
I guess that you are talking about /dev/mem implementation generelly,
because
this patch is exactly the same as the first one. All I'm trying to do here
is to
fix this simple problem, which was reported by a customer, using IMHO the
least
invasive way. Anyway is there any description what is wrong with /dev/mem
implementation? Maybe I can try to take a look.
The bottom line is that read/write to /dev/mem should be able to access
the same memory that we can mmap(). Having two different tests is
ridiculous.
Ok, I can try to look into this. I just want to point out that some other archs
like arm are doing it the same way. I simply replaced the generic check
functions
in drivers/char/mem.c with x86 specific ones.
Thanks
-hpa
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
On Thu, Sep 19, 2013 at 11:04:16AM +0200, Peter Oberparleiter wrote: > On 04.09.2013 16:42, Frantisek Hrbata wrote: > > The gcov in-memory format changed in gcc 4.7. The biggest change, which > > requires this special implementation, is that gcov_info no longer contains > > array of counters for each counter type for all functions and gcov_fn_info > > is > > not used for mapping of function's counters to these arrays(offset). Now > > each > > gcov_fn_info contans it's counters, which makes things a little bit easier. > > > > This is heavily based on the previous gcc_3_4.c implementation and patches > > provided by Peter Oberparleiter. Specially the buffer gcda implementation > > for > > iterator. > > > > v2: - removed const definition for gcov_fn_info in gcov_info > > - use vmalloc for counter values in gcov_info_dup > > - use iter buffer for gcda > > > > Signed-off-by: Frantisek Hrbata > > The patch is missing an include statement: > > CC kernel/gcov/gcc_4_7.o > kernel/gcov/gcc_4_7.c: In function ‘gcov_info_dup’: > kernel/gcov/gcc_4_7.c:293:4: error: implicit declaration of function > ‘vmalloc’ [-Werror=implicit-function-declaration] > kernel/gcov/gcc_4_7.c:293:20: warning: assignment makes pointer from integer > without a cast [enabled by default] > > With that added, it compiles and works with gcc 4.3.4 and 4.7.2 on > kernel 3.21-rc1 on s390x. > > --- a/kernel/gcov/gcc_4_7.c > +++ b/kernel/gcov/gcc_4_7.c > @@ -15,6 +15,7 @@ > #include > #include > #include > +#include > #include "gcov.h" > > #define GCOV_COUNTERS8 > Hi Peter, I'm sorry I missed that. Many thanks. In addition, do you have any other objedtions to the code? Andrew already added the patches to the -mm tree. Andrew, should I post a follow up patch adding the vmalloc.h include or is it possible to fix it directly as you did for the previous patches? Again I'm sorry for the inconvenience. Many thanks -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
On Wed, Sep 18, 2013 at 02:31:27PM -0700, Andrew Morton wrote: > On Wed, 18 Sep 2013 14:27:05 -0700 Joe Perches wrote: > > > On Wed, 2013-09-18 at 14:22 -0700, Andrew Morton wrote: > > > On Wed, 4 Sep 2013 16:42:54 +0200 Frantisek Hrbata > > > wrote: > > > > The gcov in-memory format changed in gcc 4.7. The biggest change, which > > > > requires this special implementation, is that gcov_info no longer > > > > contains > > > > array of counters for each counter type for all functions and > > > > gcov_fn_info is > > > > not used for mapping of function's counters to these arrays(offset). > > > > Now each > > > > gcov_fn_info contans it's counters, which makes things a little bit > > > > easier. > > > > > > > > This is heavily based on the previous gcc_3_4.c implementation and > > > > patches > > > > provided by Peter Oberparleiter. Specially the buffer gcda > > > > implementation for > > > > iterator. > > > > > > A couple of little tweaks: > > [] > > > +++ a/kernel/gcov/gcc_4_7.c > > [] > > > @@ -267,8 +266,8 @@ struct gcov_info *gcov_info_dup(struct g > > > if (!dup->filename) > > > goto err_free; > > > > > > - dup->functions = kzalloc(sizeof(struct gcov_fn_info *) * > > > - info->n_functions, GFP_KERNEL); > > > + dup->functions = kcalloc(sizeof(struct gcov_fn_info *), > > > + info->n_functions, GFP_KERNEL); > > > > kcalloc(n, size_t, flags) > > > > dup->functions = kcalloc(info->n_functions, > > sizeof(struct gcov_fn_info *), GFP_KERNEL); > > > > Already did that after checkpatch whined at me. Stupid thing. Many thanks Andrew and I'm really sorry about the "style problems". Lesson well learned: use checkpatch before posting. -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
On Wed, Sep 18, 2013 at 02:31:27PM -0700, Andrew Morton wrote: On Wed, 18 Sep 2013 14:27:05 -0700 Joe Perches j...@perches.com wrote: On Wed, 2013-09-18 at 14:22 -0700, Andrew Morton wrote: On Wed, 4 Sep 2013 16:42:54 +0200 Frantisek Hrbata fhrb...@redhat.com wrote: The gcov in-memory format changed in gcc 4.7. The biggest change, which requires this special implementation, is that gcov_info no longer contains array of counters for each counter type for all functions and gcov_fn_info is not used for mapping of function's counters to these arrays(offset). Now each gcov_fn_info contans it's counters, which makes things a little bit easier. This is heavily based on the previous gcc_3_4.c implementation and patches provided by Peter Oberparleiter. Specially the buffer gcda implementation for iterator. A couple of little tweaks: [] +++ a/kernel/gcov/gcc_4_7.c [] @@ -267,8 +266,8 @@ struct gcov_info *gcov_info_dup(struct g if (!dup-filename) goto err_free; - dup-functions = kzalloc(sizeof(struct gcov_fn_info *) * - info-n_functions, GFP_KERNEL); + dup-functions = kcalloc(sizeof(struct gcov_fn_info *), + info-n_functions, GFP_KERNEL); kcalloc(n, size_t, flags) dup-functions = kcalloc(info-n_functions, sizeof(struct gcov_fn_info *), GFP_KERNEL); Already did that after checkpatch whined at me. Stupid thing. Many thanks Andrew and I'm really sorry about the style problems. Lesson well learned: use checkpatch before posting. -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
On Thu, Sep 19, 2013 at 11:04:16AM +0200, Peter Oberparleiter wrote: On 04.09.2013 16:42, Frantisek Hrbata wrote: The gcov in-memory format changed in gcc 4.7. The biggest change, which requires this special implementation, is that gcov_info no longer contains array of counters for each counter type for all functions and gcov_fn_info is not used for mapping of function's counters to these arrays(offset). Now each gcov_fn_info contans it's counters, which makes things a little bit easier. This is heavily based on the previous gcc_3_4.c implementation and patches provided by Peter Oberparleiter. Specially the buffer gcda implementation for iterator. v2: - removed const definition for gcov_fn_info in gcov_info - use vmalloc for counter values in gcov_info_dup - use iter buffer for gcda Signed-off-by: Frantisek Hrbata fhrb...@redhat.com The patch is missing an include statement: CC kernel/gcov/gcc_4_7.o kernel/gcov/gcc_4_7.c: In function ‘gcov_info_dup’: kernel/gcov/gcc_4_7.c:293:4: error: implicit declaration of function ‘vmalloc’ [-Werror=implicit-function-declaration] kernel/gcov/gcc_4_7.c:293:20: warning: assignment makes pointer from integer without a cast [enabled by default] With that added, it compiles and works with gcc 4.3.4 and 4.7.2 on kernel 3.21-rc1 on s390x. --- a/kernel/gcov/gcc_4_7.c +++ b/kernel/gcov/gcc_4_7.c @@ -15,6 +15,7 @@ #include linux/slab.h #include linux/string.h #include linux/seq_file.h +#include linux/vmalloc.h #include gcov.h #define GCOV_COUNTERS8 Hi Peter, I'm sorry I missed that. Many thanks. In addition, do you have any other objedtions to the code? Andrew already added the patches to the -mm tree. Andrew, should I post a follow up patch adding the vmalloc.h include or is it possible to fix it directly as you did for the previous patches? Again I'm sorry for the inconvenience. Many thanks -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Tue, Sep 10, 2013 at 03:05:57PM +0930, Rusty Russell wrote:
> Frantisek Hrbata writes:
> > On Mon, Sep 09, 2013 at 10:44:03AM +0930, Rusty Russell wrote:
> >> Kyle McMartin writes:
> >> > On Fri, Sep 06, 2013 at 07:51:18PM +0200, Frantisek Hrbata wrote:
> >> >> > > v2: - reuse mod->ctors for .init_array section for modules, because
> >> >> > > gcc uses
> >> >> > > .ctors or .init_array, but not both at the same time
> >> >> > >
> >> >> > > Signed-off-by: Frantisek Hrbata
> >> >> >
> >> >> > Might be nice to document which gcc version changed this, so people
> >> >> > can
> >> >> > choose whether to cherry-pick this change?
> >> >>
> >> >> Thank you for pointing this out. As per gcc git this was introduced by
> >> >> commit
> >> >> ef1da80 and released in 4.7 version.
> >> >>
> >> >> $ git describe --contains ef1da80
> >> >> gcc-4_7_0-release~4358
> >> >>
> >> >> Do you want me to post v3 with this info included in the descrition?
> >> >>
> >> >
> >> > It actually depends on the combination of binutils/ld and gcc you use,
> >> > not
> >> > simply which gcc version you use. :/
> >>
> >> Indeed, and seems it was binutils 20110507 which actually handled it
> >> properly.
> >>
> >> AFAICT it's theoretically possible to have .ctors and .init_array in a
> >> module. Unlikely, but the patch should check for both and refuse to
> >> load the module in that case. Otherwise weird things would happen.
> >
> > I'm not sure if coexistence of .ctors and .init_array sections should
> > result in
> > denial of module, but I for sure know nothing about this :). Could you maybe
> > privide one example of the "weird thing"?
>
> Well, if we have both ctors and init_array, and we only call the ctors,
> part of the module will be uninitialized.
>
> I was thinking about something like the following (based on your
> previous patch).
>
> Thoughts?
> Rusty.
Thank you Rusty, from what I can say it looks ok to me. So I would go with this
version. Is there anything that needs to be done to consider this as the
correct version of the 4/4 patch? Meaning should we repost this as v3 or could
your version of the patch be picked as you posted it?
>
> From: Frantisek Hrbata
> Subject: kernel: add support for init_array constructors
>
> This adds the .init_array section as yet another section with constructors.
> This
> is needed because gcc could add __gcov_init calls to .init_array or .ctors
> section, depending on gcc (and binutils) version .
>
> v2: - reuse mod->ctors for .init_array section for modules, because gcc uses
> .ctors or .init_array, but not both at the same time
> v3: - fail to load if that does happen somehow.
>
> Signed-off-by: Frantisek Hrbata
> Signed-off-by: Rusty Russell
>
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 83e2c31..bc2121f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -473,6 +473,7 @@
> #define KERNEL_CTORS() . = ALIGN(8); \
> VMLINUX_SYMBOL(__ctors_start) = .; \
> *(.ctors) \
> + *(.init_array) \
> VMLINUX_SYMBOL(__ctors_end) = .;
> #else
> #define KERNEL_CTORS()
> diff --git a/kernel/module.c b/kernel/module.c
> index dc58274..d3f5a58 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2738,7 +2738,7 @@ static int check_modinfo(struct module *mod, struct
> load_info *info, int flags)
> return 0;
> }
>
> -static void find_module_sections(struct module *mod, struct load_info *info)
> +static int find_module_sections(struct module *mod, struct load_info *info)
> {
> mod->kp = section_objs(info, "__param",
> sizeof(*mod->kp), >num_kp);
> @@ -2768,6 +2768,18 @@ static void find_module_sections(struct module *mod,
> struct load_info *info)
> #ifdef CONFIG_CONSTRUCTORS
> mod->ctors = section_objs(info, ".ctors",
> sizeof(*mod->ctors), >num_ctors);
> + if (!mod->ctors)
> + mod->ctors = section_objs(info, ".init_array",
> + sizeof(*mod-&g
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Tue, Sep 10, 2013 at 03:05:57PM +0930, Rusty Russell wrote:
Frantisek Hrbata fhrb...@redhat.com writes:
On Mon, Sep 09, 2013 at 10:44:03AM +0930, Rusty Russell wrote:
Kyle McMartin k...@infradead.org writes:
On Fri, Sep 06, 2013 at 07:51:18PM +0200, Frantisek Hrbata wrote:
v2: - reuse mod-ctors for .init_array section for modules, because
gcc uses
.ctors or .init_array, but not both at the same time
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
Might be nice to document which gcc version changed this, so people
can
choose whether to cherry-pick this change?
Thank you for pointing this out. As per gcc git this was introduced by
commit
ef1da80 and released in 4.7 version.
$ git describe --contains ef1da80
gcc-4_7_0-release~4358
Do you want me to post v3 with this info included in the descrition?
It actually depends on the combination of binutils/ld and gcc you use,
not
simply which gcc version you use. :/
Indeed, and seems it was binutils 20110507 which actually handled it
properly.
AFAICT it's theoretically possible to have .ctors and .init_array in a
module. Unlikely, but the patch should check for both and refuse to
load the module in that case. Otherwise weird things would happen.
I'm not sure if coexistence of .ctors and .init_array sections should
result in
denial of module, but I for sure know nothing about this :). Could you maybe
privide one example of the weird thing?
Well, if we have both ctors and init_array, and we only call the ctors,
part of the module will be uninitialized.
I was thinking about something like the following (based on your
previous patch).
Thoughts?
Rusty.
Thank you Rusty, from what I can say it looks ok to me. So I would go with this
version. Is there anything that needs to be done to consider this as the
correct version of the 4/4 patch? Meaning should we repost this as v3 or could
your version of the patch be picked as you posted it?
From: Frantisek Hrbata fhrb...@redhat.com
Subject: kernel: add support for init_array constructors
This adds the .init_array section as yet another section with constructors.
This
is needed because gcc could add __gcov_init calls to .init_array or .ctors
section, depending on gcc (and binutils) version .
v2: - reuse mod-ctors for .init_array section for modules, because gcc uses
.ctors or .init_array, but not both at the same time
v3: - fail to load if that does happen somehow.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 83e2c31..bc2121f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -473,6 +473,7 @@
#define KERNEL_CTORS() . = ALIGN(8); \
VMLINUX_SYMBOL(__ctors_start) = .; \
*(.ctors) \
+ *(.init_array) \
VMLINUX_SYMBOL(__ctors_end) = .;
#else
#define KERNEL_CTORS()
diff --git a/kernel/module.c b/kernel/module.c
index dc58274..d3f5a58 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2738,7 +2738,7 @@ static int check_modinfo(struct module *mod, struct
load_info *info, int flags)
return 0;
}
-static void find_module_sections(struct module *mod, struct load_info *info)
+static int find_module_sections(struct module *mod, struct load_info *info)
{
mod-kp = section_objs(info, __param,
sizeof(*mod-kp), mod-num_kp);
@@ -2768,6 +2768,18 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
#ifdef CONFIG_CONSTRUCTORS
mod-ctors = section_objs(info, .ctors,
sizeof(*mod-ctors), mod-num_ctors);
+ if (!mod-ctors)
+ mod-ctors = section_objs(info, .init_array,
+ sizeof(*mod-ctors), mod-num_ctors);
+ else if (find_sec(info, .init_array)) {
+ /*
+ * This shouldn't happen with same compiler and binutils
+ * building all parts of the module.
+ */
+ printk(KERN_WARNING %s: has both .ctors and .init_array.\n,
+mod-name);
+ return -EINVAL;
+ }
#endif
#ifdef CONFIG_TRACEPOINTS
@@ -2806,6 +2818,8 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
info-debug = section_objs(info, __verbose,
sizeof(*info-debug), info-num_debug);
+
+ return 0;
}
static int move_module(struct module *mod, struct load_info *info)
@@ -3263,7 +3277,9 @@ static int load_module(struct load_info *info, const
char __user *uargs,
/* Now we've got everything
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Mon, Sep 09, 2013 at 10:44:03AM +0930, Rusty Russell wrote:
> Kyle McMartin writes:
> > On Fri, Sep 06, 2013 at 07:51:18PM +0200, Frantisek Hrbata wrote:
> >> > > v2: - reuse mod->ctors for .init_array section for modules, because
> >> > > gcc uses
> >> > > .ctors or .init_array, but not both at the same time
> >> > >
> >> > > Signed-off-by: Frantisek Hrbata
> >> >
> >> > Might be nice to document which gcc version changed this, so people can
> >> > choose whether to cherry-pick this change?
> >>
> >> Thank you for pointing this out. As per gcc git this was introduced by
> >> commit
> >> ef1da80 and released in 4.7 version.
> >>
> >> $ git describe --contains ef1da80
> >> gcc-4_7_0-release~4358
> >>
> >> Do you want me to post v3 with this info included in the descrition?
> >>
> >
> > It actually depends on the combination of binutils/ld and gcc you use, not
> > simply which gcc version you use. :/
>
> Indeed, and seems it was binutils 20110507 which actually handled it
> properly.
>
> AFAICT it's theoretically possible to have .ctors and .init_array in a
> module. Unlikely, but the patch should check for both and refuse to
> load the module in that case. Otherwise weird things would happen.
I'm not sure if coexistence of .ctors and .init_array sections should result in
denial of module, but I for sure know nothing about this :). Could you maybe
privide one example of the "weird thing"?
Anyway many thanks for taking time to look at this. Below is my attepmt to
implement the check you proposed.
untested/uncompiled
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 83e2c31..bc2121f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -473,6 +473,7 @@
#define KERNEL_CTORS() . = ALIGN(8); \
VMLINUX_SYMBOL(__ctors_start) = .; \
*(.ctors) \
+ *(.init_array) \
VMLINUX_SYMBOL(__ctors_end) = .;
#else
#define KERNEL_CTORS()
diff --git a/include/linux/module.h b/include/linux/module.h
index 05f2447..e775b41 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -380,6 +380,8 @@ struct module
/* Constructor functions. */
ctor_fn_t *ctors;
unsigned int num_ctors;
+ ctor_fn_t *init_array;
+ unsigned int num_init_array;
#endif
};
#ifndef MODULE_ARCH_INIT
diff --git a/kernel/module.c b/kernel/module.c
index dc58274..831b92d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2768,6 +2768,9 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
#ifdef CONFIG_CONSTRUCTORS
mod->ctors = section_objs(info, ".ctors",
sizeof(*mod->ctors), >num_ctors);
+ mod->init_array = section_objs(info, ".init_array",
+ sizeof(*mod->init_array),
+ >num_init_array);
#endif
#ifdef CONFIG_TRACEPOINTS
@@ -2808,6 +2811,18 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
sizeof(*info->debug), >num_debug);
}
+static void check_module_sections(struct module *mod)
+{
+#ifdef CONFIG_CONSTRUCTORS
+ if (mod->ctors && mod->init_array) {
+ pr_err("%s: .ctors and .init_array sections mishmash\n",
+ mod->name);
+ return -EINVAL;
+ }
+#endif
+ return 0;
+}
+
static int move_module(struct module *mod, struct load_info *info)
{
int i;
@@ -3032,6 +3047,9 @@ static void do_mod_ctors(struct module *mod)
for (i = 0; i < mod->num_ctors; i++)
mod->ctors[i]();
+
+ for (i = 0; i < mod->num_init_array; i++)
+ mod->init_array[i]();
#endif
}
@@ -3265,6 +3283,10 @@ static int load_module(struct load_info *info, const
char __user *uargs,
* find optional sections. */
find_module_sections(mod, info);
+ err = check_module_sections(mod);
+ if (err)
+ goto free_unload;
+
err = check_module_license_and_versions(mod);
if (err)
goto free_unload;
--
1.8.3.1
>
> Cheers,
> Rusty.
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Mon, Sep 09, 2013 at 10:44:03AM +0930, Rusty Russell wrote:
Kyle McMartin k...@infradead.org writes:
On Fri, Sep 06, 2013 at 07:51:18PM +0200, Frantisek Hrbata wrote:
v2: - reuse mod-ctors for .init_array section for modules, because
gcc uses
.ctors or .init_array, but not both at the same time
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
Might be nice to document which gcc version changed this, so people can
choose whether to cherry-pick this change?
Thank you for pointing this out. As per gcc git this was introduced by
commit
ef1da80 and released in 4.7 version.
$ git describe --contains ef1da80
gcc-4_7_0-release~4358
Do you want me to post v3 with this info included in the descrition?
It actually depends on the combination of binutils/ld and gcc you use, not
simply which gcc version you use. :/
Indeed, and seems it was binutils 20110507 which actually handled it
properly.
AFAICT it's theoretically possible to have .ctors and .init_array in a
module. Unlikely, but the patch should check for both and refuse to
load the module in that case. Otherwise weird things would happen.
I'm not sure if coexistence of .ctors and .init_array sections should result in
denial of module, but I for sure know nothing about this :). Could you maybe
privide one example of the weird thing?
Anyway many thanks for taking time to look at this. Below is my attepmt to
implement the check you proposed.
untested/uncompiled
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 83e2c31..bc2121f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -473,6 +473,7 @@
#define KERNEL_CTORS() . = ALIGN(8); \
VMLINUX_SYMBOL(__ctors_start) = .; \
*(.ctors) \
+ *(.init_array) \
VMLINUX_SYMBOL(__ctors_end) = .;
#else
#define KERNEL_CTORS()
diff --git a/include/linux/module.h b/include/linux/module.h
index 05f2447..e775b41 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -380,6 +380,8 @@ struct module
/* Constructor functions. */
ctor_fn_t *ctors;
unsigned int num_ctors;
+ ctor_fn_t *init_array;
+ unsigned int num_init_array;
#endif
};
#ifndef MODULE_ARCH_INIT
diff --git a/kernel/module.c b/kernel/module.c
index dc58274..831b92d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2768,6 +2768,9 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
#ifdef CONFIG_CONSTRUCTORS
mod-ctors = section_objs(info, .ctors,
sizeof(*mod-ctors), mod-num_ctors);
+ mod-init_array = section_objs(info, .init_array,
+ sizeof(*mod-init_array),
+ mod-num_init_array);
#endif
#ifdef CONFIG_TRACEPOINTS
@@ -2808,6 +2811,18 @@ static void find_module_sections(struct module *mod,
struct load_info *info)
sizeof(*info-debug), info-num_debug);
}
+static void check_module_sections(struct module *mod)
+{
+#ifdef CONFIG_CONSTRUCTORS
+ if (mod-ctors mod-init_array) {
+ pr_err(%s: .ctors and .init_array sections mishmash\n,
+ mod-name);
+ return -EINVAL;
+ }
+#endif
+ return 0;
+}
+
static int move_module(struct module *mod, struct load_info *info)
{
int i;
@@ -3032,6 +3047,9 @@ static void do_mod_ctors(struct module *mod)
for (i = 0; i mod-num_ctors; i++)
mod-ctors[i]();
+
+ for (i = 0; i mod-num_init_array; i++)
+ mod-init_array[i]();
#endif
}
@@ -3265,6 +3283,10 @@ static int load_module(struct load_info *info, const
char __user *uargs,
* find optional sections. */
find_module_sections(mod, info);
+ err = check_module_sections(mod);
+ if (err)
+ goto free_unload;
+
err = check_module_license_and_versions(mod);
if (err)
goto free_unload;
--
1.8.3.1
Cheers,
Rusty.
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Fri, Sep 06, 2013 at 11:43:08AM +0930, Rusty Russell wrote: > Frantisek Hrbata writes: > > This adds the .init_array section as yet another section with constructors. > > This > > is needed because gcc could add __gcov_init calls to .init_array or .ctors > > section, depending on gcc version. > > > > v2: - reuse mod->ctors for .init_array section for modules, because gcc uses > > .ctors or .init_array, but not both at the same time > > > > Signed-off-by: Frantisek Hrbata > > Might be nice to document which gcc version changed this, so people can > choose whether to cherry-pick this change? Thank you for pointing this out. As per gcc git this was introduced by commit ef1da80 and released in 4.7 version. $ git describe --contains ef1da80 gcc-4_7_0-release~4358 Do you want me to post v3 with this info included in the descrition? > > Acked-by: Rusty Russell Many thanks > > > --- > > include/asm-generic/vmlinux.lds.h | 1 + > > kernel/module.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/include/asm-generic/vmlinux.lds.h > > b/include/asm-generic/vmlinux.lds.h > > index 69732d2..c55d8d9 100644 > > --- a/include/asm-generic/vmlinux.lds.h > > +++ b/include/asm-generic/vmlinux.lds.h > > @@ -468,6 +468,7 @@ > > #define KERNEL_CTORS() . = ALIGN(8); \ > > VMLINUX_SYMBOL(__ctors_start) = .; \ > > *(.ctors) \ > > + *(.init_array) \ > > VMLINUX_SYMBOL(__ctors_end) = .; > > #else > > #define KERNEL_CTORS() > > diff --git a/kernel/module.c b/kernel/module.c > > index 2069158..bbbd953 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, > > struct load_info *info) > > #ifdef CONFIG_CONSTRUCTORS > > mod->ctors = section_objs(info, ".ctors", > > sizeof(*mod->ctors), >num_ctors); > > + if (!mod->ctors) > > + mod->ctors = section_objs(info, ".init_array", > > + sizeof(*mod->ctors), >num_ctors); > > #endif > > > > #ifdef CONFIG_TRACEPOINTS > > -- > > 1.8.3.1 -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/4] kernel: add support for init_array constructors
On Fri, Sep 06, 2013 at 11:43:08AM +0930, Rusty Russell wrote: Frantisek Hrbata fhrb...@redhat.com writes: This adds the .init_array section as yet another section with constructors. This is needed because gcc could add __gcov_init calls to .init_array or .ctors section, depending on gcc version. v2: - reuse mod-ctors for .init_array section for modules, because gcc uses .ctors or .init_array, but not both at the same time Signed-off-by: Frantisek Hrbata fhrb...@redhat.com Might be nice to document which gcc version changed this, so people can choose whether to cherry-pick this change? Thank you for pointing this out. As per gcc git this was introduced by commit ef1da80 and released in 4.7 version. $ git describe --contains ef1da80 gcc-4_7_0-release~4358 Do you want me to post v3 with this info included in the descrition? Acked-by: Rusty Russell ru...@rustcorp.com.au Many thanks --- include/asm-generic/vmlinux.lds.h | 1 + kernel/module.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 69732d2..c55d8d9 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -468,6 +468,7 @@ #define KERNEL_CTORS() . = ALIGN(8); \ VMLINUX_SYMBOL(__ctors_start) = .; \ *(.ctors) \ + *(.init_array) \ VMLINUX_SYMBOL(__ctors_end) = .; #else #define KERNEL_CTORS() diff --git a/kernel/module.c b/kernel/module.c index 2069158..bbbd953 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, struct load_info *info) #ifdef CONFIG_CONSTRUCTORS mod-ctors = section_objs(info, .ctors, sizeof(*mod-ctors), mod-num_ctors); + if (!mod-ctors) + mod-ctors = section_objs(info, .init_array, + sizeof(*mod-ctors), mod-num_ctors); #endif #ifdef CONFIG_TRACEPOINTS -- 1.8.3.1 -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/4] add support for gcov format introduced in gcc 4.7
This is an attempt to bring support for modified gcov format in gcc 4.7 to the kernel. It tries to leverage the existing layout/abstraction, which was designed keeping in mind that the gcov format could change, but some changes had to be make. Mostly because the current model does not take into account that even the core gcov structures, like gcov_info, could change. One part that could be problematic is the addition of the .init_array section for constructors. Tested with lcov and seems to be working fine, giving similar results as for the older format. v2: Included suggestions/code provided by Peter Oberparleiter. Detailed description in patches. Frantisek Hrbata (4): gcov: move gcov structs definitions to a gcc version specific file gcov: add support for gcc 4.7 gcov format gcov: compile specific gcov implementation based on gcc version kernel: add support for init_array constructors Documentation/gcov.txt| 4 + include/asm-generic/vmlinux.lds.h | 1 + kernel/gcov/Kconfig | 30 ++ kernel/gcov/Makefile | 32 ++- kernel/gcov/base.c| 32 ++- kernel/gcov/fs.c | 27 +- kernel/gcov/gcc_3_4.c | 115 kernel/gcov/gcc_4_7.c | 562 ++ kernel/gcov/gcov.h| 65 + kernel/module.c | 3 + 10 files changed, 790 insertions(+), 81 deletions(-) create mode 100644 kernel/gcov/gcc_4_7.c -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/4] gcov: move gcov structs definitions to a gcc version specific file
Since also the gcov structures(gcov_info, gcov_fn_info, gcov_ctr_info) can
change between gcc releases, as shown in gcc 4.7, they cannot be defined in a
common header and need to be moved to a specific gcc implemention file. This
also requires to make the gcov_info structure opaque for the common code and to
introduce simple helpers for accessing data inside gcov_info.
Signed-off-by: Frantisek Hrbata
---
kernel/gcov/base.c| 26 ++--
kernel/gcov/fs.c | 27 ++--
kernel/gcov/gcc_3_4.c | 115 ++
kernel/gcov/gcov.h| 65 +---
4 files changed, 153 insertions(+), 80 deletions(-)
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9b22d03..912576a 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -20,7 +20,6 @@
#include
#include "gcov.h"
-static struct gcov_info *gcov_info_head;
static int gcov_events_enabled;
static DEFINE_MUTEX(gcov_lock);
@@ -34,7 +33,7 @@ void __gcov_init(struct gcov_info *info)
mutex_lock(_lock);
if (gcov_version == 0) {
- gcov_version = info->version;
+ gcov_version = gcov_info_version(info);
/*
* Printing gcc's version magic may prove useful for debugging
* incompatibility reports.
@@ -45,8 +44,7 @@ void __gcov_init(struct gcov_info *info)
* Add new profiling data structure to list and inform event
* listener.
*/
- info->next = gcov_info_head;
- gcov_info_head = info;
+ gcov_info_link(info);
if (gcov_events_enabled)
gcov_event(GCOV_ADD, info);
mutex_unlock(_lock);
@@ -91,13 +89,15 @@ EXPORT_SYMBOL(__gcov_merge_delta);
*/
void gcov_enable_events(void)
{
- struct gcov_info *info;
+ struct gcov_info *info = NULL;
mutex_lock(_lock);
gcov_events_enabled = 1;
+
/* Perform event callback for previously registered entries. */
- for (info = gcov_info_head; info; info = info->next)
+ while ((info = gcov_info_next(info)))
gcov_event(GCOV_ADD, info);
+
mutex_unlock(_lock);
}
@@ -112,25 +112,23 @@ static int gcov_module_notifier(struct notifier_block
*nb, unsigned long event,
void *data)
{
struct module *mod = data;
- struct gcov_info *info;
- struct gcov_info *prev;
+ struct gcov_info *info = NULL;
+ struct gcov_info *prev = NULL;
if (event != MODULE_STATE_GOING)
return NOTIFY_OK;
mutex_lock(_lock);
- prev = NULL;
+
/* Remove entries located in module from linked list. */
- for (info = gcov_info_head; info; info = info->next) {
+ while ((info = gcov_info_next(info))) {
if (within(info, mod->module_core, mod->core_size)) {
- if (prev)
- prev->next = info->next;
- else
- gcov_info_head = info->next;
+ gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
} else
prev = info;
}
+
mutex_unlock(_lock);
return NOTIFY_OK;
diff --git a/kernel/gcov/fs.c b/kernel/gcov/fs.c
index 9bd0934..27e12ce 100644
--- a/kernel/gcov/fs.c
+++ b/kernel/gcov/fs.c
@@ -242,7 +242,7 @@ static struct gcov_node *get_node_by_name(const char *name)
list_for_each_entry(node, _head, all) {
info = get_node_info(node);
- if (info && (strcmp(info->filename, name) == 0))
+ if (info && (strcmp(gcov_info_filename(info), name) == 0))
return node;
}
@@ -279,7 +279,7 @@ static ssize_t gcov_seq_write(struct file *file, const char
__user *addr,
seq = file->private_data;
info = gcov_iter_get_info(seq->private);
mutex_lock(_lock);
- node = get_node_by_name(info->filename);
+ node = get_node_by_name(gcov_info_filename(info));
if (node) {
/* Reset counts or remove node for unloaded modules. */
if (node->num_loaded == 0)
@@ -376,8 +376,9 @@ static void add_links(struct gcov_node *node, struct dentry
*parent)
if (!node->links)
return;
for (i = 0; i < num; i++) {
- target = get_link_target(get_node_info(node)->filename,
-_link[i]);
+ target = get_link_target(
+ gcov_info_filename(get_node_info(node)),
+ _link[i]);
if (!target)
goto out_err;
basename = strrchr(target, '/');
@@ -576,7 +577,7 @@ static void add_
[PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
The gcov in-memory format changed in gcc 4.7. The biggest change, which
requires this special implementation, is that gcov_info no longer contains
array of counters for each counter type for all functions and gcov_fn_info is
not used for mapping of function's counters to these arrays(offset). Now each
gcov_fn_info contans it's counters, which makes things a little bit easier.
This is heavily based on the previous gcc_3_4.c implementation and patches
provided by Peter Oberparleiter. Specially the buffer gcda implementation for
iterator.
v2: - removed const definition for gcov_fn_info in gcov_info
- use vmalloc for counter values in gcov_info_dup
- use iter buffer for gcda
Signed-off-by: Frantisek Hrbata
---
kernel/gcov/base.c| 6 +
kernel/gcov/gcc_4_7.c | 562 ++
2 files changed, 568 insertions(+)
create mode 100644 kernel/gcov/gcc_4_7.c
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 912576a..f45b75b 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -79,6 +79,12 @@ void __gcov_merge_delta(gcov_type *counters, unsigned int
n_counters)
}
EXPORT_SYMBOL(__gcov_merge_delta);
+void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_ior);
+
/**
* gcov_enable_events - enable event reporting through gcov_event()
*
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
new file mode 100644
index 000..d91afee
--- /dev/null
+++ b/kernel/gcov/gcc_4_7.c
@@ -0,0 +1,562 @@
+/*
+ * This code provides functions to handle gcc's profiling data format
+ * introduced with gcc 4.7.
+ *
+ * This file is based heavily on gcc_3_4.c file.
+ *
+ * For a better understanding, refer to gcc source:
+ * gcc/gcov-io.h
+ * libgcc/libgcov.c
+ *
+ * Uses gcc-internal data definitions.
+ */
+
+#include
+#include
+#include
+#include
+#include "gcov.h"
+
+#define GCOV_COUNTERS 8
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+static struct gcov_info *gcov_info_head;
+
+/**
+ * struct gcov_ctr_info - information about counters for a single function
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info {
+ unsigned int num;
+ gcov_type *values;
+};
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @key: comdat key
+ * @ident: unique ident of function
+ * @lineno_checksum: function lineo_checksum
+ * @cfg_checksum: function cfg checksum
+ * @ctrs: instrumented counters
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ *
+ * Information about a single function. This uses the trailing array
+ * idiom. The number of counters is determined from the merge pointer
+ * array in gcov_info. The key is used to detect which of a set of
+ * comdat functions was selected -- it points to the gcov_info object
+ * of the object file containing the selected comdat function.
+ */
+struct gcov_fn_info {
+ const struct gcov_info *key;
+ unsigned int ident;
+ unsigned int lineno_checksum;
+ unsigned int cfg_checksum;
+ struct gcov_ctr_info ctrs[0];
+};
+
+/**
+ * struct gcov_info - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: uniquifying time stamp
+ * @filename: name of the associated gcov data file
+ * @merge: merge functions (null for unused counter type)
+ * @n_functions: number of instrumented functions
+ * @functions: pointer to pointers to function information
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info {
+ unsigned int version;
+ struct gcov_info *next;
+ unsigned int stamp;
+ const char *filename;
+ void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int);
+ unsigned int n_functions;
+ struct gcov_fn_info **functions;
+};
+
+/**
+ * gcov_info_filename - return info filename
+ * @info: profiling data set
+ */
+const char *gcov_info_filename(struct gcov_info *info)
+{
+ return info->filename;
+}
+
+/**
+ * gcov_info_version - return info version
+ * @info: profiling data set
+ */
+unsigned int gcov_info_version(struct gcov_info *info)
+{
+ return info->version;
+}
+
+/**
+ * gcov_info_next - return next profiling data set
+ * @info: profiling data set
+ *
+ * Returns next gcov_info following @info or first gcov_info in the chain if
+ * @info is %NULL.
+ */
+struct gcov_info *gcov_info_next(struct gcov_info *info)
+{
+ if (!info)
+ return gcov_info_head;
+
+ return info->next;
+}
+
+/**
+ * gcov_info_link - link/add profiling d
[PATCH v2 4/4] kernel: add support for init_array constructors
This adds the .init_array section as yet another section with constructors. This is needed because gcc could add __gcov_init calls to .init_array or .ctors section, depending on gcc version. v2: - reuse mod->ctors for .init_array section for modules, because gcc uses .ctors or .init_array, but not both at the same time Signed-off-by: Frantisek Hrbata --- include/asm-generic/vmlinux.lds.h | 1 + kernel/module.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 69732d2..c55d8d9 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -468,6 +468,7 @@ #define KERNEL_CTORS() . = ALIGN(8); \ VMLINUX_SYMBOL(__ctors_start) = .; \ *(.ctors) \ + *(.init_array) \ VMLINUX_SYMBOL(__ctors_end) = .; #else #define KERNEL_CTORS() diff --git a/kernel/module.c b/kernel/module.c index 2069158..bbbd953 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, struct load_info *info) #ifdef CONFIG_CONSTRUCTORS mod->ctors = section_objs(info, ".ctors", sizeof(*mod->ctors), >num_ctors); + if (!mod->ctors) + mod->ctors = section_objs(info, ".init_array", + sizeof(*mod->ctors), >num_ctors); #endif #ifdef CONFIG_TRACEPOINTS -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/4] gcov: compile specific gcov implementation based on gcc version
Compile the correct gcov implementation file for the specific gcc version. v2: - added possibility to explicitly select gcc version during configuration, this is based on code provided by Peter Oberparleiter. - added a note about gcov format selection to gcov.txt Signed-off-by: Frantisek Hrbata --- Documentation/gcov.txt | 4 kernel/gcov/Kconfig| 30 ++ kernel/gcov/Makefile | 32 +++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/Documentation/gcov.txt b/Documentation/gcov.txt index e7ca647..7b72778 100644 --- a/Documentation/gcov.txt +++ b/Documentation/gcov.txt @@ -50,6 +50,10 @@ Configure the kernel with: CONFIG_DEBUG_FS=y CONFIG_GCOV_KERNEL=y +select the gcc's gcov format, default is autodetect based on gcc version: + +CONFIG_GCOV_FORMAT_AUTODETECT=y + and to get coverage data for the entire kernel: CONFIG_GCOV_PROFILE_ALL=y diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index d4da55d..d04ce8a 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL larger and run slower. Also be sure to exclude files from profiling which are not linked to the kernel image to prevent linker errors. +choice + prompt "Specify GCOV format" + depends on GCOV_KERNEL + default GCOV_FORMAT_AUTODETECT + ---help--- + The gcov format is usually determined by the GCC version, but there are + exceptions where format changes are integrated in lower-version GCCs. + In such a case use this option to adjust the format used in the kernel + accordingly. + + If unsure, choose "Autodetect". + +config GCOV_FORMAT_AUTODETECT + bool "Autodetect" + ---help--- + Select this option to use the format that corresponds to your GCC + version. + +config GCOV_FORMAT_3_4 + bool "GCC 3.4 format" + ---help--- + Select this option to use the format defined by GCC 3.4. + +config GCOV_FORMAT_4_7 + bool "GCC 4.7 format" + ---help--- + Select this option to use the format defined by GCC 4.7. + +endchoice + endmenu diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index e97ca59..52aa7e8 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,3 +1,33 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o +# if-lt +# Usage VAR := $(call if-lt, $(a), $(b)) +# Returns 1 if (a < b) +if-lt = $(shell [ $(1) -lt $(2) ] && echo 1) + +ifeq ($(CONFIG_GCOV_FORMAT_3_4),y) + cc-ver := 0304 +else ifeq ($(CONFIG_GCOV_FORMAT_4_7),y) + cc-ver := 0407 +else +# Use cc-version if available, otherwise set 0 +# +# scripts/Kbuild.include, which contains cc-version function, is not included +# during make clean "make -f scripts/Makefile.clean obj=kernel/gcov" +# Meaning cc-ver is empty causing if-lt test to fail with +# "/bin/sh: line 0: [: -lt: unary operator expected" error mesage. +# This has no affect on the clean phase, but the error message could be +# confusing/annoying. So this dummy workaround sets cc-ver to zero if cc-version +# is not available. We can probably move if-lt to Kbuild.include, so it's also +# not defined during clean or to include Kbuild.include in +# scripts/Makefile.clean. But the following workaround seems least invasive. + cc-ver := $(if $(call cc-version),$(call cc-version),0) +endif + +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o + +ifeq ($(call if-lt, $(cc-ver), 0407),1) + obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o +else + obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o +endif -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/4] gcov: compile specific gcov implementation based on gcc version
Compile the correct gcov implementation file for the specific gcc version. v2: - added possibility to explicitly select gcc version during configuration, this is based on code provided by Peter Oberparleiter. - added a note about gcov format selection to gcov.txt Signed-off-by: Frantisek Hrbata fhrb...@redhat.com --- Documentation/gcov.txt | 4 kernel/gcov/Kconfig| 30 ++ kernel/gcov/Makefile | 32 +++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/Documentation/gcov.txt b/Documentation/gcov.txt index e7ca647..7b72778 100644 --- a/Documentation/gcov.txt +++ b/Documentation/gcov.txt @@ -50,6 +50,10 @@ Configure the kernel with: CONFIG_DEBUG_FS=y CONFIG_GCOV_KERNEL=y +select the gcc's gcov format, default is autodetect based on gcc version: + +CONFIG_GCOV_FORMAT_AUTODETECT=y + and to get coverage data for the entire kernel: CONFIG_GCOV_PROFILE_ALL=y diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index d4da55d..d04ce8a 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL larger and run slower. Also be sure to exclude files from profiling which are not linked to the kernel image to prevent linker errors. +choice + prompt Specify GCOV format + depends on GCOV_KERNEL + default GCOV_FORMAT_AUTODETECT + ---help--- + The gcov format is usually determined by the GCC version, but there are + exceptions where format changes are integrated in lower-version GCCs. + In such a case use this option to adjust the format used in the kernel + accordingly. + + If unsure, choose Autodetect. + +config GCOV_FORMAT_AUTODETECT + bool Autodetect + ---help--- + Select this option to use the format that corresponds to your GCC + version. + +config GCOV_FORMAT_3_4 + bool GCC 3.4 format + ---help--- + Select this option to use the format defined by GCC 3.4. + +config GCOV_FORMAT_4_7 + bool GCC 4.7 format + ---help--- + Select this option to use the format defined by GCC 4.7. + +endchoice + endmenu diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index e97ca59..52aa7e8 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,3 +1,33 @@ ccflags-y := -DSRCTREE='$(srctree)' -DOBJTREE='$(objtree)' -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o +# if-lt +# Usage VAR := $(call if-lt, $(a), $(b)) +# Returns 1 if (a b) +if-lt = $(shell [ $(1) -lt $(2) ] echo 1) + +ifeq ($(CONFIG_GCOV_FORMAT_3_4),y) + cc-ver := 0304 +else ifeq ($(CONFIG_GCOV_FORMAT_4_7),y) + cc-ver := 0407 +else +# Use cc-version if available, otherwise set 0 +# +# scripts/Kbuild.include, which contains cc-version function, is not included +# during make clean make -f scripts/Makefile.clean obj=kernel/gcov +# Meaning cc-ver is empty causing if-lt test to fail with +# /bin/sh: line 0: [: -lt: unary operator expected error mesage. +# This has no affect on the clean phase, but the error message could be +# confusing/annoying. So this dummy workaround sets cc-ver to zero if cc-version +# is not available. We can probably move if-lt to Kbuild.include, so it's also +# not defined during clean or to include Kbuild.include in +# scripts/Makefile.clean. But the following workaround seems least invasive. + cc-ver := $(if $(call cc-version),$(call cc-version),0) +endif + +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o + +ifeq ($(call if-lt, $(cc-ver), 0407),1) + obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o +else + obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o +endif -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/4] gcov: add support for gcc 4.7 gcov format
The gcov in-memory format changed in gcc 4.7. The biggest change, which
requires this special implementation, is that gcov_info no longer contains
array of counters for each counter type for all functions and gcov_fn_info is
not used for mapping of function's counters to these arrays(offset). Now each
gcov_fn_info contans it's counters, which makes things a little bit easier.
This is heavily based on the previous gcc_3_4.c implementation and patches
provided by Peter Oberparleiter. Specially the buffer gcda implementation for
iterator.
v2: - removed const definition for gcov_fn_info in gcov_info
- use vmalloc for counter values in gcov_info_dup
- use iter buffer for gcda
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
kernel/gcov/base.c| 6 +
kernel/gcov/gcc_4_7.c | 562 ++
2 files changed, 568 insertions(+)
create mode 100644 kernel/gcov/gcc_4_7.c
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 912576a..f45b75b 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -79,6 +79,12 @@ void __gcov_merge_delta(gcov_type *counters, unsigned int
n_counters)
}
EXPORT_SYMBOL(__gcov_merge_delta);
+void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_ior);
+
/**
* gcov_enable_events - enable event reporting through gcov_event()
*
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
new file mode 100644
index 000..d91afee
--- /dev/null
+++ b/kernel/gcov/gcc_4_7.c
@@ -0,0 +1,562 @@
+/*
+ * This code provides functions to handle gcc's profiling data format
+ * introduced with gcc 4.7.
+ *
+ * This file is based heavily on gcc_3_4.c file.
+ *
+ * For a better understanding, refer to gcc source:
+ * gcc/gcov-io.h
+ * libgcc/libgcov.c
+ *
+ * Uses gcc-internal data definitions.
+ */
+
+#include linux/errno.h
+#include linux/slab.h
+#include linux/string.h
+#include linux/seq_file.h
+#include gcov.h
+
+#define GCOV_COUNTERS 8
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+static struct gcov_info *gcov_info_head;
+
+/**
+ * struct gcov_ctr_info - information about counters for a single function
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info {
+ unsigned int num;
+ gcov_type *values;
+};
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @key: comdat key
+ * @ident: unique ident of function
+ * @lineno_checksum: function lineo_checksum
+ * @cfg_checksum: function cfg checksum
+ * @ctrs: instrumented counters
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ *
+ * Information about a single function. This uses the trailing array
+ * idiom. The number of counters is determined from the merge pointer
+ * array in gcov_info. The key is used to detect which of a set of
+ * comdat functions was selected -- it points to the gcov_info object
+ * of the object file containing the selected comdat function.
+ */
+struct gcov_fn_info {
+ const struct gcov_info *key;
+ unsigned int ident;
+ unsigned int lineno_checksum;
+ unsigned int cfg_checksum;
+ struct gcov_ctr_info ctrs[0];
+};
+
+/**
+ * struct gcov_info - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: uniquifying time stamp
+ * @filename: name of the associated gcov data file
+ * @merge: merge functions (null for unused counter type)
+ * @n_functions: number of instrumented functions
+ * @functions: pointer to pointers to function information
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info {
+ unsigned int version;
+ struct gcov_info *next;
+ unsigned int stamp;
+ const char *filename;
+ void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int);
+ unsigned int n_functions;
+ struct gcov_fn_info **functions;
+};
+
+/**
+ * gcov_info_filename - return info filename
+ * @info: profiling data set
+ */
+const char *gcov_info_filename(struct gcov_info *info)
+{
+ return info-filename;
+}
+
+/**
+ * gcov_info_version - return info version
+ * @info: profiling data set
+ */
+unsigned int gcov_info_version(struct gcov_info *info)
+{
+ return info-version;
+}
+
+/**
+ * gcov_info_next - return next profiling data set
+ * @info: profiling data set
+ *
+ * Returns next gcov_info following @info or first gcov_info in the chain if
+ * @info is %NULL.
+ */
+struct gcov_info *gcov_info_next(struct gcov_info *info)
+{
+ if (!info)
+ return gcov_info_head;
+
+ return info-next
[PATCH v2 4/4] kernel: add support for init_array constructors
This adds the .init_array section as yet another section with constructors. This is needed because gcc could add __gcov_init calls to .init_array or .ctors section, depending on gcc version. v2: - reuse mod-ctors for .init_array section for modules, because gcc uses .ctors or .init_array, but not both at the same time Signed-off-by: Frantisek Hrbata fhrb...@redhat.com --- include/asm-generic/vmlinux.lds.h | 1 + kernel/module.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 69732d2..c55d8d9 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -468,6 +468,7 @@ #define KERNEL_CTORS() . = ALIGN(8); \ VMLINUX_SYMBOL(__ctors_start) = .; \ *(.ctors) \ + *(.init_array) \ VMLINUX_SYMBOL(__ctors_end) = .; #else #define KERNEL_CTORS() diff --git a/kernel/module.c b/kernel/module.c index 2069158..bbbd953 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, struct load_info *info) #ifdef CONFIG_CONSTRUCTORS mod-ctors = section_objs(info, .ctors, sizeof(*mod-ctors), mod-num_ctors); + if (!mod-ctors) + mod-ctors = section_objs(info, .init_array, + sizeof(*mod-ctors), mod-num_ctors); #endif #ifdef CONFIG_TRACEPOINTS -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/4] gcov: move gcov structs definitions to a gcc version specific file
Since also the gcov structures(gcov_info, gcov_fn_info, gcov_ctr_info) can
change between gcc releases, as shown in gcc 4.7, they cannot be defined in a
common header and need to be moved to a specific gcc implemention file. This
also requires to make the gcov_info structure opaque for the common code and to
introduce simple helpers for accessing data inside gcov_info.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
kernel/gcov/base.c| 26 ++--
kernel/gcov/fs.c | 27 ++--
kernel/gcov/gcc_3_4.c | 115 ++
kernel/gcov/gcov.h| 65 +---
4 files changed, 153 insertions(+), 80 deletions(-)
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9b22d03..912576a 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -20,7 +20,6 @@
#include linux/mutex.h
#include gcov.h
-static struct gcov_info *gcov_info_head;
static int gcov_events_enabled;
static DEFINE_MUTEX(gcov_lock);
@@ -34,7 +33,7 @@ void __gcov_init(struct gcov_info *info)
mutex_lock(gcov_lock);
if (gcov_version == 0) {
- gcov_version = info-version;
+ gcov_version = gcov_info_version(info);
/*
* Printing gcc's version magic may prove useful for debugging
* incompatibility reports.
@@ -45,8 +44,7 @@ void __gcov_init(struct gcov_info *info)
* Add new profiling data structure to list and inform event
* listener.
*/
- info-next = gcov_info_head;
- gcov_info_head = info;
+ gcov_info_link(info);
if (gcov_events_enabled)
gcov_event(GCOV_ADD, info);
mutex_unlock(gcov_lock);
@@ -91,13 +89,15 @@ EXPORT_SYMBOL(__gcov_merge_delta);
*/
void gcov_enable_events(void)
{
- struct gcov_info *info;
+ struct gcov_info *info = NULL;
mutex_lock(gcov_lock);
gcov_events_enabled = 1;
+
/* Perform event callback for previously registered entries. */
- for (info = gcov_info_head; info; info = info-next)
+ while ((info = gcov_info_next(info)))
gcov_event(GCOV_ADD, info);
+
mutex_unlock(gcov_lock);
}
@@ -112,25 +112,23 @@ static int gcov_module_notifier(struct notifier_block
*nb, unsigned long event,
void *data)
{
struct module *mod = data;
- struct gcov_info *info;
- struct gcov_info *prev;
+ struct gcov_info *info = NULL;
+ struct gcov_info *prev = NULL;
if (event != MODULE_STATE_GOING)
return NOTIFY_OK;
mutex_lock(gcov_lock);
- prev = NULL;
+
/* Remove entries located in module from linked list. */
- for (info = gcov_info_head; info; info = info-next) {
+ while ((info = gcov_info_next(info))) {
if (within(info, mod-module_core, mod-core_size)) {
- if (prev)
- prev-next = info-next;
- else
- gcov_info_head = info-next;
+ gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
} else
prev = info;
}
+
mutex_unlock(gcov_lock);
return NOTIFY_OK;
diff --git a/kernel/gcov/fs.c b/kernel/gcov/fs.c
index 9bd0934..27e12ce 100644
--- a/kernel/gcov/fs.c
+++ b/kernel/gcov/fs.c
@@ -242,7 +242,7 @@ static struct gcov_node *get_node_by_name(const char *name)
list_for_each_entry(node, all_head, all) {
info = get_node_info(node);
- if (info (strcmp(info-filename, name) == 0))
+ if (info (strcmp(gcov_info_filename(info), name) == 0))
return node;
}
@@ -279,7 +279,7 @@ static ssize_t gcov_seq_write(struct file *file, const char
__user *addr,
seq = file-private_data;
info = gcov_iter_get_info(seq-private);
mutex_lock(node_lock);
- node = get_node_by_name(info-filename);
+ node = get_node_by_name(gcov_info_filename(info));
if (node) {
/* Reset counts or remove node for unloaded modules. */
if (node-num_loaded == 0)
@@ -376,8 +376,9 @@ static void add_links(struct gcov_node *node, struct dentry
*parent)
if (!node-links)
return;
for (i = 0; i num; i++) {
- target = get_link_target(get_node_info(node)-filename,
-gcov_link[i]);
+ target = get_link_target(
+ gcov_info_filename(get_node_info(node)),
+ gcov_link[i]);
if (!target)
goto out_err;
basename = strrchr(target, '/');
@@ -576,7 +577,7 @@ static void add_node(struct gcov_info *info
[PATCH v2 0/4] add support for gcov format introduced in gcc 4.7
This is an attempt to bring support for modified gcov format in gcc 4.7 to the kernel. It tries to leverage the existing layout/abstraction, which was designed keeping in mind that the gcov format could change, but some changes had to be make. Mostly because the current model does not take into account that even the core gcov structures, like gcov_info, could change. One part that could be problematic is the addition of the .init_array section for constructors. Tested with lcov and seems to be working fine, giving similar results as for the older format. v2: Included suggestions/code provided by Peter Oberparleiter. Detailed description in patches. Frantisek Hrbata (4): gcov: move gcov structs definitions to a gcc version specific file gcov: add support for gcc 4.7 gcov format gcov: compile specific gcov implementation based on gcc version kernel: add support for init_array constructors Documentation/gcov.txt| 4 + include/asm-generic/vmlinux.lds.h | 1 + kernel/gcov/Kconfig | 30 ++ kernel/gcov/Makefile | 32 ++- kernel/gcov/base.c| 32 ++- kernel/gcov/fs.c | 27 +- kernel/gcov/gcc_3_4.c | 115 kernel/gcov/gcc_4_7.c | 562 ++ kernel/gcov/gcov.h| 65 + kernel/module.c | 3 + 10 files changed, 790 insertions(+), 81 deletions(-) create mode 100644 kernel/gcov/gcc_4_7.c -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Wed, Aug 28, 2013 at 03:46:05PM +0200, Peter Oberparleiter wrote: > On 27.08.2013 15:34, Frantisek Hrbata wrote: > > On Mon, Aug 26, 2013 at 04:14:07PM +0200, Peter Oberparleiter wrote: > >> On 24.08.2013 21:44, Frantisek Hrbata wrote: > >>> On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: > >>>> On 23.08.2013 17:15, Peter Oberparleiter wrote: > >>>>> On 23.08.2013 10:39, Frantisek Hrbata wrote: > >>>>>> Compile the correct gcov implementation file for a specific gcc > >>>>>> version. In > >>>>>> the future, if another file is added, the conditions will need to be > >>>>>> somehow > >>>>>> adjusted to if-elif-else case, but at this point the simple > >>>>>> cc-ifversion should > >>>>>> be enough. > >>>> > >>>> As promised, I'm also adding the patch that makes the format-specific > >>>> part > >>>> of gcov-kernel a loadable kernel module: > >>>> > >>>> --- > >>>> kernel: gcov: make format-specific code loadable > >>>> > >>>> Turn the format-specific part of gcov-kernel into a loadable kernel > >>>> module. This enables the use of gcov-kernel with kernel modules > >>>> that were compiled with a version of GCC that produces a different > >>>> gcov format when compared to the version of GCC that was used to > >>>> compile the kernel. > >>> > >>> If I understand it correctly, this would mean that you will be able to > >>> use only > >>> one implementation of gcov format at the time. Meaning you will be able > >>> to get > >>> coverage data for module, but not for kernel if it was compiled with > >>> different > >>> gcc(gcda format). This is probably ok if you work only on your module, > >>> but I'm > >>> not sure this is generally the right approach. In this case I would > >>> probably > >>> rather see some support for more gcov formats at the same time(e.g. set of > >>> callback operations per gcov version). Again I'm probably missing > >>> something, but > >>> I still cannot see reason why to add such feature. If you want gcov > >>> support just > >>> compile your kernel and modules with the same gcc version(gcda format). > >>> But if > >>> this is really needed maybe it would be better to consider some parallel > >>> support > >>> for more gcov formats based on the gcov_info version. > >> > >> The callback approach has other drawbacks (see previous mail). > > > > Agreed, I did not realized these problems for the first time when I was > > thinking > > about the callback approach. > > > >> > >>> Would it be possible to add support for the modified gcc 4.7 gcov format > >>> and > >>> deal with this later? I can incorporate your changes: iter to use buffer, > >>> .init_array for modules and possibility to explicitly select the gcda > >>> format. > >>> In this case we will have at least the basic support in kernel. This is > >>> just me > >>> thinking out loud. > >> > >> I think that's an approach I can live with. Maybe the need for a > >> multi-version > >> support will surface again later in a more refined form, but until then > >> there > >> should be no reason to delay base GCC 4.7 support any further. > > > > Great. I can incorporate the changes you proposed and post V2. Or do you > > prefer > > to post it by your self? Based on the info and patches you already provided > > I > > guess you already have something ready. Simply what suits you best :). > > If it's not too much hassle, I'd prefer that you post V2 of your patches. Also > I guess it would make sense to put Andrew Morton on CC since the initial > gcov-patches were picked up by him. Ok, no problem. I will prepare v2. Again many thanks Peter for your help and input. It's really appreciated. > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Wed, Aug 28, 2013 at 03:46:05PM +0200, Peter Oberparleiter wrote: On 27.08.2013 15:34, Frantisek Hrbata wrote: On Mon, Aug 26, 2013 at 04:14:07PM +0200, Peter Oberparleiter wrote: On 24.08.2013 21:44, Frantisek Hrbata wrote: On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: On 23.08.2013 17:15, Peter Oberparleiter wrote: On 23.08.2013 10:39, Frantisek Hrbata wrote: Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. As promised, I'm also adding the patch that makes the format-specific part of gcov-kernel a loadable kernel module: --- kernel: gcov: make format-specific code loadable Turn the format-specific part of gcov-kernel into a loadable kernel module. This enables the use of gcov-kernel with kernel modules that were compiled with a version of GCC that produces a different gcov format when compared to the version of GCC that was used to compile the kernel. If I understand it correctly, this would mean that you will be able to use only one implementation of gcov format at the time. Meaning you will be able to get coverage data for module, but not for kernel if it was compiled with different gcc(gcda format). This is probably ok if you work only on your module, but I'm not sure this is generally the right approach. In this case I would probably rather see some support for more gcov formats at the same time(e.g. set of callback operations per gcov version). Again I'm probably missing something, but I still cannot see reason why to add such feature. If you want gcov support just compile your kernel and modules with the same gcc version(gcda format). But if this is really needed maybe it would be better to consider some parallel support for more gcov formats based on the gcov_info version. The callback approach has other drawbacks (see previous mail). Agreed, I did not realized these problems for the first time when I was thinking about the callback approach. Would it be possible to add support for the modified gcc 4.7 gcov format and deal with this later? I can incorporate your changes: iter to use buffer, .init_array for modules and possibility to explicitly select the gcda format. In this case we will have at least the basic support in kernel. This is just me thinking out loud. I think that's an approach I can live with. Maybe the need for a multi-version support will surface again later in a more refined form, but until then there should be no reason to delay base GCC 4.7 support any further. Great. I can incorporate the changes you proposed and post V2. Or do you prefer to post it by your self? Based on the info and patches you already provided I guess you already have something ready. Simply what suits you best :). If it's not too much hassle, I'd prefer that you post V2 of your patches. Also I guess it would make sense to put Andrew Morton on CC since the initial gcov-patches were picked up by him. Ok, no problem. I will prepare v2. Again many thanks Peter for your help and input. It's really appreciated. -- Peter Oberparleiter Linux on System z Development - IBM Germany -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/4] gcov: add support for gcc 4.7 gcov format
On Mon, Aug 26, 2013 at 02:45:10PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 23:00, Frantisek Hrbata wrote: > > On Fri, Aug 23, 2013 at 05:12:19PM +0200, Peter Oberparleiter wrote: > >> On 23.08.2013 10:39, Frantisek Hrbata wrote: > >>> The gcov in-memory format changed in gcc 4.7. The biggest change, which > >>> requires this special implementation, is that gcov_info no longer contains > >>> array of counters for each counter type for all functions and > >>> gcov_fn_info is > >>> not used for mapping of function's counters to these arrays(offset). Now > >>> each > >>> gcov_fn_info contans it's counters, which makes things a little bit > >>> easier. > >> > >> By now I've come to the conclusion that I may have over-engineered > >> gcov_iter_next in the original GCC 3.4 implementation. This became > >> especially > >> apparent when someone tried to adjust that code to also work with a > >> specific > >> android GCC version - adding a simple field to the output file format > >> required > >> major complex changes. > >> > >> Therefore in my version of the GCC 4.7 support, I opted to replace this > >> logic > >> with the simpler approach of generating the gcov data file in-memory during > >> open (gcov_iter_new) and reducing gcov_iter_next/gcov_iter_write to simple > >> copy-from-buffer functions. > > > > Yes, this seems reasonable and much easier approach to me, but we will end > > up > > with three copies of one gcov(gcda) file data in memory: data from gcc, our > > buffer and the buffer in seq file. But I guess this is not a big problem, > > unless someone opens a huge amount of gcda files in parallel. Generally I > > like this idea. This will be also probably faster then writing 1 or 2 ints > > per > > one iter write. > > You're right about the data being in multiple buffers for the time that a > .gcda > file is opened, but as you mentioned, this shouldn't be a problem since > usually > only one .gcda file is open at a time. Also the reduction in code complexity > is > in my opinion well worth it. agreed > > >> Since I can see the need to change the format code again in the future, > >> I think it would be easier to simplify this part of the code > >> correspondingly. > >> I'm adding my version of this part of the implementation as discussion > >> input > >> below (the relevant part starts at convert_to_gcda()). > > > > I have really only two nits to the code(please see below). I spent some time > > checking the new seq/iter implementation and it seems ok to me. Generally I > > like > > how simple this change is done. > > > >> > >> --- > >> kernel: gcov: add support for gcc 4.7 gcov format > >> > >> GCC 4.7 changed the format of gcov related data structures, both > >> on-disk and in memory. Add support for the new format. > >> > >> Signed-off-by: Peter Oberparleiter > >> --- > >> kernel/gcov/Makefile |4 > >> kernel/gcov/gcc_4_7.c | 510 > >> ++ > >> 2 files changed, 513 insertions(+), 1 deletion(-) > >> > >> --- a/kernel/gcov/Makefile > >> +++ b/kernel/gcov/Makefile > >> @@ -1,3 +1,5 @@ > >> ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' > >> > >> -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o > >> +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o > >> +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) > >> +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) > >> --- /dev/null > >> +++ b/kernel/gcov/gcc_4_7.c > >> @@ -0,0 +1,510 @@ > >> +/* > >> + * This code provides functions to handle gcc's profiling data format > >> + * introduced with gcc 4.7. Future versions of gcc may change the gcov > >> + * format (as happened before), so all format-specific information needs > >> + * to be kept modular and easily exchangeable. > >> + * > >> + * This file is based on gcc-internal definitions. Functions and data > >> + * structures are defined to be compatible with gcc counterparts. > >> + * For a better understanding, refer to gcc source: gcc/gcov-io.h. > >> + * > >> + *Copyright IBM Corp. 2013 > >> + *Author(s): Peter Oberparleiter > >> + * > >> + *Uses gcc-internal
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Mon, Aug 26, 2013 at 04:14:07PM +0200, Peter Oberparleiter wrote: > On 24.08.2013 21:44, Frantisek Hrbata wrote: > > On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: > >> On 23.08.2013 17:15, Peter Oberparleiter wrote: > >>> On 23.08.2013 10:39, Frantisek Hrbata wrote: > >>>> Compile the correct gcov implementation file for a specific gcc version. > >>>> In > >>>> the future, if another file is added, the conditions will need to be > >>>> somehow > >>>> adjusted to if-elif-else case, but at this point the simple cc-ifversion > >>>> should > >>>> be enough. > >> > >> As promised, I'm also adding the patch that makes the format-specific part > >> of gcov-kernel a loadable kernel module: > >> > >> --- > >> kernel: gcov: make format-specific code loadable > >> > >> Turn the format-specific part of gcov-kernel into a loadable kernel > >> module. This enables the use of gcov-kernel with kernel modules > >> that were compiled with a version of GCC that produces a different > >> gcov format when compared to the version of GCC that was used to > >> compile the kernel. > > > > If I understand it correctly, this would mean that you will be able to use > > only > > one implementation of gcov format at the time. Meaning you will be able to > > get > > coverage data for module, but not for kernel if it was compiled with > > different > > gcc(gcda format). This is probably ok if you work only on your module, but > > I'm > > not sure this is generally the right approach. In this case I would probably > > rather see some support for more gcov formats at the same time(e.g. set of > > callback operations per gcov version). Again I'm probably missing > > something, but > > I still cannot see reason why to add such feature. If you want gcov support > > just > > compile your kernel and modules with the same gcc version(gcda format). But > > if > > this is really needed maybe it would be better to consider some parallel > > support > > for more gcov formats based on the gcov_info version. > > The callback approach has other drawbacks (see previous mail). Agreed, I did not realized these problems for the first time when I was thinking about the callback approach. > > > Would it be possible to add support for the modified gcc 4.7 gcov format and > > deal with this later? I can incorporate your changes: iter to use buffer, > > .init_array for modules and possibility to explicitly select the gcda > > format. > > In this case we will have at least the basic support in kernel. This is > > just me > > thinking out loud. > > I think that's an approach I can live with. Maybe the need for a multi-version > support will surface again later in a more refined form, but until then there > should be no reason to delay base GCC 4.7 support any further. Great. I can incorporate the changes you proposed and post V2. Or do you prefer to post it by your self? Based on the info and patches you already provided I guess you already have something ready. Simply what suits you best :). Many thanks Peter > > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Mon, Aug 26, 2013 at 02:56:04PM +0200, Peter Oberparleiter wrote: > On 24.08.2013 21:12, Frantisek Hrbata wrote: > > On Fri, Aug 23, 2013 at 05:15:19PM +0200, Peter Oberparleiter wrote: > >> Also it is my understanding that there are some distribution-specific > >> versions > >> of GCC that include the 4.7. gcov format code but report GCC version 4.6. > >> With > >> the auto-detection code implemented like this, gcov-kernel won't work > >> correctly. > >> For that purpose I've implemented a configuration option to allow users to > >> force a specific version of gcov format. > > > > Ah, I was not aware of this inconsistency in versioning. This raises a > > question > > if it would not be better to deal directly with version in the gcov_info > > instead of these config options. This would of course mean some kind of gcov > > operations callbacks per gcov version(you already mentioned the file > > operations approach). > > Using the version information from gcov_info would make the support easier to > use, > but I see 2 problems with this approach: > > 1. How do you decide which version applies to any given gcov_info struct? > Since gcov_info is opaque this would require a compatibility check inside > the version specific code. And each version specific code assumes a different > layout for gcov_info, so in an (unlikely) worst case scenario, the > compatibility > check might fail (false positive or access beyond end of data structure) due > to > these differences > > 2. There's no easy way to "force" compatibility, for example in the case > mentioned above where a GCC 4.6 produces GCC 4.7 gcov format data. Yes, you are right of course. Problem imho is that user-space program is always linked with the correct libgcov. Meaning the gcov structures and the code dumping the gcda are tied and known(compatible) at the compilation(link) time. So there is no need to deal with different gcov_info layouts. After a little bit more thinking the "per gcov_info version callbacks" does not seem as the right way to go anymore. At this point I think the easier and probably the safest way would be the module approach as you originally proposed. Anyway I would leave this aside for now and we will see if someone requests this in the future. For now I would propose to simply support only one gcov version at the time. Meaning if you want to use gcov, compile your kernel and modules with the same gcc version. And add the possibility to explicitly select gcov version(3.4 or 4.7) during configuration as you proposed in your patch. IMHO this should be a good start :) > > >> I'm attaching the corresponding patch below: > >> > >> --- > >> kernel: gcov: make data format configurable > >> > >> Make the format of the generated gcov data configurable. This may be > >> required for example for pre-4.7 GCCs that contain the 4.7 gcov data > >> format changes. > >> > >> Signed-off-by: Peter Oberparleiter > >> --- > >> kernel/gcov/Kconfig | 30 ++ > >> kernel/gcov/Makefile | 21 +++-- > >> 2 files changed, 49 insertions(+), 2 deletions(-) > >> > >> --- a/kernel/gcov/Kconfig > >> +++ b/kernel/gcov/Kconfig > >> @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL > >>larger and run slower. Also be sure to exclude files from profiling > >>which are not linked to the kernel image to prevent linker errors. > >> > >> +choice > >> + prompt "Specify GCOV format" > >> + depends on GCOV_KERNEL > >> + default GCOV_FORMAT_AUTODETECT > >> + ---help--- > >> + The gcov format is usually determined by the GCC version, but there are > >> + exceptions where format changes are integrated in lower-version GCCs. > >> + In such a case use this option to adjust the format used in the kernel > >> + accordingly. > >> + > >> + If unsure, choose "Autodetect". > >> + > >> +config GCOV_FORMAT_AUTODETECT > >> + bool "Autodetect" > >> + ---help--- > >> + Select this option to use the format that corresponds to your GCC > >> + version. > >> + > >> +config GCOV_FORMAT_3_4 > >> + bool "GCC 3.4 format" > >> + ---help--- > >> + Select this option to use the format defined by GCC 3.4. > >> + > >> +config GCOV_FORMAT_4_7 > >> + bool "GCC 4.7 format" > >> + ---help--- > >> + Select this option to use the format defined by GCC 4.7.
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Mon, Aug 26, 2013 at 02:56:04PM +0200, Peter Oberparleiter wrote: On 24.08.2013 21:12, Frantisek Hrbata wrote: On Fri, Aug 23, 2013 at 05:15:19PM +0200, Peter Oberparleiter wrote: Also it is my understanding that there are some distribution-specific versions of GCC that include the 4.7. gcov format code but report GCC version 4.6. With the auto-detection code implemented like this, gcov-kernel won't work correctly. For that purpose I've implemented a configuration option to allow users to force a specific version of gcov format. Ah, I was not aware of this inconsistency in versioning. This raises a question if it would not be better to deal directly with version in the gcov_info instead of these config options. This would of course mean some kind of gcov operations callbacks per gcov version(you already mentioned the file operations approach). Using the version information from gcov_info would make the support easier to use, but I see 2 problems with this approach: 1. How do you decide which version applies to any given gcov_info struct? Since gcov_info is opaque this would require a compatibility check inside the version specific code. And each version specific code assumes a different layout for gcov_info, so in an (unlikely) worst case scenario, the compatibility check might fail (false positive or access beyond end of data structure) due to these differences 2. There's no easy way to force compatibility, for example in the case mentioned above where a GCC 4.6 produces GCC 4.7 gcov format data. Yes, you are right of course. Problem imho is that user-space program is always linked with the correct libgcov. Meaning the gcov structures and the code dumping the gcda are tied and known(compatible) at the compilation(link) time. So there is no need to deal with different gcov_info layouts. After a little bit more thinking the per gcov_info version callbacks does not seem as the right way to go anymore. At this point I think the easier and probably the safest way would be the module approach as you originally proposed. Anyway I would leave this aside for now and we will see if someone requests this in the future. For now I would propose to simply support only one gcov version at the time. Meaning if you want to use gcov, compile your kernel and modules with the same gcc version. And add the possibility to explicitly select gcov version(3.4 or 4.7) during configuration as you proposed in your patch. IMHO this should be a good start :) I'm attaching the corresponding patch below: --- kernel: gcov: make data format configurable Make the format of the generated gcov data configurable. This may be required for example for pre-4.7 GCCs that contain the 4.7 gcov data format changes. Signed-off-by: Peter Oberparleiter ober...@linux.vnet.ibm.com --- kernel/gcov/Kconfig | 30 ++ kernel/gcov/Makefile | 21 +++-- 2 files changed, 49 insertions(+), 2 deletions(-) --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL larger and run slower. Also be sure to exclude files from profiling which are not linked to the kernel image to prevent linker errors. +choice + prompt Specify GCOV format + depends on GCOV_KERNEL + default GCOV_FORMAT_AUTODETECT + ---help--- + The gcov format is usually determined by the GCC version, but there are + exceptions where format changes are integrated in lower-version GCCs. + In such a case use this option to adjust the format used in the kernel + accordingly. + + If unsure, choose Autodetect. + +config GCOV_FORMAT_AUTODETECT + bool Autodetect + ---help--- + Select this option to use the format that corresponds to your GCC + version. + +config GCOV_FORMAT_3_4 + bool GCC 3.4 format + ---help--- + Select this option to use the format defined by GCC 3.4. + +config GCOV_FORMAT_4_7 + bool GCC 4.7 format + ---help--- + Select this option to use the format defined by GCC 4.7. + +endchoice + endmenu --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,5 +1,22 @@ ccflags-y := -DSRCTREE='$(srctree)' -DOBJTREE='$(objtree)' +# if-lt +# Usage VAR := $(call if-lt, $(a), $(b)) +# Returns 1 if (a b) +if-lt = $(shell [ $(1) -lt $(2) ] echo 1) + +ifeq ($(CONFIG_GCOV_FORMAT_3_4),y) + cc-ver := 0304 +else ifeq ($(CONFIG_GCOV_FORMAT_4_7),y) + cc-ver := 0407 +else + cc-ver := $(call cc-version) +endif + obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) + +ifeq ($(call if-lt, $(cc-ver), 0407),1) + obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o +else + obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o +endif -- Peter Oberparleiter Linux
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Mon, Aug 26, 2013 at 04:14:07PM +0200, Peter Oberparleiter wrote: On 24.08.2013 21:44, Frantisek Hrbata wrote: On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: On 23.08.2013 17:15, Peter Oberparleiter wrote: On 23.08.2013 10:39, Frantisek Hrbata wrote: Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. As promised, I'm also adding the patch that makes the format-specific part of gcov-kernel a loadable kernel module: --- kernel: gcov: make format-specific code loadable Turn the format-specific part of gcov-kernel into a loadable kernel module. This enables the use of gcov-kernel with kernel modules that were compiled with a version of GCC that produces a different gcov format when compared to the version of GCC that was used to compile the kernel. If I understand it correctly, this would mean that you will be able to use only one implementation of gcov format at the time. Meaning you will be able to get coverage data for module, but not for kernel if it was compiled with different gcc(gcda format). This is probably ok if you work only on your module, but I'm not sure this is generally the right approach. In this case I would probably rather see some support for more gcov formats at the same time(e.g. set of callback operations per gcov version). Again I'm probably missing something, but I still cannot see reason why to add such feature. If you want gcov support just compile your kernel and modules with the same gcc version(gcda format). But if this is really needed maybe it would be better to consider some parallel support for more gcov formats based on the gcov_info version. The callback approach has other drawbacks (see previous mail). Agreed, I did not realized these problems for the first time when I was thinking about the callback approach. Would it be possible to add support for the modified gcc 4.7 gcov format and deal with this later? I can incorporate your changes: iter to use buffer, .init_array for modules and possibility to explicitly select the gcda format. In this case we will have at least the basic support in kernel. This is just me thinking out loud. I think that's an approach I can live with. Maybe the need for a multi-version support will surface again later in a more refined form, but until then there should be no reason to delay base GCC 4.7 support any further. Great. I can incorporate the changes you proposed and post V2. Or do you prefer to post it by your self? Based on the info and patches you already provided I guess you already have something ready. Simply what suits you best :). Many thanks Peter -- Peter Oberparleiter Linux on System z Development - IBM Germany -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/4] gcov: add support for gcc 4.7 gcov format
On Mon, Aug 26, 2013 at 02:45:10PM +0200, Peter Oberparleiter wrote: On 23.08.2013 23:00, Frantisek Hrbata wrote: On Fri, Aug 23, 2013 at 05:12:19PM +0200, Peter Oberparleiter wrote: On 23.08.2013 10:39, Frantisek Hrbata wrote: The gcov in-memory format changed in gcc 4.7. The biggest change, which requires this special implementation, is that gcov_info no longer contains array of counters for each counter type for all functions and gcov_fn_info is not used for mapping of function's counters to these arrays(offset). Now each gcov_fn_info contans it's counters, which makes things a little bit easier. By now I've come to the conclusion that I may have over-engineered gcov_iter_next in the original GCC 3.4 implementation. This became especially apparent when someone tried to adjust that code to also work with a specific android GCC version - adding a simple field to the output file format required major complex changes. Therefore in my version of the GCC 4.7 support, I opted to replace this logic with the simpler approach of generating the gcov data file in-memory during open (gcov_iter_new) and reducing gcov_iter_next/gcov_iter_write to simple copy-from-buffer functions. Yes, this seems reasonable and much easier approach to me, but we will end up with three copies of one gcov(gcda) file data in memory: data from gcc, our buffer and the buffer in seq file. But I guess this is not a big problem, unless someone opens a huge amount of gcda files in parallel. Generally I like this idea. This will be also probably faster then writing 1 or 2 ints per one iter write. You're right about the data being in multiple buffers for the time that a .gcda file is opened, but as you mentioned, this shouldn't be a problem since usually only one .gcda file is open at a time. Also the reduction in code complexity is in my opinion well worth it. agreed Since I can see the need to change the format code again in the future, I think it would be easier to simplify this part of the code correspondingly. I'm adding my version of this part of the implementation as discussion input below (the relevant part starts at convert_to_gcda()). I have really only two nits to the code(please see below). I spent some time checking the new seq/iter implementation and it seems ok to me. Generally I like how simple this change is done. --- kernel: gcov: add support for gcc 4.7 gcov format GCC 4.7 changed the format of gcov related data structures, both on-disk and in memory. Add support for the new format. Signed-off-by: Peter Oberparleiter ober...@linux.vnet.ibm.com --- kernel/gcov/Makefile |4 kernel/gcov/gcc_4_7.c | 510 ++ 2 files changed, 513 insertions(+), 1 deletion(-) --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,3 +1,5 @@ ccflags-y := -DSRCTREE='$(srctree)' -DOBJTREE='$(objtree)' -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) --- /dev/null +++ b/kernel/gcov/gcc_4_7.c @@ -0,0 +1,510 @@ +/* + * This code provides functions to handle gcc's profiling data format + * introduced with gcc 4.7. Future versions of gcc may change the gcov + * format (as happened before), so all format-specific information needs + * to be kept modular and easily exchangeable. + * + * This file is based on gcc-internal definitions. Functions and data + * structures are defined to be compatible with gcc counterparts. + * For a better understanding, refer to gcc source: gcc/gcov-io.h. + * + *Copyright IBM Corp. 2013 + *Author(s): Peter Oberparleiter ober...@linux.vnet.ibm.com + * + *Uses gcc-internal data definitions. + */ + +#include linux/errno.h +#include linux/slab.h +#include linux/string.h +#include linux/seq_file.h +#include linux/vmalloc.h +#include linux/types.h +#include gcov.h + +/* + * Profiling data types used for gcc 4.7 and above - these are defined by + * gcc and need to be kept as close to the original definition as possible to + * remain compatible. + */ +#define GCOV_COUNTERS 8 +#define GCOV_DATA_MAGIC ((unsigned int) 0x67636461) +#define GCOV_TAG_FUNCTION ((unsigned int) 0x0100) +#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a1) +#define GCOV_TAG_FOR_COUNTER(count) \ + (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) 17)) + +#if BITS_PER_LONG = 64 +typedef long gcov_type; +#else +typedef long long gcov_type; +#endif + +/** + * struct gcov_ctr_info - profiling data per function and counter type + * @num: number of counter values
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 17:15, Peter Oberparleiter wrote: > > On 23.08.2013 10:39, Frantisek Hrbata wrote: > >> Compile the correct gcov implementation file for a specific gcc version. In > >> the future, if another file is added, the conditions will need to be > >> somehow > >> adjusted to if-elif-else case, but at this point the simple cc-ifversion > >> should > >> be enough. > > As promised, I'm also adding the patch that makes the format-specific part > of gcov-kernel a loadable kernel module: > > --- > kernel: gcov: make format-specific code loadable > > Turn the format-specific part of gcov-kernel into a loadable kernel > module. This enables the use of gcov-kernel with kernel modules > that were compiled with a version of GCC that produces a different > gcov format when compared to the version of GCC that was used to > compile the kernel. If I understand it correctly, this would mean that you will be able to use only one implementation of gcov format at the time. Meaning you will be able to get coverage data for module, but not for kernel if it was compiled with different gcc(gcda format). This is probably ok if you work only on your module, but I'm not sure this is generally the right approach. In this case I would probably rather see some support for more gcov formats at the same time(e.g. set of callback operations per gcov version). Again I'm probably missing something, but I still cannot see reason why to add such feature. If you want gcov support just compile your kernel and modules with the same gcc version(gcda format). But if this is really needed maybe it would be better to consider some parallel support for more gcov formats based on the gcov_info version. Would it be possible to add support for the modified gcc 4.7 gcov format and deal with this later? I can incorporate your changes: iter to use buffer, .init_array for modules and possibility to explicitly select the gcda format. In this case we will have at least the basic support in kernel. This is just me thinking out loud. Many thanks Peter! > > Signed-off-by: Peter Oberparleiter > --- > kernel/gcov/Kconfig | 19 +-- > kernel/gcov/Makefile |7 --- > 2 files changed, 21 insertions(+), 5 deletions(-) > > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -29,8 +29,23 @@ config GCOV_KERNEL > and: > GCOV_PROFILE := n > > - Note that the debugfs filesystem has to be mounted to access > - profiling data. > + Note that GCOV_KERNEL_FS has to specified as well and the debugfs > + filesystem has to be mounted to access profiling data. > + > +config GCOV_KERNEL_FS > + tristate "Provide gcov data files in debugfs" > + depends on GCOV_KERNEL > + default y > + ---help--- > + Make profiling data available in debugfs at /sys/kernel/debug/gcov. > + > + Say M if you want to enable collecting coverage data for kernel modules > + which are compiled using a gcc version different from the one that > + was used to compile the kernel. In that case, re-compile the gcov > + kernel module with corresponding format support and load that module > + instead. > + > + If unsure, say Y. > > config GCOV_PROFILE_ALL > bool "Profile entire Kernel" > --- a/kernel/gcov/Makefile > +++ b/kernel/gcov/Makefile > @@ -13,10 +13,11 @@ else >cc-ver := $(call cc-version) > endif > > -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o > +obj-$(CONFIG_GCOV_KERNEL) += base.o > +obj-$(CONFIG_GCOV_KERNEL_FS) += gcov.o > > ifeq ($(call if-lt, $(cc-ver), 0407),1) > - obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o > + gcov-objs += fs.o gcc_3_4.o > else > - obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o > + gcov-objs += fs.o gcc_4_7.o > endif > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Fri, Aug 23, 2013 at 05:15:19PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 10:39, Frantisek Hrbata wrote: > > Compile the correct gcov implementation file for a specific gcc version. In > > the future, if another file is added, the conditions will need to be somehow > > adjusted to if-elif-else case, but at this point the simple cc-ifversion > > should > > be enough. > > Looks good, though I think this could be merged into the main 4.7 format > patch, > since without it, the 4.7 code will never be reached. Sure, I can merge these two patches. > > Also it is my understanding that there are some distribution-specific versions > of GCC that include the 4.7. gcov format code but report GCC version 4.6. With > the auto-detection code implemented like this, gcov-kernel won't work > correctly. > For that purpose I've implemented a configuration option to allow users to > force a specific version of gcov format. Ah, I was not aware of this inconsistency in versioning. This raises a question if it would not be better to deal directly with version in the gcov_info instead of these config options. This would of course mean some kind of gcov operations callbacks per gcov version(you already mentioned the file operations approach). > > I'm attaching the corresponding patch below: > > --- > kernel: gcov: make data format configurable > > Make the format of the generated gcov data configurable. This may be > required for example for pre-4.7 GCCs that contain the 4.7 gcov data > format changes. > > Signed-off-by: Peter Oberparleiter > --- > kernel/gcov/Kconfig | 30 ++ > kernel/gcov/Makefile | 21 +++-- > 2 files changed, 49 insertions(+), 2 deletions(-) > > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL > larger and run slower. Also be sure to exclude files from profiling > which are not linked to the kernel image to prevent linker errors. > > +choice > + prompt "Specify GCOV format" > + depends on GCOV_KERNEL > + default GCOV_FORMAT_AUTODETECT > + ---help--- > + The gcov format is usually determined by the GCC version, but there are > + exceptions where format changes are integrated in lower-version GCCs. > + In such a case use this option to adjust the format used in the kernel > + accordingly. > + > + If unsure, choose "Autodetect". > + > +config GCOV_FORMAT_AUTODETECT > + bool "Autodetect" > + ---help--- > + Select this option to use the format that corresponds to your GCC > + version. > + > +config GCOV_FORMAT_3_4 > + bool "GCC 3.4 format" > + ---help--- > + Select this option to use the format defined by GCC 3.4. > + > +config GCOV_FORMAT_4_7 > + bool "GCC 4.7 format" > + ---help--- > + Select this option to use the format defined by GCC 4.7. > + > +endchoice > + > endmenu > --- a/kernel/gcov/Makefile > +++ b/kernel/gcov/Makefile > @@ -1,5 +1,22 @@ > ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' > > +# if-lt > +# Usage VAR := $(call if-lt, $(a), $(b)) > +# Returns 1 if (a < b) > +if-lt = $(shell [ $(1) -lt $(2) ] && echo 1) > + > +ifeq ($(CONFIG_GCOV_FORMAT_3_4),y) > + cc-ver := 0304 > +else ifeq ($(CONFIG_GCOV_FORMAT_4_7),y) > + cc-ver := 0407 > +else > + cc-ver := $(call cc-version) > +endif > + > obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o > -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) > -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) > + > +ifeq ($(call if-lt, $(cc-ver), 0407),1) > + obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o > +else > + obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o > +endif > > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Fri, Aug 23, 2013 at 05:15:19PM +0200, Peter Oberparleiter wrote: On 23.08.2013 10:39, Frantisek Hrbata wrote: Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. Looks good, though I think this could be merged into the main 4.7 format patch, since without it, the 4.7 code will never be reached. Sure, I can merge these two patches. Also it is my understanding that there are some distribution-specific versions of GCC that include the 4.7. gcov format code but report GCC version 4.6. With the auto-detection code implemented like this, gcov-kernel won't work correctly. For that purpose I've implemented a configuration option to allow users to force a specific version of gcov format. Ah, I was not aware of this inconsistency in versioning. This raises a question if it would not be better to deal directly with version in the gcov_info instead of these config options. This would of course mean some kind of gcov operations callbacks per gcov version(you already mentioned the file operations approach). I'm attaching the corresponding patch below: --- kernel: gcov: make data format configurable Make the format of the generated gcov data configurable. This may be required for example for pre-4.7 GCCs that contain the 4.7 gcov data format changes. Signed-off-by: Peter Oberparleiter ober...@linux.vnet.ibm.com --- kernel/gcov/Kconfig | 30 ++ kernel/gcov/Makefile | 21 +++-- 2 files changed, 49 insertions(+), 2 deletions(-) --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -46,4 +46,34 @@ config GCOV_PROFILE_ALL larger and run slower. Also be sure to exclude files from profiling which are not linked to the kernel image to prevent linker errors. +choice + prompt Specify GCOV format + depends on GCOV_KERNEL + default GCOV_FORMAT_AUTODETECT + ---help--- + The gcov format is usually determined by the GCC version, but there are + exceptions where format changes are integrated in lower-version GCCs. + In such a case use this option to adjust the format used in the kernel + accordingly. + + If unsure, choose Autodetect. + +config GCOV_FORMAT_AUTODETECT + bool Autodetect + ---help--- + Select this option to use the format that corresponds to your GCC + version. + +config GCOV_FORMAT_3_4 + bool GCC 3.4 format + ---help--- + Select this option to use the format defined by GCC 3.4. + +config GCOV_FORMAT_4_7 + bool GCC 4.7 format + ---help--- + Select this option to use the format defined by GCC 4.7. + +endchoice + endmenu --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,5 +1,22 @@ ccflags-y := -DSRCTREE='$(srctree)' -DOBJTREE='$(objtree)' +# if-lt +# Usage VAR := $(call if-lt, $(a), $(b)) +# Returns 1 if (a b) +if-lt = $(shell [ $(1) -lt $(2) ] echo 1) + +ifeq ($(CONFIG_GCOV_FORMAT_3_4),y) + cc-ver := 0304 +else ifeq ($(CONFIG_GCOV_FORMAT_4_7),y) + cc-ver := 0407 +else + cc-ver := $(call cc-version) +endif + obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) -obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) + +ifeq ($(call if-lt, $(cc-ver), 0407),1) + obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o +else + obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o +endif -- Peter Oberparleiter Linux on System z Development - IBM Germany -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
On Fri, Aug 23, 2013 at 05:21:12PM +0200, Peter Oberparleiter wrote: On 23.08.2013 17:15, Peter Oberparleiter wrote: On 23.08.2013 10:39, Frantisek Hrbata wrote: Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. As promised, I'm also adding the patch that makes the format-specific part of gcov-kernel a loadable kernel module: --- kernel: gcov: make format-specific code loadable Turn the format-specific part of gcov-kernel into a loadable kernel module. This enables the use of gcov-kernel with kernel modules that were compiled with a version of GCC that produces a different gcov format when compared to the version of GCC that was used to compile the kernel. If I understand it correctly, this would mean that you will be able to use only one implementation of gcov format at the time. Meaning you will be able to get coverage data for module, but not for kernel if it was compiled with different gcc(gcda format). This is probably ok if you work only on your module, but I'm not sure this is generally the right approach. In this case I would probably rather see some support for more gcov formats at the same time(e.g. set of callback operations per gcov version). Again I'm probably missing something, but I still cannot see reason why to add such feature. If you want gcov support just compile your kernel and modules with the same gcc version(gcda format). But if this is really needed maybe it would be better to consider some parallel support for more gcov formats based on the gcov_info version. Would it be possible to add support for the modified gcc 4.7 gcov format and deal with this later? I can incorporate your changes: iter to use buffer, .init_array for modules and possibility to explicitly select the gcda format. In this case we will have at least the basic support in kernel. This is just me thinking out loud. Many thanks Peter! Signed-off-by: Peter Oberparleiter ober...@linux.vnet.ibm.com --- kernel/gcov/Kconfig | 19 +-- kernel/gcov/Makefile |7 --- 2 files changed, 21 insertions(+), 5 deletions(-) --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -29,8 +29,23 @@ config GCOV_KERNEL and: GCOV_PROFILE := n - Note that the debugfs filesystem has to be mounted to access - profiling data. + Note that GCOV_KERNEL_FS has to specified as well and the debugfs + filesystem has to be mounted to access profiling data. + +config GCOV_KERNEL_FS + tristate Provide gcov data files in debugfs + depends on GCOV_KERNEL + default y + ---help--- + Make profiling data available in debugfs at /sys/kernel/debug/gcov. + + Say M if you want to enable collecting coverage data for kernel modules + which are compiled using a gcc version different from the one that + was used to compile the kernel. In that case, re-compile the gcov + kernel module with corresponding format support and load that module + instead. + + If unsure, say Y. config GCOV_PROFILE_ALL bool Profile entire Kernel --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -13,10 +13,11 @@ else cc-ver := $(call cc-version) endif -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o +obj-$(CONFIG_GCOV_KERNEL) += base.o +obj-$(CONFIG_GCOV_KERNEL_FS) += gcov.o ifeq ($(call if-lt, $(cc-ver), 0407),1) - obj-$(CONFIG_GCOV_KERNEL) += gcc_3_4.o + gcov-objs += fs.o gcc_3_4.o else - obj-$(CONFIG_GCOV_KERNEL) += gcc_4_7.o + gcov-objs += fs.o gcc_4_7.o endif -- Peter Oberparleiter Linux on System z Development - IBM Germany -- Frantisek Hrbata -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 2/4] gcov: add support for gcc 4.7 gcov format
On Fri, Aug 23, 2013 at 05:12:19PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 10:39, Frantisek Hrbata wrote: > > The gcov in-memory format changed in gcc 4.7. The biggest change, which > > requires this special implementation, is that gcov_info no longer contains > > array of counters for each counter type for all functions and gcov_fn_info > > is > > not used for mapping of function's counters to these arrays(offset). Now > > each > > gcov_fn_info contans it's counters, which makes things a little bit easier. > > By now I've come to the conclusion that I may have over-engineered > gcov_iter_next in the original GCC 3.4 implementation. This became especially > apparent when someone tried to adjust that code to also work with a specific > android GCC version - adding a simple field to the output file format required > major complex changes. > > Therefore in my version of the GCC 4.7 support, I opted to replace this logic > with the simpler approach of generating the gcov data file in-memory during > open (gcov_iter_new) and reducing gcov_iter_next/gcov_iter_write to simple > copy-from-buffer functions. Yes, this seems reasonable and much easier approach to me, but we will end up with three copies of one gcov(gcda) file data in memory: data from gcc, our buffer and the buffer in seq file. But I guess this is not a big problem, unless someone opens a huge amount of gcda files in parallel. Generally I like this idea. This will be also probably faster then writing 1 or 2 ints per one iter write. > > Since I can see the need to change the format code again in the future, > I think it would be easier to simplify this part of the code correspondingly. > I'm adding my version of this part of the implementation as discussion input > below (the relevant part starts at convert_to_gcda()). I have really only two nits to the code(please see below). I spent some time checking the new seq/iter implementation and it seems ok to me. Generally I like how simple this change is done. > > --- > kernel: gcov: add support for gcc 4.7 gcov format > > GCC 4.7 changed the format of gcov related data structures, both > on-disk and in memory. Add support for the new format. > > Signed-off-by: Peter Oberparleiter > --- > kernel/gcov/Makefile |4 > kernel/gcov/gcc_4_7.c | 510 > ++ > 2 files changed, 513 insertions(+), 1 deletion(-) > > --- a/kernel/gcov/Makefile > +++ b/kernel/gcov/Makefile > @@ -1,3 +1,5 @@ > ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' > > -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o > +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o > +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) > +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) > --- /dev/null > +++ b/kernel/gcov/gcc_4_7.c > @@ -0,0 +1,510 @@ > +/* > + * This code provides functions to handle gcc's profiling data format > + * introduced with gcc 4.7. Future versions of gcc may change the gcov > + * format (as happened before), so all format-specific information needs > + * to be kept modular and easily exchangeable. > + * > + * This file is based on gcc-internal definitions. Functions and data > + * structures are defined to be compatible with gcc counterparts. > + * For a better understanding, refer to gcc source: gcc/gcov-io.h. > + * > + *Copyright IBM Corp. 2013 > + *Author(s): Peter Oberparleiter > + * > + *Uses gcc-internal data definitions. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include "gcov.h" > + > +/* > + * Profiling data types used for gcc 4.7 and above - these are defined by > + * gcc and need to be kept as close to the original definition as possible to > + * remain compatible. > + */ > +#define GCOV_COUNTERS8 > +#define GCOV_DATA_MAGIC ((unsigned int) 0x67636461) > +#define GCOV_TAG_FUNCTION((unsigned int) 0x0100) > +#define GCOV_TAG_COUNTER_BASE((unsigned int) 0x01a1) > +#define GCOV_TAG_FOR_COUNTER(count) \ > + (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17)) > + > +#if BITS_PER_LONG >= 64 > +typedef long gcov_type; > +#else > +typedef long long gcov_type; > +#endif > + > +/** > + * struct gcov_ctr_info - profiling data per function and counter type > + * @num: number of counter values for this type > + * @values: array of counter values for this type > + * > + * This data is generated by gcc during compilation and doesn't change > + * at run-time with the exception of the values ar
Re: [RFC PATCH 4/4] kernel: add support for init_array constructors
On Fri, Aug 23, 2013 at 05:13:31PM +0200, Peter Oberparleiter wrote:
> On 23.08.2013 10:39, Frantisek Hrbata wrote:
> > This adds the .init_array section as yet another section with constructors.
> > This
> > is needed because gcc is adding __gcov_init calls to .init_array.
> >
> > Signed-off-by: Frantisek Hrbata
> > ---
> > include/asm-generic/vmlinux.lds.h | 1 +
> > include/linux/module.h| 2 ++
> > kernel/module.c | 6 ++
> > 3 files changed, 9 insertions(+)
> >
> > diff --git a/include/asm-generic/vmlinux.lds.h
> > b/include/asm-generic/vmlinux.lds.h
> > index 69732d2..c55d8d9 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -468,6 +468,7 @@
> > #define KERNEL_CTORS() . = ALIGN(8); \
> > VMLINUX_SYMBOL(__ctors_start) = .; \
> > *(.ctors) \
> > + *(.init_array) \
>
> This is exactly how I (would) have done it.
>
> > VMLINUX_SYMBOL(__ctors_end) = .;
> > #else
> > #define KERNEL_CTORS()
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 46f1ea0..89ff829 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -374,6 +374,8 @@ struct module
> > /* Constructor functions. */
> > ctor_fn_t *ctors;
> > unsigned int num_ctors;
> > + ctor_fn_t *init_array;
> > + unsigned int num_init_array;
>
> I think it would be safe to re-use the ctors-related fields here. Each GCC
> version will only ever create either of .ctors or .init_array, but never
> both.
> The only case where separate fields would be required, would be when
> linking a kernel module from multiple object files compiled with different
> versions of GCC.
Ah ok, I was not aware of this.
>
> > #endif
> > };
> > #ifndef MODULE_ARCH_INIT
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 2069158..581ae89 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod,
> > struct load_info *info)
> > #ifdef CONFIG_CONSTRUCTORS
> > mod->ctors = section_objs(info, ".ctors",
> > sizeof(*mod->ctors), >num_ctors);
>
> When reusing mod->ctors, you could check for mod->ctors == NULL here and
> fill in the values from section_objs(".init_array").
Yes, in the case that we only have .ctors or .init_array section, this sounds
as the right approach.
>
> > + mod->init_array = section_objs(info, ".init_array",
> > + sizeof(*mod->init_array),
> > + >num_init_array);
> > #endif
> >
> > #ifdef CONFIG_TRACEPOINTS
> > @@ -3024,6 +3027,9 @@ static void do_mod_ctors(struct module *mod)
> >
> > for (i = 0; i < mod->num_ctors; i++)
> > mod->ctors[i]();
> > +
> > + for (i = 0; i < mod->num_init_array; i++)
> > + mod->init_array[i]();
> > #endif
> > }
> >
>
> --
> Peter Oberparleiter
> Linux on System z Development - IBM Germany
>
--
Frantisek Hrbata
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/4] gcov: move gcov structs definitions to a gcc version specific file
On Fri, Aug 23, 2013 at 05:09:58PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 10:39, Frantisek Hrbata wrote: > > Since also the gcov structures(gcov_info, gcov_fn_info, gcov_ctr_info) can > > change between gcc releases, as shown in gcc 4.7, they cannot be defined in > > a > > common header and need to be moved to a specific gcc implemention file. This > > also requires to make the gcov_info structure opaque for the common code > > and to > > introduce simple helpers for accessing data inside gcov_info. > > I've taken a similar approach in my version, although I stopped at isolating > the code that handles the linked list. I like your version better since it's > more consistent. :) I also have doubts with the list "abstraction", it isn't very nice. I tried to keep the changes as simple as possible in the generic code. I'm not sayint it is the right approach, but your design is pretty good, so I had no urges to change it more deeper. I'm of course open to any suggestions. > > > diff --git a/kernel/gcov/gcc_3_4.c b/kernel/gcov/gcc_3_4.c > > index ae5bb42..27bc88a 100644 > > --- a/kernel/gcov/gcc_3_4.c > > +++ b/kernel/gcov/gcc_3_4.c > > @@ -21,6 +21,121 @@ > > #include > > #include "gcov.h" > > > > +#define GCOV_COUNTERS 5 > > The value for GCOV_COUNTERS has been changed with GCC 4.3. Before it was 5, > starting with GCC 4.3 the value is 8. While this is not strictly necessary, > I'm > wondering if this should be added here to prevent any unwanted side-effects. Yes I was thinking about this two. My first idea was to use some define, maybe in the Makefile during the gcc version check and set the number of counters properly later based on this define. Something like #if GCOV_GCC_VERIONS >= 0430 #define GCOV_COUNTERS 8 #elif ... for the gcc_3_4.c implementation. But I'm not sure what the new counters are good for and if they are really needed for the coverage info. This would require deeper understanding what and how the types of counters are used. At this point a simply did not change the value for the format before gcc 4.7, because each counter type has a tag and this should be backward compatible. We only miss the new counters. Again this is something that probably deserves more attention. Thanks for pointing this out! > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/4] add support for gcov format introduced in gcc 4.7
On Fri, Aug 23, 2013 at 05:08:07PM +0200, Peter Oberparleiter wrote: > On 23.08.2013 10:39, Frantisek Hrbata wrote: > > This is an attempt to bring support for modified gcov format in gcc 4.7 to > > the kernel. It tries to leverage the existing layout/abstraction, which was > > designed keeping in mind that the gcov format could change, but some > > changes had > > to be make. Mostly because the current model does not take into account that > > even the core gcov structures, like gcov_info, could change. One part that > > could > > be problematic is the addition of the .init_array section for constructors. > > It appears that gcc 4.7 support for gcov-kernel is quite important to a > number of people, at least that is what I derive from the fact that I > now know of 3 people who've been working on this support separately from > each other: you, myself (I've been close to posting my own version to > LKML) and Christophe Guillon. First, thank you for your quick reply. Second, great, I was worried there is no interest to have the new format supported, because it's quite some time gcc 4.7 is out. > > It's apparent now that I made a mistake delaying the discussion of the > effort for too long, but I think your posting the patches opens up a > good opportunity to combine the best of all previous efforts. To be honest I ran into this problem 14 days ago when I was asked to take a look. So I cannot say I know everything about gcov format and stuff around it. I mostly followed your original code after I got some idea of the gcov in-memory layout. Meaning, I'm glad that there is also yours and Christophe's code and I for sure welcome the "combine the best of all previous efforts" approach, even if it means dropping my code :). > > Most of your code looks very familiar. There's one feature missing though > that Christophe brought up as a requirement: the ability for gcov-kernel > to cope with kernel modules being compiled with GCC versions implementing > a different gcov format (apparently this can happen in some embedded > setups). Here follows quote from the gcc/gcov-io.h file Coverage information is held in two files. A notes file, which is generated by the compiler, and a data file, which is generated by the program under test. Both files use a similar structure. We do not attempt to make these files backwards compatible with previous versions, as you only need coverage information when developing a program. We do hold version information, so that mismatches can be detected, and we use a format that allows tools to skip information they do not understand or are not interested in. Also from my experience, I would expect that the gcov will be used during development, meaning re-compilation isn't a big problem here. I for sure can be missing something and I'm by no means saying it wouldn't be useful feature. Just that it would complite things a little bit. > > Christophe proposed run-time version checking and a file-ops type function > table which is chosen based on info->version. I found this approach > somewhat intrusive and this would also not have covered the case where a > new GCC versions was used to compile kernel modules for which the base > kernel has no support. I tried to solve this requirement by combining > two changes: > > 1) make the gcov-format generated by gcov-kernel compile-time configurable > 2) separate the gcov-format specific code into a loadable kernel module So if I understand it correctly you would separate the input format(gcov_info) from the output(gcda files). Meaning no matter which gcc compiled the module you can select the gcda format. And even if the kernel does not know the new format you can simply compile a module supporting it, instead of the whole kernel. Can you please give me an example why this is needed? As I wrote I'm not that familiar with gcov and I'm probably missing something, but I do not understand why this(handling several versions at runtime, not only the one used by gcc during compilation) is useful(note my comment above about the gcov used during development). > > This way, the gcov-format specific part of gcov-kernel could be replaced > when working with a different version GCC. I'll post the corresponding > patches as reply in another mail. Great, I will take a look, but it may take me some time :). > > Back to your patches: I tested them and they work fine on s390x when > compiled with GCC 4.3.4 and 4.7.2. I'll provide some more specific > comments as replies to your patch-mails. Many thanks! > > > Regards, > Peter Oberparleiter > > -- > Peter Oberparleiter > Linux on System z Development - IBM Germany > -- Frantisek Hrbata -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 4/4] kernel: add support for init_array constructors
This adds the .init_array section as yet another section with constructors. This is needed because gcc is adding __gcov_init calls to .init_array. Signed-off-by: Frantisek Hrbata --- include/asm-generic/vmlinux.lds.h | 1 + include/linux/module.h| 2 ++ kernel/module.c | 6 ++ 3 files changed, 9 insertions(+) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 69732d2..c55d8d9 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -468,6 +468,7 @@ #define KERNEL_CTORS() . = ALIGN(8); \ VMLINUX_SYMBOL(__ctors_start) = .; \ *(.ctors) \ + *(.init_array) \ VMLINUX_SYMBOL(__ctors_end) = .; #else #define KERNEL_CTORS() diff --git a/include/linux/module.h b/include/linux/module.h index 46f1ea0..89ff829 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -374,6 +374,8 @@ struct module /* Constructor functions. */ ctor_fn_t *ctors; unsigned int num_ctors; + ctor_fn_t *init_array; + unsigned int num_init_array; #endif }; #ifndef MODULE_ARCH_INIT diff --git a/kernel/module.c b/kernel/module.c index 2069158..581ae89 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, struct load_info *info) #ifdef CONFIG_CONSTRUCTORS mod->ctors = section_objs(info, ".ctors", sizeof(*mod->ctors), >num_ctors); + mod->init_array = section_objs(info, ".init_array", + sizeof(*mod->init_array), + >num_init_array); #endif #ifdef CONFIG_TRACEPOINTS @@ -3024,6 +3027,9 @@ static void do_mod_ctors(struct module *mod) for (i = 0; i < mod->num_ctors; i++) mod->ctors[i](); + + for (i = 0; i < mod->num_init_array; i++) + mod->init_array[i](); #endif } -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. Signed-off-by: Frantisek Hrbata --- kernel/gcov/Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index e97ca59..d57b712 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,3 +1,6 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o + +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 2/4] gcov: add support for gcc 4.7 gcov format
The gcov in-memory format changed in gcc 4.7. The biggest change, which
requires this special implementation, is that gcov_info no longer contains
array of counters for each counter type for all functions and gcov_fn_info is
not used for mapping of function's counters to these arrays(offset). Now each
gcov_fn_info contans it's counters, which makes things a little bit easier.
This is heavily based on the previous gcc_3_4.c implementation.
Signed-off-by: Frantisek Hrbata
---
kernel/gcov/base.c| 6 +
kernel/gcov/gcc_4_7.c | 612 ++
2 files changed, 618 insertions(+)
create mode 100644 kernel/gcov/gcc_4_7.c
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 912576a..f45b75b 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -79,6 +79,12 @@ void __gcov_merge_delta(gcov_type *counters, unsigned int
n_counters)
}
EXPORT_SYMBOL(__gcov_merge_delta);
+void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_ior);
+
/**
* gcov_enable_events - enable event reporting through gcov_event()
*
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
new file mode 100644
index 000..31bee00
--- /dev/null
+++ b/kernel/gcov/gcc_4_7.c
@@ -0,0 +1,612 @@
+/*
+ * This code provides functions to handle gcc's profiling data format
+ * introduced with gcc 4.7.
+ *
+ * This file is based heavily on gcc_3_4.c file.
+ *
+ * For a better understanding, refer to gcc source:
+ * gcc/gcov-io.h
+ * libgcc/libgcov.c
+ *
+ * Uses gcc-internal data definitions.
+ */
+
+#include
+#include
+#include
+#include
+#include "gcov.h"
+
+#define GCOV_COUNTERS 8
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+static struct gcov_info *gcov_info_head;
+
+/**
+ * struct gcov_ctr_info - information about counters for a single function
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info {
+ unsigned int num;
+ gcov_type *values;
+};
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @key: comdat key
+ * @ident: unique ident of function
+ * @lineno_checksum: function lineo_checksum
+ * @cfg_checksum: function cfg checksum
+ * @ctrs: instrumented counters
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ *
+ * Information about a single function. This uses the trailing array
+ * idiom. The number of counters is determined from the merge pointer
+ * array in gcov_info. The key is used to detect which of a set of
+ * comdat functions was selected -- it points to the gcov_info object
+ * of the object file containing the selected comdat function.
+ */
+struct gcov_fn_info {
+ const struct gcov_info *key;
+ unsigned int ident;
+ unsigned int lineno_checksum;
+ unsigned int cfg_checksum;
+ struct gcov_ctr_info ctrs[0];
+};
+
+/**
+ * struct gcov_info - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: uniquifying time stamp
+ * @filename: name of the associated gcov data file
+ * @merge: merge functions (null for unused counter type)
+ * @n_functions: number of instrumented functions
+ * @functions: pointer to pointers to function information
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info {
+ unsigned int version;
+ struct gcov_info *next;
+ unsigned int stamp;
+ const char *filename;
+ void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int);
+ unsigned int n_functions;
+ const struct gcov_fn_info *const *functions;
+};
+
+/**
+ * gcov_info_filename - return info filename
+ * @info: profiling data set
+ */
+const char *gcov_info_filename(struct gcov_info *info)
+{
+ return info->filename;
+}
+
+/**
+ * gcov_info_version - return info version
+ * @info: profiling data set
+ */
+unsigned int gcov_info_version(struct gcov_info *info)
+{
+ return info->version;
+}
+
+/**
+ * gcov_info_next - return next profiling data set
+ * @info: profiling data set
+ *
+ * Returns next gcov_info following @info or first gcov_info in the chain if
+ * @info is %NULL.
+ */
+struct gcov_info *gcov_info_next(struct gcov_info *info)
+{
+ if (!info)
+ return gcov_info_head;
+
+ return info->next;
+}
+
+/**
+ * gcov_info_link - link/add profiling data set to the list
+ * @info: profiling data set
+ */
+void gcov_info_link(struct gcov_info *info)
+{
+ info->next = gcov_info_head;
+ gcov_info_head = info;
+}
+
+/**
+ * gcov_info_unlink - unlink/remove profiling
[RFC PATCH 1/4] gcov: move gcov structs definitions to a gcc version specific file
Since also the gcov structures(gcov_info, gcov_fn_info, gcov_ctr_info) can
change between gcc releases, as shown in gcc 4.7, they cannot be defined in a
common header and need to be moved to a specific gcc implemention file. This
also requires to make the gcov_info structure opaque for the common code and to
introduce simple helpers for accessing data inside gcov_info.
Signed-off-by: Frantisek Hrbata
---
kernel/gcov/base.c| 26 ++--
kernel/gcov/fs.c | 27 ++--
kernel/gcov/gcc_3_4.c | 115 ++
kernel/gcov/gcov.h| 65 +---
4 files changed, 153 insertions(+), 80 deletions(-)
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9b22d03..912576a 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -20,7 +20,6 @@
#include
#include "gcov.h"
-static struct gcov_info *gcov_info_head;
static int gcov_events_enabled;
static DEFINE_MUTEX(gcov_lock);
@@ -34,7 +33,7 @@ void __gcov_init(struct gcov_info *info)
mutex_lock(_lock);
if (gcov_version == 0) {
- gcov_version = info->version;
+ gcov_version = gcov_info_version(info);
/*
* Printing gcc's version magic may prove useful for debugging
* incompatibility reports.
@@ -45,8 +44,7 @@ void __gcov_init(struct gcov_info *info)
* Add new profiling data structure to list and inform event
* listener.
*/
- info->next = gcov_info_head;
- gcov_info_head = info;
+ gcov_info_link(info);
if (gcov_events_enabled)
gcov_event(GCOV_ADD, info);
mutex_unlock(_lock);
@@ -91,13 +89,15 @@ EXPORT_SYMBOL(__gcov_merge_delta);
*/
void gcov_enable_events(void)
{
- struct gcov_info *info;
+ struct gcov_info *info = NULL;
mutex_lock(_lock);
gcov_events_enabled = 1;
+
/* Perform event callback for previously registered entries. */
- for (info = gcov_info_head; info; info = info->next)
+ while ((info = gcov_info_next(info)))
gcov_event(GCOV_ADD, info);
+
mutex_unlock(_lock);
}
@@ -112,25 +112,23 @@ static int gcov_module_notifier(struct notifier_block
*nb, unsigned long event,
void *data)
{
struct module *mod = data;
- struct gcov_info *info;
- struct gcov_info *prev;
+ struct gcov_info *info = NULL;
+ struct gcov_info *prev = NULL;
if (event != MODULE_STATE_GOING)
return NOTIFY_OK;
mutex_lock(_lock);
- prev = NULL;
+
/* Remove entries located in module from linked list. */
- for (info = gcov_info_head; info; info = info->next) {
+ while ((info = gcov_info_next(info))) {
if (within(info, mod->module_core, mod->core_size)) {
- if (prev)
- prev->next = info->next;
- else
- gcov_info_head = info->next;
+ gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
} else
prev = info;
}
+
mutex_unlock(_lock);
return NOTIFY_OK;
diff --git a/kernel/gcov/fs.c b/kernel/gcov/fs.c
index 9bd0934..27e12ce 100644
--- a/kernel/gcov/fs.c
+++ b/kernel/gcov/fs.c
@@ -242,7 +242,7 @@ static struct gcov_node *get_node_by_name(const char *name)
list_for_each_entry(node, _head, all) {
info = get_node_info(node);
- if (info && (strcmp(info->filename, name) == 0))
+ if (info && (strcmp(gcov_info_filename(info), name) == 0))
return node;
}
@@ -279,7 +279,7 @@ static ssize_t gcov_seq_write(struct file *file, const char
__user *addr,
seq = file->private_data;
info = gcov_iter_get_info(seq->private);
mutex_lock(_lock);
- node = get_node_by_name(info->filename);
+ node = get_node_by_name(gcov_info_filename(info));
if (node) {
/* Reset counts or remove node for unloaded modules. */
if (node->num_loaded == 0)
@@ -376,8 +376,9 @@ static void add_links(struct gcov_node *node, struct dentry
*parent)
if (!node->links)
return;
for (i = 0; i < num; i++) {
- target = get_link_target(get_node_info(node)->filename,
-_link[i]);
+ target = get_link_target(
+ gcov_info_filename(get_node_info(node)),
+ _link[i]);
if (!target)
goto out_err;
basename = strrchr(target, '/');
@@ -576,7 +577,7 @@ static void add_
[RFC PATCH 0/4] add support for gcov format introduced in gcc 4.7
This is an attempt to bring support for modified gcov format in gcc 4.7 to the kernel. It tries to leverage the existing layout/abstraction, which was designed keeping in mind that the gcov format could change, but some changes had to be make. Mostly because the current model does not take into account that even the core gcov structures, like gcov_info, could change. One part that could be problematic is the addition of the .init_array section for constructors. Tested with lcov and seems to be working fine, giving similar results as for the older format. Frantisek Hrbata (4): gcov: move gcov structs definitions to a gcc version specific file gcov: add support for gcc 4.7 gcov format gcov: compile specific gcov implementation based on gcc version kernel: add support for init_array constructors include/asm-generic/vmlinux.lds.h | 1 + include/linux/module.h| 2 + kernel/gcov/Makefile | 5 +- kernel/gcov/base.c| 32 +- kernel/gcov/fs.c | 27 +- kernel/gcov/gcc_3_4.c | 115 +++ kernel/gcov/gcc_4_7.c | 612 ++ kernel/gcov/gcov.h| 65 +--- kernel/module.c | 6 + 9 files changed, 784 insertions(+), 81 deletions(-) create mode 100644 kernel/gcov/gcc_4_7.c -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 0/4] add support for gcov format introduced in gcc 4.7
This is an attempt to bring support for modified gcov format in gcc 4.7 to the kernel. It tries to leverage the existing layout/abstraction, which was designed keeping in mind that the gcov format could change, but some changes had to be make. Mostly because the current model does not take into account that even the core gcov structures, like gcov_info, could change. One part that could be problematic is the addition of the .init_array section for constructors. Tested with lcov and seems to be working fine, giving similar results as for the older format. Frantisek Hrbata (4): gcov: move gcov structs definitions to a gcc version specific file gcov: add support for gcc 4.7 gcov format gcov: compile specific gcov implementation based on gcc version kernel: add support for init_array constructors include/asm-generic/vmlinux.lds.h | 1 + include/linux/module.h| 2 + kernel/gcov/Makefile | 5 +- kernel/gcov/base.c| 32 +- kernel/gcov/fs.c | 27 +- kernel/gcov/gcc_3_4.c | 115 +++ kernel/gcov/gcc_4_7.c | 612 ++ kernel/gcov/gcov.h| 65 +--- kernel/module.c | 6 + 9 files changed, 784 insertions(+), 81 deletions(-) create mode 100644 kernel/gcov/gcc_4_7.c -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 1/4] gcov: move gcov structs definitions to a gcc version specific file
Since also the gcov structures(gcov_info, gcov_fn_info, gcov_ctr_info) can
change between gcc releases, as shown in gcc 4.7, they cannot be defined in a
common header and need to be moved to a specific gcc implemention file. This
also requires to make the gcov_info structure opaque for the common code and to
introduce simple helpers for accessing data inside gcov_info.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
kernel/gcov/base.c| 26 ++--
kernel/gcov/fs.c | 27 ++--
kernel/gcov/gcc_3_4.c | 115 ++
kernel/gcov/gcov.h| 65 +---
4 files changed, 153 insertions(+), 80 deletions(-)
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9b22d03..912576a 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -20,7 +20,6 @@
#include linux/mutex.h
#include gcov.h
-static struct gcov_info *gcov_info_head;
static int gcov_events_enabled;
static DEFINE_MUTEX(gcov_lock);
@@ -34,7 +33,7 @@ void __gcov_init(struct gcov_info *info)
mutex_lock(gcov_lock);
if (gcov_version == 0) {
- gcov_version = info-version;
+ gcov_version = gcov_info_version(info);
/*
* Printing gcc's version magic may prove useful for debugging
* incompatibility reports.
@@ -45,8 +44,7 @@ void __gcov_init(struct gcov_info *info)
* Add new profiling data structure to list and inform event
* listener.
*/
- info-next = gcov_info_head;
- gcov_info_head = info;
+ gcov_info_link(info);
if (gcov_events_enabled)
gcov_event(GCOV_ADD, info);
mutex_unlock(gcov_lock);
@@ -91,13 +89,15 @@ EXPORT_SYMBOL(__gcov_merge_delta);
*/
void gcov_enable_events(void)
{
- struct gcov_info *info;
+ struct gcov_info *info = NULL;
mutex_lock(gcov_lock);
gcov_events_enabled = 1;
+
/* Perform event callback for previously registered entries. */
- for (info = gcov_info_head; info; info = info-next)
+ while ((info = gcov_info_next(info)))
gcov_event(GCOV_ADD, info);
+
mutex_unlock(gcov_lock);
}
@@ -112,25 +112,23 @@ static int gcov_module_notifier(struct notifier_block
*nb, unsigned long event,
void *data)
{
struct module *mod = data;
- struct gcov_info *info;
- struct gcov_info *prev;
+ struct gcov_info *info = NULL;
+ struct gcov_info *prev = NULL;
if (event != MODULE_STATE_GOING)
return NOTIFY_OK;
mutex_lock(gcov_lock);
- prev = NULL;
+
/* Remove entries located in module from linked list. */
- for (info = gcov_info_head; info; info = info-next) {
+ while ((info = gcov_info_next(info))) {
if (within(info, mod-module_core, mod-core_size)) {
- if (prev)
- prev-next = info-next;
- else
- gcov_info_head = info-next;
+ gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
} else
prev = info;
}
+
mutex_unlock(gcov_lock);
return NOTIFY_OK;
diff --git a/kernel/gcov/fs.c b/kernel/gcov/fs.c
index 9bd0934..27e12ce 100644
--- a/kernel/gcov/fs.c
+++ b/kernel/gcov/fs.c
@@ -242,7 +242,7 @@ static struct gcov_node *get_node_by_name(const char *name)
list_for_each_entry(node, all_head, all) {
info = get_node_info(node);
- if (info (strcmp(info-filename, name) == 0))
+ if (info (strcmp(gcov_info_filename(info), name) == 0))
return node;
}
@@ -279,7 +279,7 @@ static ssize_t gcov_seq_write(struct file *file, const char
__user *addr,
seq = file-private_data;
info = gcov_iter_get_info(seq-private);
mutex_lock(node_lock);
- node = get_node_by_name(info-filename);
+ node = get_node_by_name(gcov_info_filename(info));
if (node) {
/* Reset counts or remove node for unloaded modules. */
if (node-num_loaded == 0)
@@ -376,8 +376,9 @@ static void add_links(struct gcov_node *node, struct dentry
*parent)
if (!node-links)
return;
for (i = 0; i num; i++) {
- target = get_link_target(get_node_info(node)-filename,
-gcov_link[i]);
+ target = get_link_target(
+ gcov_info_filename(get_node_info(node)),
+ gcov_link[i]);
if (!target)
goto out_err;
basename = strrchr(target, '/');
@@ -576,7 +577,7 @@ static void add_node(struct gcov_info *info
[RFC PATCH 2/4] gcov: add support for gcc 4.7 gcov format
The gcov in-memory format changed in gcc 4.7. The biggest change, which
requires this special implementation, is that gcov_info no longer contains
array of counters for each counter type for all functions and gcov_fn_info is
not used for mapping of function's counters to these arrays(offset). Now each
gcov_fn_info contans it's counters, which makes things a little bit easier.
This is heavily based on the previous gcc_3_4.c implementation.
Signed-off-by: Frantisek Hrbata fhrb...@redhat.com
---
kernel/gcov/base.c| 6 +
kernel/gcov/gcc_4_7.c | 612 ++
2 files changed, 618 insertions(+)
create mode 100644 kernel/gcov/gcc_4_7.c
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 912576a..f45b75b 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -79,6 +79,12 @@ void __gcov_merge_delta(gcov_type *counters, unsigned int
n_counters)
}
EXPORT_SYMBOL(__gcov_merge_delta);
+void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_ior);
+
/**
* gcov_enable_events - enable event reporting through gcov_event()
*
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
new file mode 100644
index 000..31bee00
--- /dev/null
+++ b/kernel/gcov/gcc_4_7.c
@@ -0,0 +1,612 @@
+/*
+ * This code provides functions to handle gcc's profiling data format
+ * introduced with gcc 4.7.
+ *
+ * This file is based heavily on gcc_3_4.c file.
+ *
+ * For a better understanding, refer to gcc source:
+ * gcc/gcov-io.h
+ * libgcc/libgcov.c
+ *
+ * Uses gcc-internal data definitions.
+ */
+
+#include linux/errno.h
+#include linux/slab.h
+#include linux/string.h
+#include linux/seq_file.h
+#include gcov.h
+
+#define GCOV_COUNTERS 8
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+static struct gcov_info *gcov_info_head;
+
+/**
+ * struct gcov_ctr_info - information about counters for a single function
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info {
+ unsigned int num;
+ gcov_type *values;
+};
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @key: comdat key
+ * @ident: unique ident of function
+ * @lineno_checksum: function lineo_checksum
+ * @cfg_checksum: function cfg checksum
+ * @ctrs: instrumented counters
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ *
+ * Information about a single function. This uses the trailing array
+ * idiom. The number of counters is determined from the merge pointer
+ * array in gcov_info. The key is used to detect which of a set of
+ * comdat functions was selected -- it points to the gcov_info object
+ * of the object file containing the selected comdat function.
+ */
+struct gcov_fn_info {
+ const struct gcov_info *key;
+ unsigned int ident;
+ unsigned int lineno_checksum;
+ unsigned int cfg_checksum;
+ struct gcov_ctr_info ctrs[0];
+};
+
+/**
+ * struct gcov_info - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: uniquifying time stamp
+ * @filename: name of the associated gcov data file
+ * @merge: merge functions (null for unused counter type)
+ * @n_functions: number of instrumented functions
+ * @functions: pointer to pointers to function information
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info {
+ unsigned int version;
+ struct gcov_info *next;
+ unsigned int stamp;
+ const char *filename;
+ void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int);
+ unsigned int n_functions;
+ const struct gcov_fn_info *const *functions;
+};
+
+/**
+ * gcov_info_filename - return info filename
+ * @info: profiling data set
+ */
+const char *gcov_info_filename(struct gcov_info *info)
+{
+ return info-filename;
+}
+
+/**
+ * gcov_info_version - return info version
+ * @info: profiling data set
+ */
+unsigned int gcov_info_version(struct gcov_info *info)
+{
+ return info-version;
+}
+
+/**
+ * gcov_info_next - return next profiling data set
+ * @info: profiling data set
+ *
+ * Returns next gcov_info following @info or first gcov_info in the chain if
+ * @info is %NULL.
+ */
+struct gcov_info *gcov_info_next(struct gcov_info *info)
+{
+ if (!info)
+ return gcov_info_head;
+
+ return info-next;
+}
+
+/**
+ * gcov_info_link - link/add profiling data set to the list
+ * @info: profiling data set
+ */
+void gcov_info_link(struct gcov_info *info)
+{
+ info-next = gcov_info_head;
+ gcov_info_head = info
[RFC PATCH 3/4] gcov: compile specific gcov implementation based on gcc version
Compile the correct gcov implementation file for a specific gcc version. In the future, if another file is added, the conditions will need to be somehow adjusted to if-elif-else case, but at this point the simple cc-ifversion should be enough. Signed-off-by: Frantisek Hrbata fhrb...@redhat.com --- kernel/gcov/Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index e97ca59..d57b712 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -1,3 +1,6 @@ ccflags-y := -DSRCTREE='$(srctree)' -DOBJTREE='$(objtree)' -obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o gcc_3_4.o +obj-$(CONFIG_GCOV_KERNEL) := base.o fs.o + +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -ge, 0407, gcc_4_7.o) +obj-$(CONFIG_GCOV_KERNEL) += $(call cc-ifversion, -lt, 0407, gcc_3_4.o) -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 4/4] kernel: add support for init_array constructors
This adds the .init_array section as yet another section with constructors. This is needed because gcc is adding __gcov_init calls to .init_array. Signed-off-by: Frantisek Hrbata fhrb...@redhat.com --- include/asm-generic/vmlinux.lds.h | 1 + include/linux/module.h| 2 ++ kernel/module.c | 6 ++ 3 files changed, 9 insertions(+) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 69732d2..c55d8d9 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -468,6 +468,7 @@ #define KERNEL_CTORS() . = ALIGN(8); \ VMLINUX_SYMBOL(__ctors_start) = .; \ *(.ctors) \ + *(.init_array) \ VMLINUX_SYMBOL(__ctors_end) = .; #else #define KERNEL_CTORS() diff --git a/include/linux/module.h b/include/linux/module.h index 46f1ea0..89ff829 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -374,6 +374,8 @@ struct module /* Constructor functions. */ ctor_fn_t *ctors; unsigned int num_ctors; + ctor_fn_t *init_array; + unsigned int num_init_array; #endif }; #ifndef MODULE_ARCH_INIT diff --git a/kernel/module.c b/kernel/module.c index 2069158..581ae89 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2760,6 +2760,9 @@ static void find_module_sections(struct module *mod, struct load_info *info) #ifdef CONFIG_CONSTRUCTORS mod-ctors = section_objs(info, .ctors, sizeof(*mod-ctors), mod-num_ctors); + mod-init_array = section_objs(info, .init_array, + sizeof(*mod-init_array), + mod-num_init_array); #endif #ifdef CONFIG_TRACEPOINTS @@ -3024,6 +3027,9 @@ static void do_mod_ctors(struct module *mod) for (i = 0; i mod-num_ctors; i++) mod-ctors[i](); + + for (i = 0; i mod-num_init_array; i++) + mod-init_array[i](); #endif } -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
