Re: numatop: %{optflags} fail the 32bit build
The issue has been solved by my reviewer, so thank you all because as
usual I've learned interesting things :)
Comments inlined.
On Thu, Sep 12, 2013 at 2:53 PM, Florian Weimer fwei...@redhat.com wrote:
On 09/12/2013 02:11 PM, Dridi Boukelmoune wrote:
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I kind of understand what you're doing here, but it's not all clear.
I get the push/pop instructions save and restore the reserved ebx
register, which is needed because apparently the cpuid instruction
would otherwise overwrite it.
I don't understand the mov instruction, but I suppose you're storing
ebx's value from cpuid somewhere else before restoring it with the
pop instruction.
Correct, the intent is to write the %ebx register value to the address in
the %esi register. (%1) is a pointer dereference, as oppose to plain %1.
I don't understand the last 5 lines of __asm in both functions, I've
never seen this syntax before.
These are register constraints. a, c, d, S refer to the %eax, %ecx,
%edx, %esi registers, respectively. = marks output constraints. The
constraints before the final : are output registers, and after colon,
there are the input registers. There's just one, and 0 means to reuse the
first output register.
Okay, silly me, I should have listed S among the output registers, instead
the inputs:
Actually, this morning (in the train) I've tested your code with my
tmp variable and it works if I remove the deref!
mov %%ebx, %1
(btw, what do %1 and %4 mean ?)
I could painlessly add a println in my patch since the file already
includes stdio.h. Normal and hardened build provide the same cpuid
values.
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%4)\n\t
pop %%ebx
: =a (*eax),
=c (*ecx),
=d (*edx)
: 0 (*eax),
S (ebx));
}
I also forget that for full correctness, there should now be a memory
clobber as well (in the clobber section after yet another colon):
What would it do ? A compiler memory barrier ?
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%4)\n\t
pop %%ebx
: =a (*eax),
=c (*ecx),
=d (*edx)
: 0 (*eax),
S (ebx)
: memory);
}
It's a good thing my reviewer submitted a patch, because I wouldn't
feel that confident with mine :)
By the way, we could generate much better code if the registers were passed
as an array or struct, so that they are in consecutive memory:
struct regs {
unsigned eax, ebx, ecx, edx;
};
void
cpuid(struct regs *r)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%eax, (%0)\n\t
mov %%ebx, 4(%0)\n\t
mov %%ecx, 8(%0)\n\t
mov %%edx, 12(%0)\n\t
pop %%ebx
:
: S (r)
: eax, ecx, edx, memory);
}
Obviously, this needs adjustments to the callers.
Yup, but for the sake of simplicity, I wouldn't do that.
--
Florian Weimer / Red Hat Product Security Team
Dridi
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On 09/13/2013 10:12 AM, Dridi Boukelmoune wrote: Actually, this morning (in the train) I've tested your code with my tmp variable and it works if I remove the deref! Yes, that should work as well, because the value itself is an output register. mov %%ebx, %1 (btw, what do %1 and %4 mean ?) These are placeholders for the register names and are replaced with the actual register names chosen by the compiler, based on the specified constraints and the surrounding code the compiler generated from the C source. I also forget that for full correctness, there should now be a memory clobber as well (in the clobber section after yet another colon): What would it do ? A compiler memory barrier ? Correct. -- Florian Weimer / Red Hat Product Security Team -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On Thu, Sep 12, 2013 at 5:39 PM, Dave Jones da...@redhat.com wrote:
On Wed, Sep 11, 2013 at 08:46:33AM +0200, Florian Weimer wrote:
This is the offending function:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(cpuid\n\t
: =a (*eax),
=b (*ebx),
=c (*ecx),
=d (*edx)
: a (*eax));
}
The cryptic GCC error message (inconsistent operand constraints in
an ‘asm’) refers to the fact that in PIC mode (which is activated
by the hardening flags), %ebx is a reserved register.
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I have not actually tested this. There are other solutions
floating around, but they are clearly incorrect and produce wrong
code.
A better fix would be to rip all this out, and use reads from
/dev/cpu/*/cpuid, which is arch agnostic, and also takes care of cpu affinity
problems.
Dave
On my ubuntu machine at work, I have this:
$ ls /dev/cpu/
microcode
Also, wouldn't it be a problem on a chrooted environment ?
And during my tests I've noticed that numatop calls cpuid once for
each core on my laptop. It should discard the affinity problem,
shouldn't it ?
Tanks again for your help, it's been very interesting for me !
Dridi
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
Hi,
On Wed, Sep 11, 2013 at 8:46 AM, Florian Weimer fwei...@redhat.com wrote:
On 09/11/2013 12:31 AM, Dridi Boukelmoune wrote:
I have my first packaging issue on the numatop package[1].
During the review it appeared that I forgot the %{optflags}, and that
adding them breaks the i686 build. The upstream dev is very patient
and willing to help me, but I feel I have wasted enough of his time.
The guilty gcc flag seems to be:
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 [2]
It would be helpful to provide more context and pointers to the analysis so
far.
The failing build log is here:
http://kojipkgs.fedoraproject.org//work/tasks/1414/5911414/build.log
This is the offending function:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(cpuid\n\t
: =a (*eax),
=b (*ebx),
=c (*ecx),
=d (*edx)
: a (*eax));
}
The cryptic GCC error message (inconsistent operand constraints in an ‘asm’)
refers to the fact that in PIC mode (which is activated by the hardening
flags), %ebx is a reserved register.
Thanks for the explanation, I could never have figured this out.
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I kind of understand what you're doing here, but it's not all clear.
I get the push/pop instructions save and restore the reserved ebx
register, which is needed because apparently the cpuid instruction
would otherwise overwrite it.
I don't understand the mov instruction, but I suppose you're storing
ebx's value from cpuid somewhere else before restoring it with the
pop instruction.
I don't understand the last 5 lines of __asm in both functions, I've
never seen this syntax before.
I have not actually tested this. There are other solutions floating around,
but they are clearly incorrect and produce wrong code.
It builds, but it segfaults. Probably because the value is written
directly in the ebx variable, which is a pointer. I've added a temp
variable to fix the segfault, but I still need to check whether it
gives the expected value:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
#ifdef __x86_64
// original version
#else
unsigned int tmp;
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (tmp),
=c (*ecx),
=d (*edx)
: 0 (*eax));
*ebx = tmp;
#endif
}
I don't actually have the code on this computer but I remember doing
something like that, so this is only pseudo code :)
In 64 bit mode, you should use the original version.
--
Florian Weimer / Red Hat Product Security Team
Thank you both for your help, I'll find some time this weekend to test
it further before sending it back to the upstream project.
Dridi
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On 09/12/2013 02:11 PM, Dridi Boukelmoune wrote: I don't understand the last 5 lines of __asm in both functions, I've never seen this syntax before. It's gcc's extended asm syntax (Aka. inline asm in C): c.f. http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html ff. Ralf -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On 09/12/2013 02:11 PM, Dridi Boukelmoune wrote:
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I kind of understand what you're doing here, but it's not all clear.
I get the push/pop instructions save and restore the reserved ebx
register, which is needed because apparently the cpuid instruction
would otherwise overwrite it.
I don't understand the mov instruction, but I suppose you're storing
ebx's value from cpuid somewhere else before restoring it with the
pop instruction.
Correct, the intent is to write the %ebx register value to the address
in the %esi register. (%1) is a pointer dereference, as oppose to
plain %1.
I don't understand the last 5 lines of __asm in both functions, I've
never seen this syntax before.
These are register constraints. a, c, d, S refer to the %eax,
%ecx, %edx, %esi registers, respectively. = marks output constraints.
The constraints before the final : are output registers, and after
colon, there are the input registers. There's just one, and 0 means
to reuse the first output register.
Okay, silly me, I should have listed S among the output registers,
instead the inputs:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%4)\n\t
pop %%ebx
: =a (*eax),
=c (*ecx),
=d (*edx)
: 0 (*eax),
S (ebx));
}
I also forget that for full correctness, there should now be a memory
clobber as well (in the clobber section after yet another colon):
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%4)\n\t
pop %%ebx
: =a (*eax),
=c (*ecx),
=d (*edx)
: 0 (*eax),
S (ebx)
: memory);
}
By the way, we could generate much better code if the registers were
passed as an array or struct, so that they are in consecutive memory:
struct regs {
unsigned eax, ebx, ecx, edx;
};
void
cpuid(struct regs *r)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%eax, (%0)\n\t
mov %%ebx, 4(%0)\n\t
mov %%ecx, 8(%0)\n\t
mov %%edx, 12(%0)\n\t
pop %%ebx
:
: S (r)
: eax, ecx, edx, memory);
}
Obviously, this needs adjustments to the callers.
--
Florian Weimer / Red Hat Product Security Team
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On 09/12/2013 02:53 PM, Florian Weimer wrote:
By the way, we could generate much better code if the registers were
passed as an array or struct, so that they are in consecutive memory:
struct regs {
unsigned eax, ebx, ecx, edx;
};
void
cpuid(struct regs *r)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%eax, (%0)\n\t
mov %%ebx, 4(%0)\n\t
mov %%ecx, 8(%0)\n\t
mov %%edx, 12(%0)\n\t
pop %%ebx
:
: S (r)
: eax, ecx, edx, memory);
}
Okay, I forgot to load the %eax register. This version has actually
been tested (on x86_64):
void
cpuid(struct regs *r)
{
__asm volatile
(mov (%0), %%eax\n\t
push %%rbx\n\t
cpuid\n\t
mov %%eax, (%0)\n\t
mov %%ebx, 4(%0)\n\t
mov %%ecx, 8(%0)\n\t
mov %%edx, 12(%0)\n\t
pop %%rbx
:
: D (r)
: eax, ecx, edx, memory);
}
D picks the %edi register, which saves another move because it
contains the first function parameter on x86_64.
For the i386 version, replace %rbx with %ebx.
--
Florian Weimer / Red Hat Product Security Team
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On Wed, Sep 11, 2013 at 08:46:33AM +0200, Florian Weimer wrote:
This is the offending function:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(cpuid\n\t
: =a (*eax),
=b (*ebx),
=c (*ecx),
=d (*edx)
: a (*eax));
}
The cryptic GCC error message (inconsistent operand constraints in
an ‘asm’) refers to the fact that in PIC mode (which is activated
by the hardening flags), %ebx is a reserved register.
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I have not actually tested this. There are other solutions
floating around, but they are clearly incorrect and produce wrong
code.
A better fix would be to rip all this out, and use reads from
/dev/cpu/*/cpuid, which is arch agnostic, and also takes care of cpu affinity
problems.
Dave
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On Wed, 11 Sep 2013 00:31:12 +0200
Dridi Boukelmoune dridi.boukelmo...@gmail.com wrote:
Hi,
I have my first packaging issue on the numatop package[1].
During the review it appeared that I forgot the %{optflags}, and that
adding them breaks the i686 build. The upstream dev is very patient
and willing to help me, but I feel I have wasted enough of his time.
The guilty gcc flag seems to be:
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 [2]
I can (hopefully) easily reproduce the failure with just mock on my
machine, but right now I can't figure out how to solve this. And the
fact that I don't know/understand this flag at all doesn't help.
Does anyone know what could be the cause and how to solve this ?
the -spec option is used to set flags for gcc from a file, see
https://bugzilla.redhat.com/show_bug.cgi?id=991221#c15
Dan
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: numatop: %{optflags} fail the 32bit build
On 09/11/2013 12:31 AM, Dridi Boukelmoune wrote:
I have my first packaging issue on the numatop package[1].
During the review it appeared that I forgot the %{optflags}, and that
adding them breaks the i686 build. The upstream dev is very patient
and willing to help me, but I feel I have wasted enough of his time.
The guilty gcc flag seems to be:
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 [2]
It would be helpful to provide more context and pointers to the analysis
so far.
The failing build log is here:
http://kojipkgs.fedoraproject.org//work/tasks/1414/5911414/build.log
This is the offending function:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(cpuid\n\t
: =a (*eax),
=b (*ebx),
=c (*ecx),
=d (*edx)
: a (*eax));
}
The cryptic GCC error message (inconsistent operand constraints in an
‘asm’) refers to the fact that in PIC mode (which is activated by the
hardening flags), %ebx is a reserved register.
This version should work in 32 bit mode, and only in 32 bit mode:
void
cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
{
__asm volatile
(push %%ebx\n\t
cpuid\n\t
mov %%ebx, (%1)\n\t
pop %%ebx
: =a (*eax),
=S (ebx),
=c (*ecx),
=d (*edx)
: 0 (*eax));
}
I have not actually tested this. There are other solutions floating
around, but they are clearly incorrect and produce wrong code.
In 64 bit mode, you should use the original version.
--
Florian Weimer / Red Hat Product Security Team
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
numatop: %{optflags} fail the 32bit build
Hi,
I have my first packaging issue on the numatop package[1].
During the review it appeared that I forgot the %{optflags}, and that
adding them breaks the i686 build. The upstream dev is very patient
and willing to help me, but I feel I have wasted enough of his time.
The guilty gcc flag seems to be:
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 [2]
I can (hopefully) easily reproduce the failure with just mock on my
machine, but right now I can't figure out how to solve this. And the
fact that I don't know/understand this flag at all doesn't help.
Does anyone know what could be the cause and how to solve this ?
Dridi
[1] https://bugzilla.redhat.com/show_bug.cgi?id=numatop#c11
[2] it fails regardless of the _hardened_build macro
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
