Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221006 > unrolled thread

Re: [PATCH] ARM: fix alignement of __bug_table section entries

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-08 19:10 +0200
    Re: [PATCH] ARM: fix alignement of __bug_table section entries Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-08 22:10 +0200
      Re: [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-08 23:00 +0200
      Re: [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-10 01:20 +0200
        Re: [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-10 21:10 +0200
          Re: [PATCH] ARM: fix alignement of __bug_table section entries Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-10 21:20 +0200
            Re: [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-10 23:00 +0200
              Re: [PATCH] ARM: fix alignement of __bug_table section entries Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-11 12:00 +0200
                [PATCH 2/2] ARM: domains: add memory dependencies to  get_domain/set_domain Russell King <rmk+kernel@arm.linux.org.uk> - 2015-09-11 12:00 +0200
                  Re: [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-11 17:10 +0200
                    Re: [PATCH 2/2] ARM: domains: add memory dependencies to  get_domain/set_domain Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-11 17:20 +0200
                      Re: [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-11 17:50 +0200
                [PATCH 1/2] ARM: domains: thread_info.h no longer needs asm/domains.h Russell King <rmk+kernel@arm.linux.org.uk> - 2015-09-11 12:00 +0200

#1221006 — Re: [PATCH] ARM: fix alignement of __bug_table section entries

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

>> Gah, silly me. But even with [1], I still get an error [2]. I have a
>> confirmation that I have a "Page Permission" fault on the
>> probe_kernel_address().
>
> Hmm, that's not right.  If it's the DACR, then it should be a page domain
> fault, not a page permission fault.
>
>> [2] Oops
>> ========
>> # insmod /tmp/unalign.ko 
>> RJK1: fsr=23 far=e1c23643 dacr=51
>> RJK2: fsr=23 far=e1c23643 dacr=51
>> RJK3: fsr=2f far=bf00202c dacr=51
>> RJK: fault=4 instr=0x00000000 instrptr=bf00202c
>
> Can you add a show_pte(current->mm, instrptr) to dump those page
> table entries please?
Most certainly, here we go :

# insmod /tmp/unalign.ko 
RJK1: fsr=23 far=e1c1f743 dacr=51
RJK2: fsr=23 far=e1c1f743 dacr=51
pgd = e1cc4000
[bf00202c] *pgd=c1cab851, *pte=c1cb504f, *ppte=c1cb501f
RJK3: fsr=2f far=bf00202c dacr=51
RJK4: fault=4 instr=0x00000000 instrptr=bf00202c
pgd = e1cc4000
[bf00202c] *pgd=c1cab851, *pte=c1cb504f, *ppte=c1cb501f

Unable to handle kernel paging request at virtual address e1c1f743
pgd = e1cc4000
[e1c1f743] *pgd=c1c0044e(bad)
Internal error: Oops: 823 [#1] ARM
Modules linked in: unalign(+)
CPU: 0 PID: 608 Comm: insmod Not tainted 4.2.0-rc8-next-20150828-cm-x300+ #926
Hardware name: CM-X300 module
task: e1c68380 ti: e1c84000 task.ti: e1c84000
PC is at u_init+0x2c/0x40 [unalign]
LR is at u_init+0x14/0x40 [unalign]
pc : [<bf00202c>]    lr : [<bf002014>]    psr: a0000013
sp : e1c85df8  ip : e1c1f700  fp : 1e3e041c
r10: e1c1fc00  r9 : 00000001  r8 : 00000000
r7 : bf002000  r6 : e1cad660  r5 : c0b85b80  r4 : c0b85b80
r3 : e1c1f740  r2 : 00000004  r1 : a0000013  r0 : 00000000
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 0000397f  Table: c1cc4018  DAC: 00000051
Process insmod (pid: 608, stack limit = 0xe1c84198)

It happens on both mioa701(pxa270) and cm-x300(pxa310), with the same
cross-compiler+host and kernel source.
Yet doesn't happen on zylonite(pxa310), but different cross-compiler+host.

I'll try to have a single kernel (binary) tried over the cm-x300 and zylonite to
cross-check.

Cheers.

-- 
Robert

PS: unalign.ko is a module which does a p=kmalloc(4096), then dereferences
    *(p+3)

[1] Personal memo: memory pagetables
====================================
# cat /sys/kernel/debug/kernel_page_tables 
---[ Modules ]---
0xbf000000-0xbf001000           4K     RW x      MEM/CACHED/WBRA
0xbf002000-0xbf003000           4K     RW x      MEM/CACHED/WBRA
---[ Kernel Mapping ]---
0xc0000000-0xc4000000          64M     RW x     
0xe0000000-0xe4000000          64M     RW x     
---[ vmalloc() Area ]---
0xe4804000-0xe4844000         256K     RW NX     SO/UNCACHED
0xe4845000-0xe4850000          44K     RW NX     MEM/CACHED/WBRA
0xe485a000-0xe485b000           4K     RW NX SHD DEV/SHARED
0xe485c000-0xe485d000           4K     RW NX SHD DEV/SHARED
0xe485e000-0xe485f000           4K     RW NX     SO/UNCACHED
0xe4860000-0xe4870000          64K     RW NX SHD DEV/SHARED
0xe487a000-0xe487d000          12K     RW NX     MEM/CACHED/WBRA
0xe4880000-0xe48c0000         256K     RW NX SHD DEV/SHARED
0xe48c1000-0xe4903000         264K     RW NX     MEM/CACHED/WBRA
0xe4904000-0xe491e000         104K     RW NX     SO/UNCACHED
0xe49a0000-0xe49b0000          64K     RW NX SHD DEV/SHARED
0xe49b1000-0xe49d5000         144K     RW NX     MEM/CACHED/WBRA
0xe49d6000-0xe49e1000          44K     RW NX     MEM/CACHED/WBRA
0xf2000000-0xf4000000          32M     RW x  SHD
0xf6000000-0xf6200000           2M     RW x  SHD
0xf6200000-0xf6201000           4K     RW NX SHD DEV/SHARED
0xf6300000-0xf6400000           1M     RW x  SHD
---[ vmalloc() End ]---
---[ Fixmap Area ]---
---[ Vectors ]---
0xffff0000-0xffff1000           4K USR ro x      MEM/CACHED/WBRA
0xffff1000-0xffff2000           4K     ro x      MEM/CACHED/WBRA
---[ Vectors End ]---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221063

On Tue, Sep 08, 2015 at 07:01:00PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> >> Gah, silly me. But even with [1], I still get an error [2]. I have a
> >> confirmation that I have a "Page Permission" fault on the
> >> probe_kernel_address().
> >
> > Hmm, that's not right.  If it's the DACR, then it should be a page domain
> > fault, not a page permission fault.
> >
> >> [2] Oops
> >> ========
> >> # insmod /tmp/unalign.ko 
> >> RJK1: fsr=23 far=e1c23643 dacr=51
> >> RJK2: fsr=23 far=e1c23643 dacr=51
> >> RJK3: fsr=2f far=bf00202c dacr=51
> >> RJK: fault=4 instr=0x00000000 instrptr=bf00202c
> >
> > Can you add a show_pte(current->mm, instrptr) to dump those page
> > table entries please?
> Most certainly, here we go :
> 
> # insmod /tmp/unalign.ko 
> RJK1: fsr=23 far=e1c1f743 dacr=51
> RJK2: fsr=23 far=e1c1f743 dacr=51
> pgd = e1cc4000
> [bf00202c] *pgd=c1cab851, *pte=c1cb504f, *ppte=c1cb501f
> RJK3: fsr=2f far=bf00202c dacr=51
> RJK4: fault=4 instr=0x00000000 instrptr=bf00202c
> pgd = e1cc4000
> [bf00202c] *pgd=c1cab851, *pte=c1cb504f, *ppte=c1cb501f

Okay, so domain = 2 (which for an Xscale 3 kernel is DOMAIN_KERNEL).
The page table entry has AP=01, which is user no-access, svc read/write.

What should happen is:

#define probe_kernel_address(addr, retval)              \
        ({                                              \
                long ret;                               \
                mm_segment_t old_fs = get_fs();         \
                                                        \
                set_fs(KERNEL_DS);                      \

This should update the DACR from 0x51 to 0x71 - switching domain 2 to
manager mode.

                pagefault_disable();                    \
                ret = __copy_from_user_inatomic(&(retval), \
			(__force typeof(retval) __user *)(addr), \
			sizeof(retval)); \
...
__copy_from_user_inatomic is an alias for __copy_from_user:

static inline unsigned long __must_check
__copy_from_user(void *to, const void __user *from, unsigned long n)
{
        unsigned int __ua_flags = uaccess_save_and_enable();

and uaccess_save_and_enable() does:

        unsigned int old_domain = get_domain();

        /* Set the current domain access to permit user accesses */
        set_domain((old_domain & ~domain_mask(DOMAIN_USER)) |
                   domain_val(DOMAIN_USER, DOMAIN_CLIENT));

So this should then end up changing the DACR from 0x71 to 0x75,
enabling user access (because this function may access userspace.)

What this means is that (going back to __copy_from_user):

        n = arm_copy_from_user(to, from, n);

At the point we call into this code, the DACR should be 0x75, which
should allow us to read the instruction at 0xbf00202c.  But this is
failing with a permission error - which it would do if it thought
the kernel domain was in manager mode (iow, 0x55).

I think some debugging in the above areas is needed - but I can't
see anything wrong.  If something were wrong, I'm pretty sure we'd
have every pre-ARMv6 machine exploding right now because of it -
and oopses wouldn't work.

You're clearly getting oopses reported correctly, which rather
proves out this code path.

Now, when you get the fault inside arm_copy_from_user(), you can
print the DACR value saved at the time the fault was generated by
printing the word above struct pt_regs on the stack - add to
arch/arm/mm/fault.c:do_DataAbort():

if (addr == 0xbf00202c) printk("DACR=0x%08x\n", *(u32 *)(regs + 1));

before the "if (!inf->fn(addr, fsr & ~FSR_LNX_PF, regs))" line.
That'll tell us what the DACR register was when we saved it.

If it isn't 0x75, then the next part of the puzzle is going to be
working out why it isn't.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221082

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> What should happen is:
Thanks very much for the explanation, hopefully I have enough material to fly on
my own now.

> Now, when you get the fault inside arm_copy_from_user(), you can
> print the DACR value saved at the time the fault was generated by
> printing the word above struct pt_regs on the stack - add to
> arch/arm/mm/fault.c:do_DataAbort():
>
> if (addr == 0xbf00202c) printk("DACR=0x%08x\n", *(u32 *)(regs + 1));
>
> before the "if (!inf->fn(addr, fsr & ~FSR_LNX_PF, regs))" line.
> That'll tell us what the DACR register was when we saved it.
>
> If it isn't 0x75, then the next part of the puzzle is going to be
> working out why it isn't.
It's 0x55. I'll track down how this happens, there are not that many places
where DACR is touched, and I'm in a very controlled environement, so I can
cunningly place JTAG breakpoints and watch DACR.

I'll report once I have a better idea what is happening, that might take me a
couple of days given that most of my workforce is available on weekends only.

Thanks again for the free lesson.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221788

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Tue, Sep 08, 2015 at 07:01:00PM +0200, Robert Jarzmik wrote:
>> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> At the point we call into this code, the DACR should be 0x75, which
> should allow us to read the instruction at 0xbf00202c.  But this is
> failing with a permission error - which it would do if it thought
> the kernel domain was in manager mode (iow, 0x55).

Okay Russell, I have a good idea what's happening now. Basically, it boils down
to compiler optimization of get_domain() which is called twice (set_fs() ->
modify_domain() -> get_domain()). See the piece in [1] for a more complete
explanation.

I still haven't finished my work, as I need to disassemble the do_alignment()
function to confirm the DACR read by get_domain() is only done once in the
probe_kernel_address() call, and yet my hopes are high this is the cause as I
traced the DACR modifications, which led me to :
[-0] 0xc0017080: set_domain(0x00000055) => dacr = 0x00000055 => second set_domain()
[-1] 0xc0017080: set_domain(0x00000071) => dacr = 0x00000071 => first set_domain()
[-2] 0xc008a124: set_domain(0x00000051) => dacr = 0x00000051

Once I have my disassembly properly analyzed (ie. the second set_fs() doesn't do
a mrc instruction), I'll have my proof. My setup is too full of traces and
attempts to stall pipeline/prefetch to conclude yet, but there is a fair chance
I'm closer to the solution now.

Cheers.

--
Robert

[1] Current patch
=================
---8<---
From 07cdda877b1b0c4fcdba3d756f6a22ce035ee672 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Thu, 10 Sep 2015 00:32:26 +0200
Subject: [PATCH] ARM: fix domain access

On the pxa310 platform, unaligned accesses are not fixed anymore with
the SW_DOMAIN_PAN configuration activated.

The trouble is that probe_kernel_address() cannot read the faulting
instruction, because of a permission error. The permission error comes
from the DACR register, whose value is incorrectly set to 0x55 instead
of 0x75.

I appears that at least gcc 4.8.2 optmizes the do_alignment() function,
and inside probe_kernel_address(), where set_fs() is called twice,
ie. modify_domain() is called twice, the get_domain() is called only
once, and the first call's value is reused. As a consequence, instead of
changing DACR as 0x51 -> 0x71 -> 0x75, it instead does 0x51 -> 0x71 ->
0x55. This is because instead of doing the last transition as :
 - (as (0x71 & ~0x03) | 0x04 = 0x75)
it does
 - (as (0x51 & ~0x03) | 0x04 = 0x55)

As a consequence, the alignment fault cannot be fixed, and triggers an
Oops:
Unable to handle kernel paging request at virtual address e1c20ec3
pgd = e1ce4000
[e1c20ec3] *pgd=c1c0044e(bad)
Internal error: Oops: 823 [#1] ARM
Modules linked in: unalign(+)
CPU: 0 PID: 610 Comm: insmod Not tainted 4.2.0-rc8-next-20150828-cm-x300+ #946
Hardware name: CM-X300 module
task: e1c69500 ti: e1cc0000 task.ti: e1cc0000
PC is at u_init+0x2c/0x40 [unalign]
LR is at u_init+0x14/0x40 [unalign]
pc : [<bf00202c>]    lr : [<bf002014>]    psr: a0000013
sp : e1cc1df0  ip : e1c20640  fp : 1e3dffdc
r10: 00000001  r9 : e1c20040  r8 : 00000000
r7 : bf002000  r6 : e1c5ee80  r5 : c0be5b80  r4 : c0be5b80
r3 : e1c20ec0  r2 : 00000004  r1 : a0000013  r0 : 00000000
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 0000397f  Table: c1ce4018  DAC: 00000051
Process insmod (pid: 610, stack limit = 0xe1cc0198)
Stack: (0xe1cc1df0 to 0xe1cc2000)

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 arch/arm/include/asm/domain.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/domain.h b/arch/arm/include/asm/domain.h
index 10c9a38636ac..47f3b8445237 100644
--- a/arch/arm/include/asm/domain.h
+++ b/arch/arm/include/asm/domain.h
@@ -87,7 +87,7 @@ static inline unsigned int get_domain(void)
 {
 	unsigned int domain;
 
-	asm(
+	asm volatile(
 	"mrc	p15, 0, %0, c3, c0	@ get domain"
 	 : "=r" (domain));
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222354

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>
>> On Tue, Sep 08, 2015 at 07:01:00PM +0200, Robert Jarzmik wrote:
>>> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>> At the point we call into this code, the DACR should be 0x75, which
>> should allow us to read the instruction at 0xbf00202c.  But this is
>> failing with a permission error - which it would do if it thought
>> the kernel domain was in manager mode (iow, 0x55).
>
> Okay Russell, I have a good idea what's happening now. Basically, it boils down
> to compiler optimization of get_domain() which is called twice (set_fs() ->
> modify_domain() -> get_domain()). See the piece in [1] for a more complete
> explanation.
>
> I still haven't finished my work, as I need to disassemble the do_alignment()
And I have the proof of gcc optimization, which I'll add to the commit message
if you want :
00000728 <do_alignment>:
     ...
     770:	ee134f10 	mrc	15, 0, r4, cr3, cr0, {0}
     ... no r4 or mrc/mcr usage
     788:	e3842030 	orr	r2, r4, #48	; 0x30
     ... no r2/r4 or mrc/mcr usage
     794:	ee032f10 	mcr	15, 0, r2, cr3, cr0, {0}
     798:	ee07cf95 	mcr	15, 0, ip, cr7, cr5, {4}
     ... no r4 or mrc/mcr usage
     7ac:	e3c4300c 	bic	r3, r4, #12
     7b0:	e3833004 	orr	r3, r3, #4
     7b4:	ee033f10 	mcr	15, 0, r3, cr3, cr0, {0}
     ... no mrc/mcr usage
     7cc:	ebfffffe 	bl	0 <arm_copy_from_user>

Here, we have in probe_kernel_address() in do_alignment():
 - @770 : r4 = DACR
 - @794 : DACR = r4 | 0x30
 - @7b4 : DACR = (r4 & 0x0c) | 0x04 => the 0x30 is lost !!!

I'll send my patch to the mailing list tomorrow, as well as the other one to
align the __bug_table session.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222360

On Thu, Sep 10, 2015 at 09:01:41PM +0200, Robert Jarzmik wrote:
> And I have the proof of gcc optimization, which I'll add to the commit message
> if you want :
> 00000728 <do_alignment>:
>      ...
>      770:	ee134f10 	mrc	15, 0, r4, cr3, cr0, {0}
>      ... no r4 or mrc/mcr usage
>      788:	e3842030 	orr	r2, r4, #48	; 0x30
>      ... no r2/r4 or mrc/mcr usage
>      794:	ee032f10 	mcr	15, 0, r2, cr3, cr0, {0}
>      798:	ee07cf95 	mcr	15, 0, ip, cr7, cr5, {4}
>      ... no r4 or mrc/mcr usage
>      7ac:	e3c4300c 	bic	r3, r4, #12
>      7b0:	e3833004 	orr	r3, r3, #4
>      7b4:	ee033f10 	mcr	15, 0, r3, cr3, cr0, {0}
>      ... no mrc/mcr usage
>      7cc:	ebfffffe 	bl	0 <arm_copy_from_user>
> 
> Here, we have in probe_kernel_address() in do_alignment():
>  - @770 : r4 = DACR
>  - @794 : DACR = r4 | 0x30
>  - @7b4 : DACR = (r4 & 0x0c) | 0x04 => the 0x30 is lost !!!
> 
> I'll send my patch to the mailing list tomorrow, as well as the other one to
> align the __bug_table session.

I've been wondering whether we can teach GCC that set_domain modifies
the value that get_domain returns, rather than throwing a volatile
onto the asm in get_domain.  The issue with a volatile there is that
even if the result is unused, but the code is reachable, gcc still has
to output the code to read the register.

We might be able to get away with a memory clobber on the set_domain,
and fake a memory read in get_domain, eg, by passing
	"m" (current_thread_info()->cpu_domain))
to the get_domain asm.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222393

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> I've been wondering whether we can teach GCC that set_domain modifies
> the value that get_domain returns, rather than throwing a volatile
> onto the asm in get_domain.  The issue with a volatile there is that
> even if the result is unused, but the code is reachable, gcc still has
> to output the code to read the register.
>
> We might be able to get away with a memory clobber on the set_domain,
> and fake a memory read in get_domain, eg, by passing
> 	"m" (current_thread_info()->cpu_domain))
> to the get_domain asm.
Ok, let's say we do it that way.

I have some concerns about it:
  (a) I see an inbalance, as set_domain() doesn't actually modify
      current_thread_info()->cpu_domain. I don't see how it will protect use
      from this scenario :
        - get_domain()
        - set_domain()
        - set_domain()

  (b) domain.h is included from thread_info.h, not the other way around
      => current_thread_info() is not accessible from domain.h
      => that would require a bit of moving things around, as thread_info
         structure description should be available for example.
      This is currently my biggest problem with this approach.

  (c) I was also wondering if a case like this could happen :
     - a function foo() does a get_domain()
       => an exception/irq whatever happens and modifies the DACR
     - foo() continues a makes a modify_domain()
       => and here the modify_domain() uses the old DACR value
      Or said differently, I wonder if there is a case of 2 get_domain() calls
      in a row with a DACR modification in between. I

What about something such as [1], without a memory clobber, but a "fake" memory
variable link ?

Cheers.

--
Robert

[1] get_domain() / set_domain() link
diff --git a/arch/arm/include/asm/domain.h b/arch/arm/include/asm/domain.h
index e878129f2fee..fc1d9c43aa08 100644
--- a/arch/arm/include/asm/domain.h
+++ b/arch/arm/include/asm/domain.h
@@ -83,13 +83,17 @@
 
 #ifndef __ASSEMBLY__
 
+static int domain_barrier;
+/*
+ * how to get the current stack pointer in C
+ */
 static inline unsigned int get_domain(void)
 {
        unsigned int domain;
 
        asm(
        "mrc    p15, 0, %0, c3, c0      @ get domain"
-        : "=r" (domain));
+        : "=r" (domain), "=m" (domain_barrier));
 
        return domain;
 }
@@ -97,8 +101,8 @@ static inline unsigned int get_domain(void)
 static inline void set_domain(unsigned val)
 {
        asm volatile(
-       "mcr    p15, 0, %0, c3, c0      @ set domain"
-         : : "r" (val));
+       "mcr    p15, 0, %1, c3, c0      @ set domain"
+       : "=m" (domain_barrier) : "r" (val));
        isb();
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222666

On Thu, Sep 10, 2015 at 10:53:43PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > I've been wondering whether we can teach GCC that set_domain modifies
> > the value that get_domain returns, rather than throwing a volatile
> > onto the asm in get_domain.  The issue with a volatile there is that
> > even if the result is unused, but the code is reachable, gcc still has
> > to output the code to read the register.
> >
> > We might be able to get away with a memory clobber on the set_domain,
> > and fake a memory read in get_domain, eg, by passing
> > 	"m" (current_thread_info()->cpu_domain))
> > to the get_domain asm.
> Ok, let's say we do it that way.
> 
> I have some concerns about it:
>   (a) I see an inbalance, as set_domain() doesn't actually modify
>       current_thread_info()->cpu_domain. I don't see how it will protect use
>       from this scenario :
>         - get_domain()
>         - set_domain()
>         - set_domain()

That should be fine, because if you've only got one get_domain(), then
you only get the value of the DACR once.

>   (b) domain.h is included from thread_info.h, not the other way around
>       => current_thread_info() is not accessible from domain.h
>       => that would require a bit of moving things around, as thread_info
>          structure description should be available for example.
>       This is currently my biggest problem with this approach.

It's not a problem since 1eef5d2f1b46 removed the need for domain.h to be
included by thread_info.h - the existing include can be dropped.

>   (c) I was also wondering if a case like this could happen :
>      - a function foo() does a get_domain()
>        => an exception/irq whatever happens and modifies the DACR

We always preserve the value of DACR across an exception.

>      - foo() continues a makes a modify_domain()
>        => and here the modify_domain() uses the old DACR value
>       Or said differently, I wonder if there is a case of 2 get_domain() calls
>       in a row with a DACR modification in between. I
> 
> What about something such as [1], without a memory clobber, but a "fake" memory
> variable link ?

The problem is the compiler will need to issue instructions to arrange
for the address of this variable to end up in registers even though the
assembly doesn't use it.

That's true of my suggestion as well, but looking at the callsites, we
generally already have, or very shortly there-after have the current
thread_info address in a register.

Patches to follow - I've not been able to confirm the instruction ordering
you've observed with my compiler, so I can't prove whether this solves
the problem locally.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222667 — [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain

We need to have memory dependencies on get_domain/set_domain to avoid
the compiler over-optimising these inline assembly instructions.

Loads/stores must not be reordered across a set_domain(), so introduce
a compiler barrier for that assembly.

The value of get_domain() must not be cached across a set_domain(), but
we still want to allow the compiler to optimise it away.  Introduce a
dependency on current_thread_info()->cpu_domain to avoid this; the new
memory clobber in set_domain() should therefore cause the compiler to
re-load this.  The other advantage of using this is we should have its
address in the register set already, or very soon after at most call
sites.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/domain.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/domain.h b/arch/arm/include/asm/domain.h
index e878129f2fee..fc8ba1663601 100644
--- a/arch/arm/include/asm/domain.h
+++ b/arch/arm/include/asm/domain.h
@@ -12,6 +12,7 @@
 
 #ifndef __ASSEMBLY__
 #include <asm/barrier.h>
+#include <asm/thread_info.h>
 #endif
 
 /*
@@ -89,7 +90,8 @@ static inline unsigned int get_domain(void)
 
 	asm(
 	"mrc	p15, 0, %0, c3, c0	@ get domain"
-	 : "=r" (domain));
+	 : "=r" (domain)
+	 : "m" (current_thread_info()->cpu_domain));
 
 	return domain;
 }
@@ -98,7 +100,7 @@ static inline void set_domain(unsigned val)
 {
 	asm volatile(
 	"mcr	p15, 0, %0, c3, c0	@ set domain"
-	  : : "r" (val));
+	  : : "r" (val) : "memory");
 	isb();
 }
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222902 — Re: [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain

Russell King <rmk+kernel@arm.linux.org.uk> writes:

> We need to have memory dependencies on get_domain/set_domain to avoid
> the compiler over-optimising these inline assembly instructions.
>
> Loads/stores must not be reordered across a set_domain(), so introduce
> a compiler barrier for that assembly.
>
> The value of get_domain() must not be cached across a set_domain(), but
> we still want to allow the compiler to optimise it away.  Introduce a
> dependency on current_thread_info()->cpu_domain to avoid this; the new
> memory clobber in set_domain() should therefore cause the compiler to
> re-load this.  The other advantage of using this is we should have its
> address in the register set already, or very soon after at most call
> sites.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

The test is failing without these 2 patches, while with them, an unaligned
access is fixed, and the generated code looks good :
     7ac:       ee13cf10        mrc     15, 0, ip, cr3, cr0, {0}
     7b0:       e3cc300c        bic     r3, ip, #12
     7b4:       e58dc014        str     ip, [sp, #20]
     7b8:       e3833004        orr     r3, r3, #4
     7bc:       ee033f10        mcr     15, 0, r3, cr3, cr0, {0}

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222908 — Re: [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain

On Fri, Sep 11, 2015 at 04:56:27PM +0200, Robert Jarzmik wrote:
> Russell King <rmk+kernel@arm.linux.org.uk> writes:
> > We need to have memory dependencies on get_domain/set_domain to avoid
> > the compiler over-optimising these inline assembly instructions.
> >
> > Loads/stores must not be reordered across a set_domain(), so introduce
> > a compiler barrier for that assembly.
> >
> > The value of get_domain() must not be cached across a set_domain(), but
> > we still want to allow the compiler to optimise it away.  Introduce a
> > dependency on current_thread_info()->cpu_domain to avoid this; the new
> > memory clobber in set_domain() should therefore cause the compiler to
> > re-load this.  The other advantage of using this is we should have its
> > address in the register set already, or very soon after at most call
> > sites.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> The test is failing without these 2 patches, while with them, an unaligned
> access is fixed, and the generated code looks good :
>      7ac:       ee13cf10        mrc     15, 0, ip, cr3, cr0, {0}
>      7b0:       e3cc300c        bic     r3, ip, #12
>      7b4:       e58dc014        str     ip, [sp, #20]
>      7b8:       e3833004        orr     r3, r3, #4
>      7bc:       ee033f10        mcr     15, 0, r3, cr3, cr0, {0}

Thanks Robert, I've queued these as fixes now.

If you want to put the bug alignment patch in the patch system, I'll get
that off to Linus this weekend too.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222922 — Re: [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> If you want to put the bug alignment patch in the patch system, I'll get
> that off to Linus this weekend too.
Sure, that will be done before tomorrow evening.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222672 — [PATCH 1/2] ARM: domains: thread_info.h no longer needs asm/domains.h

As of 1eef5d2f1b46 ("ARM: domains: switch to keeping domain value in
register") we no longer need to include asm/domains.h into
asm/thread_info.h.  Remove it.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/thread_info.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index d0a1119dcaf3..776757d1604a 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -25,7 +25,6 @@
 struct task_struct;
 
 #include <asm/types.h>
-#include <asm/domain.h>
 
 typedef unsigned long mm_segment_t;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web