fix ldcw inline assembler
Helge Deller
maintainerPATCH 1/1 fix ldcw inline assembler
This "fix ldcw assembler" patch below is really an old one now, but sadly it never got applied.
I just tried it again. Without this patch I always see login-problems when ssh-ing into my parisc box. The very first time sshd just drops the connection (Connection closed by remote host). With this patch I didn't faced this problem again.
I'm wondering, if not other userspace problems suddenly go away then as well, e.g. the uid/gid issues others are seeing:
http://marc.info/?l=linux-parisc&m=121114269417948&w=2
Kyle, please apply.
Helge
-------- Original Message --------
Subject: [PATCH] ldcw inline assembler patch
From: Dave Anglin
There are two reasons to expose the memory *a in the asm:
1) To prevent the compiler from discarding a preceeding write to *a, and
2) to prevent it from caching *a in a register over the asm.
The change has had a few days testing with a SMP build of 2.6.22.19 running on a rp3440.
This patch is about the correctness of the __ldcw() macro itself. The use of the macro should be confined to small inline functions to try to limit the effect of clobbering memory on GCC's optimization of loads and stores.
arch/parisc/include/asm/system.h +2 -2
@@ -168,8 +168,8 @@ static inline void set_eiem(unsigned long val)/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*. */ #define __ldcw (a) ({ \ unsigned __ret; \ - __asm__ __volatile__(__LDCW " 0(%1),%0" \ - : "=r" (__ret) : "r" (a)); \ + __asm__ __volatile__(__LDCW " 0(%2),%0" \ + : "=r" (__ret), "+m" (*(a)) : "r" (a)); \ __ret; \ })
Quoted 6 lines · Helge Deller
This "fix ldcw assembler" patch below is really an old one now, but sadly it never got applied.
I just tried it again. Without this patch I always see login-problems when ssh-ing into my parisc box. The very first time sshd just drops the connection (Connection closed by remote host). With this patch I didn't faced this problem again.
Quoted 6 lines
I'm wondering, if not other userspace problems suddenly go away then as well, e.g. the uid/gid issues others are seeing:
http://marc.info/?l=linux-parisc&m=121114269417948&w=2Kyle, please apply.
gotcha.
Quoted 7 lines · Helge Deller
Quoted 6 lines
This "fix ldcw assembler" patch below is really an old one now, but sadly it never got applied.
I just tried it again. Without this patch I always see login-problems when ssh-ing into my parisc box. The very first time sshd just drops the connection (Connection closed by remote host). With this patch I didn't faced this problem again.
Quoted 8 lines
Quoted 6 lines
I'm wondering, if not other userspace problems suddenly go away then as well, e.g. the uid/gid issues others are seeing:
http://marc.info/?l=linux-parisc&m=121114269417948&w=2Kyle, please apply.
gotcha.
Kyle, can you try and push it into 2.6.30? It fixes a major security hole.
I've tried it with 2.6.30-rc4 on my c3750 and agree with Helge that it fixes the ssh bug that I had on this machine.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Quoted 14 lines
Quoted 14 lines · Helge Deller
Quoted 12 lines
This "fix ldcw assembler" patch below is really an old one now, but sadly it never got applied.
I just tried it again. Without this patch I always see login-problems when ssh-ing into my parisc box. The very first time sshd just drops the connection (Connection closed by remote host). With this patch I didn't faced this problem again.
I'm wondering, if not other userspace problems suddenly go away then as well, e.g. the uid/gid issues others are seeing:
http://marc.info/?l=linux-parisc&m=121114269417948&w=2Kyle, please apply.
Quoted 6 lines
Quoted 1 line
gotcha.
Kyle, can you try and push it into 2.6.30? It fixes a major security hole.
I've tried it with 2.6.30-rc4 on my c3750 and agree with Helge that it fixes the ssh bug that I had on this machine.
Sorry, I should not have pushed for 2.6.30. The ssh bug is still present on my c3750.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
arch/parisc/include/a=
Quoted 1 line · Helge Deller
arch/parisc/include/a= renamed from arch/parisc/include/asm/system.h +0
-0
sm/system.h
Quoted 5 lines
index ee80c92..d91357b 100644
arch/parisc/include/asm/system.h +0
-0@@ -168,8 +168,8 @@ static inline void set_eiem(unsigned long val)=A0/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.=
=A0*/
Quoted 1 line
=A0#define
__ldcw(a)({ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
Quoted 1 line
=A0 =A0 =A0 =A0unsigned
__ret; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 \
Quoted 1 line
- =A0 =A0 =A0
__asm____volatile__(__LDCW " 0(%1),%0" =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 \
Quoted 1 line
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 : "=3Dr" (
__ret) : "r" (a)); =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
Quoted 1 line
+ =A0 =A0 =A0
__asm____volatile__(__LDCW " 0(%2),%0" =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 \
Quoted 1 line
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 : "=3Dr" (
__ret), "+m" (*(a)) : "r" (a)=
); =A0 =A0 =A0 =A0 \
Quoted 1 line
=A0 =A0 =A0 =A0__ret; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
Quoted 1 line
=A0})
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define __ldcw(a) \
({ =
\
unsigned int __ret; =
\
__asm__ __volatile__("ldcw 0(%1),%0" =
\
: "=3Dr" (__ret) : "r" (a) : "memory"); = \
__ret; =
\
})
~~~
I wonder if I should change that to match the kernel?
This is currently used in the Linuxthreads->NPTL compat code, and in the old Linuxthreads code.
Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Carlos O'Donell
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\
Quoted 8 lines
({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Quoted 16 lines · John David Anglin
Quoted 5 lines · Carlos O'Donell
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\Quoted 8 lines
({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?
Quoted 4 lines
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
I, er, don't think we do, not for the spinlock primitives at least, as far as I can tell...
regards, Kyle
James Bottomley
maintainerQuoted 17 lines · Kyle McMartin
Quoted 16 lines · John David Anglin
Quoted 13 lines · Carlos O'Donell
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?
Quoted 7 lines
Quoted 4 lines
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
I, er, don't think we do, not for the spinlock primitives at least, as far as I can tell...
Yes we do ... look in asm/spinlock.h
it's all the mb(); statements that are scattered through our _raw_spin_ ops
The original problem was that the spinlocks were compile barrier leaky and caused infrequent but hard to debug issues on smp. The barriers are likely overkill (since we have two in each) but at least they prevent problems.
James
Quoted 28 lines · James Bottomley
Quoted 17 lines · Kyle McMartin
Quoted 16 lines · John David Anglin
Quoted 13 lines · Carlos O'Donell
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?Quoted 7 lines
Quoted 4 lines
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
I, er, don't think we do, not for the spinlock primitives at least, as far as I can tell...
Yes we do ... look in
asm/spinlock.h
Quoted 8 lines
it's all the
mb(); statements that are scattered through our _raw_spin_ opsThe original problem was that the spinlocks were compile barrier leaky and caused infrequent but hard to debug issues on smp. The barriers are likely overkill (since we have two in each) but at least they prevent problems.
Yeah, I was looking at the lack of a barrier between ldcw and the test of *a == 0. I guess this would be fixed by Helge's patch.
--Kyle
James Bottomley
maintainerQuoted 32 lines · Kyle McMartin
Quoted 31 lines · James Bottomley
Quoted 17 lines · Kyle McMartin
Quoted 16 lines · John David Anglin
Quoted 13 lines · Carlos O'Donell
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?Quoted 7 lines
Quoted 4 lines
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
I, er, don't think we do, not for the spinlock primitives at least, as far as I can tell...
Yes we do ... look in
asm/spinlock.hit's all the
mb(); statements that are scattered through our _raw_spin_ ops
Quoted 8 lines
Quoted 5 lines
The original problem was that the spinlocks were compile barrier leaky and caused infrequent but hard to debug issues on smp. The barriers are likely overkill (since we have two in each) but at least they prevent problems.
Yeah, I was looking at the lack of a barrier between ldcw and the test of *a == 0. I guess this would be fixed by Helge's patch.
OK, now I'm confused. Barriers are used to inform the compiler about interlocks it isn't aware of (like when an asm changes a variable). The ldcw and the *a both mention a which is sufficient an interlock for the compiler to get it right without any barrier.
Added to which, *a is declared volatile, which is enough of a cautionary note to make the compiler behave in a very straightforward fashion.
James
Quoted 9 lines · James Bottomley
OK, now I'm confused. Barriers are used to inform the compiler about interlocks it isn't aware of (like when an asm changes a variable). The ldcw and the *a both mention a which is sufficient an interlock for the compiler to get it right without any barrier.
Added to which, *a is declared volatile, which is enough of a cautionary note to make the compiler behave in a very straightforward fashion.
I guess... I don't understand the gcc clobber semantics on asm() anymore, I'm just thinking of that stupid bug with ip_fast_csum we saw on parisc a few years ago...
Don't mind me.
--Kyle
Quoted 3 lines
I guess... I don't understand the gcc clobber semantics on
asm()anymore, I'm just thinking of that stupid bug withip_fast_csumwe saw on parisc a few years ago...
These are in fact subtle and complex. There were two or three bugs that appeared in the transition to 4.4.0 as a result of the change to the new register allocator. In paricular, using clobbers of hard registers in call patterns caused a number of problems.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Quoted 17 lines · John David Anglin
Quoted 16 lines · Carlos O'Donell
Quoted 14 lines
Historical note...
We clobber all of memory in userspace, like this:
~~~
#define__ldcw(a)\({ \ unsigned int __ret; \ __asm__ __volatile__("ldcw 0(%1),%0" \ : "=r" (__ret) : "r" (a) : "memory"); \ __ret; \})
~~~
I wonder if I should change that to match the kernel?
Quoted 7 lines
Quoted 4 lines
The above is perfectly safe. I believe the kernel provides a memory barrier when necessary. There's a discussion somewhere in the mail archives.
I, er, don't think we do, not for the spinlock primitives at least, as far as I can tell...
Ok, I'm going to see if the "memory" clobber improves life.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)