1:58 p.m.
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
This document represents a proposed Change. As part of the Changes
process, proposals are publicly announced in order to receive
community feedback. This proposal will only be implemented if approved
by the Fedora Engineering Steering Committee.
== Summary ==
Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
improve mitigation of security issues arising from buffer overflows in
packages in Fedora.
== Owner ==
* Name: [[User:siddhesh| Siddhesh Poyarekar]]
* Email: sipoyare(a)redhat.com
== Detailed Description ==
Default C and C++ compiler flags to build packages in Fedora currently
includes `-Wp,-D_FORTIFY_SOURCE=2`, which enables fortification of
some functions in glibc, thus providing some mitigation against buffer
overflows. Since glibc 2.34 and GCC 12, there has been a new
fortification level (`_FORTIFY_SOURCE=3`) which improves the coverage
of this mitigation.
The core change to bring in this mitigation is to change the default
build flags in `redhat-rpm-config` so that packages build by default
with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
that do not interact well with `_FORTIFY_SOURCE` and will also need a
workaround to downgrade fortification to level 2. The change will also
include this override.
== Benefit to Fedora ==
[https://docs.google.com/spreadsheets/d/1nPSmbEf3HVB91zI8yBraMqVry3_ILmlV2...
Analysis of packages] in Fedora rawhide indicate that the improvement
of mitigation coverage is on average over 2.4x, in some cases
protecting more than half of the fortified glibc calls in the target
application.
This change will thus harden Fedora to a significant extent, thus
making it a more secure distribution out of the box.
== Scope ==
* Proposal owners: Post a merge request to redhat-rpm-config with the
actual change to build flags.
* Other developers:
Resolve bugs filed for build failures, either by fixing the bug
exposed by `_FORTIFY_SOURCE=3` or by disabling `_FORTIFY_SOURCE=3` for
the package if it is a false positive or if the package is unable to
adapt to the change.
* Release engineering: Mass rebuild required
* Policies and guidelines: Guidelines should include workaround for
packages that fail to build with `-Wp,-D_FORTIFY_SOURCE=3` due to a
false positive.
* Trademark approval: N/A (not needed for this Change)
== Upgrade/compatibility impact ==
No ABI change, so there should be no impact on compatibility in a
mixed environment.
== How To Test ==
* Smoke testing of packages to ensure that they continue to work
correctly. Some packages may have overflows exposed at runtime, which
may need to be fixed.
== User Experience ==
No noticeable change to users.
== Dependencies ==
None.
== Contingency Plan ==
* Contingency mechanism: (What to do? Who will do it?) If too many
packages are found to be broken at runtime, the default for
fortification will be left at `_FORTIFY_SOURCE=2` for Fedora 38.
Change owner will do this in `redhat-rpm-config`
* Contingency deadline: Beta freeze
* Blocks release? Yes
* Blocks product? No
== Documentation ==
[https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
More context on `_FORTIFY_SOURCE=3` improvements].
== Release Notes ==
--
Ben Cotton
He / Him / His
Fedora Program Manager
Red Hat
TZ=America/Indiana/Indianapolis
2:04 p.m.
On Mon, Dec 5, 2022 at 2:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
* Release engineering: Mass rebuild required
Please file a ticket with Release Engineering if you haven't already.
== Contingency Plan ==
* Contingency mechanism: (What to do? Who will do it?) If too many
packages are found to be broken at runtime, the default for
fortification will be left at `_FORTIFY_SOURCE=2` for Fedora 38.
Change owner will do this in `redhat-rpm-config`
* Contingency deadline: Beta freeze
Would a full mass rebuild be required to invoke the contingency
mechanism, or would we just need to rebuild affected packages? In
either case (but definitely if a full mass rebuild is required), I'm
concerned that this contingency deadline is too late in the schedule.
Branch day (2023-02-07) seems like it would be a better fit.
--
Ben Cotton
He / Him / His
Fedora Program Manager
Red Hat
TZ=America/Indiana/Indianapolis
2:16 p.m.
On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
It is my vague recollection (I could easily be wrong, so
correct me as appropriate) that _FORTIFY_SOURCE=3
adds some runtime overhead that did not apply in
previous levels.
If that is correct, has the potential performance impact
been evaluated and documented somewhere? And, if
correct, the change proposal should probably be modified
to mention the potential performance impacts.
4:52 p.m.
On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
>
>
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
>
It is my vague recollection (I could easily be wrong, so
correct me as appropriate) that _FORTIFY_SOURCE=3
adds some runtime overhead that did not apply in
previous levels.
If that is correct, has the potential performance impact
been evaluated and documented somewhere? And, if
correct, the change proposal should probably be modified
to mention the potential performance impacts.
It has a similar impact that turning back on frame pointers would.
Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
I'm extremely displeased now, as the toolchain team basically told us
they wouldn't accept register pressure on x86_64 and then turned
around and made a proposal that does the same thing. Apparently
quality of life improvements for developers and real-time tracing
(e.g. making bpftrace useful) isn't worth it, but this is.
I want a really good justification for not doing both at the same time
if we're going to accept this.
--
真実はいつも一つ！/ Always, there's only one truth!
9:12 p.m.
On Mon, Dec 5, 2022 at 10:53 PM Neal Gompa <ngompa13(a)gmail.com> wrote:
It has a similar impact that turning back on frame pointers would.
Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
That article explicitly states:
"We need a proper study of performance and code size to understand
the magnitude of the impact"
I look forward to seeing the results of that proper study before
this is even considered for approval (since, after all, one of the
strong push-backs for -fno-omit-frame-pointer was performance).
2:19 a.m.
On Tue, Dec 06, 2022 at 03:12:19AM +0000, Gary Buhrmaster wrote:
On Mon, Dec 5, 2022 at 10:53 PM Neal Gompa <ngompa13(a)gmail.com>
wrote:
> It has a similar impact that turning back on frame pointers would.
>
> Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
>
That article explicitly states:
"We need a proper study of performance and code size to understand
the magnitude of the impact"
I look forward to seeing the results of that proper study before
this is even considered for approval (since, after all, one of the
strong push-backs for -fno-omit-frame-pointer was performance).
Note that is not a fully equivalent scenario. The no-omit-frame-pointer
proposal was only offering a functional debugging benefit to a fairly
small number of users who are also developers, while adding a likely
performance hit to all users. There needs to be a high bar to justify
the performance hit when the benefit offered is narrow.
This proposal is adding a functional security benefit to all users,
alongside the possible performance hit. This is more easily justifiable,
especially given Fedora's track record of being willing to security
improvements even when they have a performance hit.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
2:46 a.m.
On Tue, Dec 06, 2022 at 08:19:22AM +0000, Daniel P. Berrangé wrote:
On Tue, Dec 06, 2022 at 03:12:19AM +0000, Gary Buhrmaster wrote:
> On Mon, Dec 5, 2022 at 10:53 PM Neal Gompa <ngompa13(a)gmail.com> wrote:
>
> > It has a similar impact that turning back on frame pointers would.
> >
> > Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
> >
>
> That article explicitly states:
> "We need a proper study of performance and code size to understand
> the magnitude of the impact"
>
> I look forward to seeing the results of that proper study before
> this is even considered for approval (since, after all, one of the
> strong push-backs for -fno-omit-frame-pointer was performance).
Note that is not a fully equivalent scenario. The no-omit-frame-pointer
proposal was only offering a functional debugging benefit to a fairly
small number of users who are also developers, while adding a likely
performance hit to all users. There needs to be a high bar to justify
the performance hit when the benefit offered is narrow.
I'm not sure about this - more reliable stack traces affect anyone who
hits a bug, which is all users. (Plus we should strive to turn more
users into developers as a general point about computing.)
This proposal is adding a functional security benefit to all users,
alongside the possible performance hit. This is more easily justifiable,
especially given Fedora's track record of being willing to security
improvements even when they have a performance hit.
I agree here.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
11:52 a.m.
On Tue, Dec 06, 2022 at 03:12:19AM +0000, Gary Buhrmaster wrote:
Note that is not a fully equivalent scenario. The no-omit-frame-pointer
proposal was only offering a functional debugging benefit to a fairly
small number of users who are also developers, while adding a likely
performance hit to all users. There needs to be a high bar to justify
the performance hit when the benefit offered is narrow.
First, frame pointers are not just for debugging benefit. It's not even it's main
benefit from POV of https://pagure.io/fesco/issue/2817. Frame pointers are for performance
profiling and observability first and foremost, and that's most useful under
real-world conditions of production workloads. Not some custom re-built debug versions of
applications.
Second, it might benefit a relatively small (but not tiny, it's at least thousands of
people doing performance profiling) fraction of users, but those users (developers that
care about performance) are the ones bringing benefits to very wide user base.
And third, with appropriate infrastructure of background perf profiling that projects like
GNOME and KDE can put in place (if they haven't already), user doesn't have to be
involved in performance profiling directly to benefit the ecosystem at large, providing
anonymized source of real-world profiling data that could be aggregated and looked at by
GNOME/KDE/whatnot developers that do care about performance.
But this won't happen unless GNOME/KDE can rely on having frame pointers in user
systems.
>
> This proposal is adding a functional security benefit to all users,
> alongside the possible performance hit. This is more easily justifiable,
> especially given Fedora's track record of being willing to security
> improvements even when they have a performance hit.
>
> With regards,
> Daniel
1:02 p.m.
On Tue, Dec 06, 2022 at 05:52:16PM -0000, Andrii Nakryiko wrote:
> On Tue, Dec 06, 2022 at 03:12:19AM +0000, Gary Buhrmaster
wrote:
>
> Note that is not a fully equivalent scenario. The no-omit-frame-pointer
> proposal was only offering a functional debugging benefit to a fairly
> small number of users who are also developers, while adding a likely
> performance hit to all users. There needs to be a high bar to justify
> the performance hit when the benefit offered is narrow.
First, frame pointers are not just for debugging benefit. It's not even it's main
benefit from POV of https://pagure.io/fesco/issue/2817. Frame pointers are for performance
profiling and observability first and foremost, and that's most useful under
real-world conditions of production workloads. Not some custom re-built debug versions of
applications.
Second, it might benefit a relatively small (but not tiny, it's at least thousands of
people doing performance profiling) fraction of users, but those users (developers that
care about performance) are the ones bringing benefits to very wide user base.
Yes! I spent a frustrating time getting perf to record stack traces
properly until I recompiled the program with frame pointers. (I know
about --call-graph=dwarf but it doesn't seem to work most of the time.)
Rich.
And third, with appropriate infrastructure of background perf
profiling that projects like GNOME and KDE can put in place (if they haven't already),
user doesn't have to be involved in performance profiling directly to benefit the
ecosystem at large, providing anonymized source of real-world profiling data that could be
aggregated and looked at by GNOME/KDE/whatnot developers that do care about performance.
But this won't happen unless GNOME/KDE can rely on having frame pointers in user
systems.
>
> This proposal is adding a functional security benefit to all users,
> alongside the possible performance hit. This is more easily justifiable,
> especially given Fedora's track record of being willing to security
> improvements even when they have a performance hit.
>
> With regards,
> Daniel
_______________________________________________
devel mailing list -- devel(a)lists.fedoraproject.org
To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
7:24 p.m.
On 12/6/22 14:02, Richard W.M. Jones wrote:
On Tue, Dec 06, 2022 at 05:52:16PM -0000, Andrii Nakryiko wrote:
>> On Tue, Dec 06, 2022 at 03:12:19AM +0000, Gary Buhrmaster wrote:
>>
>> Note that is not a fully equivalent scenario. The no-omit-frame-pointer
>> proposal was only offering a functional debugging benefit to a fairly
>> small number of users who are also developers, while adding a likely
>> performance hit to all users. There needs to be a high bar to justify
>> the performance hit when the benefit offered is narrow.
>
> First, frame pointers are not just for debugging benefit. It's not even it's
main benefit from POV of https://pagure.io/fesco/issue/2817. Frame pointers are for
performance profiling and observability first and foremost, and that's most useful
under real-world conditions of production workloads. Not some custom re-built debug
versions of applications.
>
> Second, it might benefit a relatively small (but not tiny, it's at least
thousands of people doing performance profiling) fraction of users, but those users
(developers that care about performance) are the ones bringing benefits to very wide user
base.
Yes! I spent a frustrating time getting perf to record stack traces
properly until I recompiled the program with frame pointers. (I know
about --call-graph=dwarf but it doesn't seem to work most of the time.)
That is due to known limitations in perf, IIUC. Hence why at least I was
pushing so heavily to improve perf to not require frame pointers.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
7:18 a.m.
On Mon, Dec 5, 2022 at 10:20 PM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
On Mon, Dec 5, 2022 at 10:53 PM Neal Gompa <ngompa13(a)gmail.com> wrote:
> It has a similar impact that turning back on frame pointers would.
>
> Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
>
That article explicitly states:
"We need a proper study of performance and code size to understand
the magnitude of the impact"
I look forward to seeing the results of that proper study before
this is even considered for approval (since, after all, one of the
strong push-backs for -fno-omit-frame-pointer was performance).
Besides the point that the two are not comparable, code size results
are in the sheet I linked to in the proposal; there's no negative
impact. The feature has been enabled by default in OpenSUSE for
nearly a year and there have been no noticeable performance issues
reported there either. IMO the magnitude of improvement is such that
a benchmark shouldn't be a blocker for this feature, but if that's
what FESCO insists, I'll be happy to run whatever benchmark they
suggest.
Sid
6:49 a.m.
On Mon, Dec 5, 2022 at 5:53 PM Neal Gompa <ngompa13(a)gmail.com> wrote:
On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
> >
> >
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
> >
>
> It is my vague recollection (I could easily be wrong, so
> correct me as appropriate) that _FORTIFY_SOURCE=3
> adds some runtime overhead that did not apply in
> previous levels.
>
> If that is correct, has the potential performance impact
> been evaluated and documented somewhere? And, if
> correct, the change proposal should probably be modified
> to mention the potential performance impacts.
It has a similar impact that turning back on frame pointers would.
Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
I'm extremely displeased now, as the toolchain team basically told us
they wouldn't accept register pressure on x86_64 and then turned
around and made a proposal that does the same thing. Apparently
quality of life improvements for developers and real-time tracing
(e.g. making bpftrace useful) isn't worth it, but this is.
I want a really good justification for not doing both at the same time
if we're going to accept this.
They're only similar to the extent of potentially having a performance
impact. One may improve debugging experience while the other improves
security mitigation coverage by a factor of 2.4x in the average case
and 5-10x in some key cases.
They're apples and butter chicken.
Sid
7:13 a.m.
On Tue, Dec 6, 2022 at 7:50 AM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
On Mon, Dec 5, 2022 at 5:53 PM Neal Gompa <ngompa13(a)gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
> <gary.buhrmaster(a)gmail.com> wrote:
> >
> > On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
> > >
> > >
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
> > >
> >
> > It is my vague recollection (I could easily be wrong, so
> > correct me as appropriate) that _FORTIFY_SOURCE=3
> > adds some runtime overhead that did not apply in
> > previous levels.
> >
> > If that is correct, has the potential performance impact
> > been evaluated and documented somewhere? And, if
> > correct, the change proposal should probably be modified
> > to mention the potential performance impacts.
>
> It has a similar impact that turning back on frame pointers would.
>
> Cf.
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
>
> I'm extremely displeased now, as the toolchain team basically told us
> they wouldn't accept register pressure on x86_64 and then turned
> around and made a proposal that does the same thing. Apparently
> quality of life improvements for developers and real-time tracing
> (e.g. making bpftrace useful) isn't worth it, but this is.
>
> I want a really good justification for not doing both at the same time
> if we're going to accept this.
They're only similar to the extent of potentially having a performance
impact. One may improve debugging experience while the other improves
security mitigation coverage by a factor of 2.4x in the average case
and 5-10x in some key cases.
"may improve" is proven to be "does improve significantly". We had
GNOME and other desktop software developers and hyperscale developers
telling us it would be helpful to have. Entire classes of tracing and
debugging tools *don't work* without frame pointers.
I say that the impact is about equal, just in different areas, with
the same kind of performance hit.
(But who cares about developers, I guess?)
They're apples and butter chicken.
Well, okay then, that's one I hadn't heard before. :P
--
真実はいつも一つ！/ Always, there's only one truth!
7:30 a.m.
On Tue, Dec 06, 2022 at 08:13:51AM -0500, Neal Gompa wrote:
"may improve" is proven to be "does improve
significantly". We had
That is nonsense. Even with -fno-omit-frame-pointers, you can't rely
on frame pointers, they are not accurate in function prologues and epilogues
and they are total garbage e.g. in a lot of functions written in assembly.
The only reliable way to get backtraces is DWARF info or something derived
from it, that is for code emitted by the compilers (with the default
-fasynchronous-unwind-tables) accurate for all instructions and for
hand written assembly one has at least a way to describe that through
.cfi_* directives.
As has been written multiple times, for profiling there doesn't need to be
full DWARF unwinder in the kernel, it is enough that there is something
that can handle the wast majority of cases and punt (copy to userland full
stack) in case of anything unexpected as long as it is rare.
Say on x86-64 and various other targets, the stack pointer, CFA (how to get
caller's stack pointer), frame pointer if any or return address is very
rarely stored anywhere but on the stack, so it should be enough to consider
CFA, sp, bp, ip during unwinding and ignore everything else and from the
harder DW_CFA_* ops (those that need DWARF expression evaluation)
perhaps only pattern recognize the most common ones (say PLT slot, signal
frame).
Frame pointers will never result in anything reliable though, it results
purely in severe performance degradation and false feeling they can be
relied on.
Jakub
11:46 a.m.
On Tue, Dec 06, 2022 at 08:13:51AM -0500, Neal Gompa wrote:
That is nonsense. Even with -fno-omit-frame-pointers, you can't rely
on frame pointers, they are not accurate in function prologues and epilogues
and they are total garbage e.g. in a lot of functions written in assembly.
First of all, https://pagure.io/fesco/issue/2817 is first and foremost about enabling
low-overhead **profiling** of applications (periodic in background or on-demand requested
by users explicitly), not debugging use cases. For debugging use cases GDB might be
perfectly adequate to rely only on DWARF information. But -fno-omit-frame-pointers is to
enable profiling **production workloads** as they happen, because very often
reproducibility of results is impossible without understanding the issue in the first
place, which is what frame pointers are needed for. Even more often you don't even
realize that application is doing something suboptimal unless you profile it continuously,
as it handles *real workload*.
Now, about prologues/epilogues. What percentage of useful workload is spent in those? Tiny
fraction of a percent at best? Even if we don't get accurate stack trace in such cases
it doesn't matter in the grand scheme of things.
As for hand-written assembly functions. I looked at strcmp, memcpy and similar very
frequent and hot functions in glibc. Yes, they don't save %rbp and don't maintain
frame pointers. But they also don't use %rbp register at all, which means **they
don't clobber it**. So if we take stack trace while such function is executing,
we'll still get non-broken stack trace all the way up to the root parent function, we
just won't have leaf-level function in stack traces. That's much better and more
useful than completely broken stack and allows to reason very well about application
performance. Also, almost all fpu-related routines under
sysdeps/x86_64/fpu/multiarch/svml*.S that are using %rbp, are also properly maintaining
frame pointers:
There is a very good reason why Meta enabled -fno-omit-frame-pointers across all its
internal software 5 years ago and never looked back. We rely on frame pointers being
available across millions of machines to drive performance improvements and investigations
saving millions of dollars of real non-hypothetical savings. Google, Netflix, Apple, etc
-- all enable frame pointers due to sheer usefulness of them in practice, either for
performance profiling and/or better real-time introspection of their workloads.
So much for the "nonsense". I realize that not everyone have experience with
tracing production workloads and generally working in profiling area, but I expect people
to keep an open mind and not use double standards when thinking about implications of
system-wide changes like this one or frame pointers one.
The only reliable way to get backtraces is DWARF info or something
derived
from it, that is for code emitted by the compilers (with the default
-fasynchronous-unwind-tables) accurate for all instructions and for
hand written assembly one has at least a way to describe that through
.cfi_* directives.
As has been written multiple times, for profiling there doesn't need to be
full DWARF unwinder in the kernel, it is enough that there is something
that can handle the wast majority of cases and punt (copy to userland full
stack) in case of anything unexpected as long as it is rare.
Say on x86-64 and various other targets, the stack pointer, CFA (how to get
caller's stack pointer), frame pointer if any or return address is very
rarely stored anywhere but on the stack, so it should be enough to consider
CFA, sp, bp, ip during unwinding and ignore everything else and from the
harder DW_CFA_* ops (those that need DWARF expression evaluation)
perhaps only pattern recognize the most common ones (say PLT slot, signal
frame).
You clearly have never tried to do this in practice. You'd know about the price of
discovering and pre-computing such look up tables, both in terms of memory usage, CPU
usage, complexity and reliability. And then you'd also realize the problem of tracing
short-lived processes that started after profiling started and were discovered upfront. As
for the approach with capturing stack and sending it for post-processing. Beyond just the
overhead of capturing and sending so much data for post-processing, you'd also stop
and wonder what is the right size of stack to capture to handle deep stacks/recursion or
functions with large stack size usage.
DWARF is not a panacea. For production workloads it doesn't even come close as an
satisfactory solution, if employed in isolation.
Frame pointers will never result in anything reliable though, it results
purely in severe performance degradation and false feeling they can be
relied on.
THIS is nonsense, both on degradation and reliability points. Performance engineers and
just typical applications owners laugh at your statement detached from the reality.
>
> Jakub
12:17 p.m.
On Tue, Dec 06, 2022 at 05:46:11PM -0000, Andrii Nakryiko wrote:
Now, about prologues/epilogues. What percentage of useful workload is
spent in those? Tiny fraction of a percent at best? Even if we don't get accurate
stack trace in such cases it doesn't matter in the grand scheme of things.
Depends on how large functions are. If you have lots of small functions,
it can be more than 50% of instructions you get the frame pointer wrong.
As for hand-written assembly functions. I looked at strcmp, memcpy
and similar very frequent and hot functions in glibc.
You haven't looked carefully enough.
find . -name \*.S | xargs grep -l %rbp | xargs grep -L movq.*%rsp.*%rbp
./sysdeps/x86_64/fpu/multiarch/svml_s_tanhf16_core_avx512.S
./sysdeps/x86_64/fpu/multiarch/svml_s_tanhf8_core_avx2.S
./sysdeps/x86_64/fpu/multiarch/svml_s_atanhf16_core_avx512.S
./sysdeps/x86_64/fpu/multiarch/svml_s_atanhf8_core_avx2.S
./sysdeps/x86_64/fpu/svml_d_sincos2_core.S
./sysdeps/x86_64/fpu/svml_s_sincosf4_core.S
./sysdeps/x86_64/addmul_1.S
./sysdeps/x86_64/__longjmp.S
./sysdeps/x86_64/setjmp.S
./sysdeps/x86_64/_mcount.S
./sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
./sysdeps/unix/sysv/linux/x86_64/setcontext.S
./sysdeps/unix/sysv/linux/x86_64/swapcontext.S
./sysdeps/unix/sysv/linux/x86_64/getcontext.S
E.g. mpn_addmul_1 is used by printf, which certainly shows up a lot in
normal workloads. The above cases mean the bp register contains total
garbage if one tries to use it for backtrace purposes. And obviously
glibc isn't the only library with inline assembly out there...
As I said, you can't rely on frame pointers making sense, with unwind info
such as in DWARF you know that in this case rbp is used as a frame pointer
and in this case it is not, use some other register or this offset from
stack pointer etc.
AFAIK systemtap already uses DWARF unwinder in the kernel and as I said, for
the profiling purposes one can use only a small subset of that for the vast
majority of cases.
Jakub
1:12 p.m.
On Tue, Dec 06, 2022 at 05:46:11PM -0000, Andrii Nakryiko wrote:
Depends on how large functions are. If you have lots of small functions,
it can be more than 50% of instructions you get the frame pointer wrong.
You might be technically correct, but if some application is spending 50% of the time
entering and exiting functions, then it's not doing a ton of useful work efficiently
anyways and should be rethought and improved. And profiling data will point this out very
clearly.
Let's just not use these convoluted unrealistic corner cases as a generalized claims
about how frame pointers are useless, ok?
You haven't looked carefully enough.
Oh, I didn't claim I studied glibc's assembly code very carefully for sure. I did
a very similar grepping to yours and found just the same **very few** functions that do
use %rbp.
find . -name \*.S | xargs grep -l %rbp | xargs grep -L
movq.*%rsp.*%rbp
./sysdeps/x86_64/fpu/multiarch/svml_s_tanhf16_core_avx512.S
./sysdeps/x86_64/fpu/multiarch/svml_s_tanhf8_core_avx2.S
./sysdeps/x86_64/fpu/multiarch/svml_s_atanhf16_core_avx512.S
./sysdeps/x86_64/fpu/multiarch/svml_s_atanhf8_core_avx2.S
./sysdeps/x86_64/fpu/svml_d_sincos2_core.S
./sysdeps/x86_64/fpu/svml_s_sincosf4_core.S
./sysdeps/x86_64/addmul_1.S
./sysdeps/x86_64/__longjmp.S
./sysdeps/x86_64/setjmp.S
./sysdeps/x86_64/_mcount.S
./sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
./sysdeps/unix/sysv/linux/x86_64/setcontext.S
./sysdeps/unix/sysv/linux/x86_64/swapcontext.S
./sysdeps/unix/sysv/linux/x86_64/getcontext.S
Out of all of these, we have addmul_1.S as seemingly generic. But grepping around glibc
sources I could find any place that's actually calling into __mpn_addmul_1, so there
must be some more magic happening. I'll believe you that it's used somewhere in
printf family of functions, though I certainly didn't see any mention of
__mpn_addmul_1 in llvm-objdump and llvm-readelf outputs for a sample program that is just
doing printf or snprintf.
But all that aside. You've found 14 files that do use %rbp and again are using this as
a point that frame pointers are useless.
And I'm telling you that **in practice**, in real world applications, they are very
much useful and are relied upon, even if in some circumstances we fail to get frame
pointers. I don't understand how one can honestly can use this line of argument to say
that frame pointers are useless.
And by enabling frame pointers even wider we make them even more useful.
E.g. mpn_addmul_1 is used by printf, which certainly shows up a lot in
normal workloads. The above cases mean the bp register contains total
garbage if one tries to use it for backtrace purposes. And obviously
glibc isn't the only library with inline assembly out there...
As I said, you can't rely on frame pointers making sense, with unwind info
See my replies. Yes, frame pointers might be broken in some rare situations, yes we
don't have 100% success rate capturing stack traces precisely because there is
embedded asm that clobbers %rbp, or binaries and libraries are build with
-fomit-frame-pointers. We are asking to minimize the latter.
And for the former, it's on a case-by-case basis to try to mitigate this, but
ultimately no one expect that stack traces will be captured with 100% success rate. We
want this rate to be as high as possible though. And arguments like yours are not helping,
but also misleading people that are not familiar with the matters at hand, but trust
people like you just because you are "a compiler/toolchain engineer with considerable
experience", while it's clear that compiler/toolchain engineers would rather
spend time arguing about petty low-probability counter-examples than be helpful and see
the problem in a wider context.
such as in DWARF you know that in this case rbp is used as a frame
pointer
and in this case it is not, use some other register or this offset from
stack pointer etc.
AFAIK systemtap already uses DWARF unwinder in the kernel and as I said, for
the profiling purposes one can use only a small subset of that for the vast
majority of cases.
This does not work for any of multitude of BPF-based tools. And even with perf, that
supports very expensive DWARF-based unwinding, this doesn't work very well in
practice. There is evidence even from people commenting in this thread. And all this was
explained at length in https://pagure.io/fesco/issue/2817.
>
> Jakub
12:34 p.m.
Andrii,
copilot to pilot, you are responding to Jakub Jelinek's points, not
Neal's. Jakub is a compiler/toolchain engineer with considerable
experience, so when he talks about compiler technology involved in
tracing execution flow, I am inclined to believe him.
I understand that your experience lies in running (and
tracing/profiling) production applications, but please consider that
your viewpoint may be biased by your experience with existing frame
pointer-based instrumentation. This means that you just know from
experience when your results are solid and when you have to be careful
because of e.g. a particular application's large number of small
prolog/epilog-dominated functions or complex and intensive flow of
memory allocations. Jakub is saying that DWARF is a superior technology
that provide the frame pointer information more reliably, so the real
issue is that frame pointer based infrastructure is already here and
DWARF handling requires more development. I haven't heard anyone
question the solidity of the DWARF approach outlined by Jakub and other
people involved with the toolchain, so I think it is reasonable to
expect that it will get implemented.
Are you actually against using the DWARF approach for technical reasons,
or is your argument based on the practical consideration of what's
available right now?
Very Respectfully
p.k.
On 12/6/22 12:46, Andrii Nakryiko wrote:
> On Tue, Dec 06, 2022 at 08:13:51AM -0500, Neal Gompa wrote:
>
> That is nonsense. Even with -fno-omit-frame-pointers, you can't rely
> on frame pointers, they are not accurate in function prologues and epilogues
> and they are total garbage e.g. in a lot of functions written in assembly.
First of all,
https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpagure....
is first and foremost about enabling low-overhead **profiling** of applications (periodic
in background or on-demand requested by users explicitly), not debugging use cases. For
debugging use cases GDB might be perfectly adequate to rely only on DWARF information. But
-fno-omit-frame-pointers is to enable profiling **production workloads** as they happen,
because very often reproducibility of results is impossible without understanding the
issue in the first place, which is what frame pointers are needed for. Even more often you
don't even realize that application is doing something suboptimal unless you profile
it continuously, as it handles *real workload*.
Now, about prologues/epilogues. What percentage of useful workload is spent in those?
Tiny fraction of a percent at best? Even if we don't get accurate stack trace in such
cases it doesn't matter in the grand scheme of things.
As for hand-written assembly functions. I looked at strcmp, memcpy and similar very
frequent and hot functions in glibc. Yes, they don't save %rbp and don't maintain
frame pointers. But they also don't use %rbp register at all, which means **they
don't clobber it**. So if we take stack trace while such function is executing,
we'll still get non-broken stack trace all the way up to the root parent function, we
just won't have leaf-level function in stack traces. That's much better and more
useful than completely broken stack and allows to reason very well about application
performance. Also, almost all fpu-related routines under
sysdeps/x86_64/fpu/multiarch/svml*.S that are using %rbp, are also properly maintaining
frame pointers:
There is a very good reason why Meta enabled -fno-omit-frame-pointers across all its
internal software 5 years ago and never looked back. We rely on frame pointers being
available across millions of machines to drive performance improvements and investigations
saving millions of dollars of real non-hypothetical savings. Google, Netflix, Apple, etc
-- all enable frame pointers due to sheer usefulness of them in practice, either for
performance profiling and/or better real-time introspection of their workloads.
So much for the "nonsense". I realize that not everyone have experience with
tracing production workloads and generally working in profiling area, but I expect people
to keep an open mind and not use double standards when thinking about implications of
system-wide changes like this one or frame pointers one.
> The only reliable way to get backtraces is DWARF info or something derived
> from it, that is for code emitted by the compilers (with the default
> -fasynchronous-unwind-tables) accurate for all instructions and for
> hand written assembly one has at least a way to describe that through
> .cfi_* directives.
>
> As has been written multiple times, for profiling there doesn't need to be
> full DWARF unwinder in the kernel, it is enough that there is something
> that can handle the wast majority of cases and punt (copy to userland full
> stack) in case of anything unexpected as long as it is rare.
> Say on x86-64 and various other targets, the stack pointer, CFA (how to get
> caller's stack pointer), frame pointer if any or return address is very
> rarely stored anywhere but on the stack, so it should be enough to consider
> CFA, sp, bp, ip during unwinding and ignore everything else and from the
> harder DW_CFA_* ops (those that need DWARF expression evaluation)
> perhaps only pattern recognize the most common ones (say PLT slot, signal
> frame).
You clearly have never tried to do this in practice. You'd know about the price of
discovering and pre-computing such look up tables, both in terms of memory usage, CPU
usage, complexity and reliability. And then you'd also realize the problem of tracing
short-lived processes that started after profiling started and were discovered upfront. As
for the approach with capturing stack and sending it for post-processing. Beyond just the
overhead of capturing and sending so much data for post-processing, you'd also stop
and wonder what is the right size of stack to capture to handle deep stacks/recursion or
functions with large stack size usage.
DWARF is not a panacea. For production workloads it doesn't even come close as an
satisfactory solution, if employed in isolation.
> Frame pointers will never result in anything reliable though, it results
> purely in severe performance degradation and false feeling they can be
> relied on.
THIS is nonsense, both on degradation and reliability points. Performance engineers and
just typical applications owners laugh at your statement detached from the reality.
> Jakub
_______________________________________________
devel mailing list -- devel(a)lists.fedoraproject.org
To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
Fedora Code of Conduct:
https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.fe...
List Guidelines:
https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Ffedorap...
List Archives:
https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.f...
Do not reply to spam, report it:
https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpagure....
1:04 p.m.
Red Hat's desktop performance engineer has repeatedly rejected use of DWARF as
impractical and outlandish, including on this mailing list [1] and most recently at [2].
[1]
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.o...
[2] https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/issues/1437#note_119...
3:28 p.m.
On Tue, Dec 6, 2022 at 7:04 PM Michael Catanzaro via devel
<devel(a)lists.fedoraproject.org> wrote:
Red Hat's desktop performance engineer ....
Since you bring up RH's performance engineer,
have they done performance evaluation on
_FORTIFY_SOURCE=3? And while I understand
that until the eval is reviewed and reproduced no
final results will be available, can they share
any initial preprint results from their work?
4:20 p.m.
No. He probably doesn't even know about this proposal yet, since it was just published
yesterday. This is not the sort of thing that matters for desktop performance, where we
care about orders of magnitude rather than a few percent improvement here or there. Even
if extra bounds checking makes code 10% slower, which seems very unlikely, the benefit of
the extra hardening would still be worth it. _FORTIFY_SOURCE=3 is going to make it harder
to hack Fedora users, converting code execution vulnerabilities into denial of service
vulnerabilities. That's worth a small performance cost. Reasonable educated users will
want to make that trade even if we don't know exactly how much the cost is.
Ordinarily I would not have even felt any need to comment on a proposal like this, except
it comes immediately after the rejection of frame pointers, which leaves us unable to
measure where Fedora performance problems occur without rebuilding the world. People who
care about performance should be *very* upset about that decision. Anyway, the effort that
went into that change proposal has established new expectations for any change that will
impact system performance: the new flags should be benchmarked in an environment where all
Fedora packages have been rebuilt with the new flags, so we can critique the change based
on benchmarks that are not representative of real-world usage and reject it if they show a
2% performance hit, regardless of value to Fedora. If you don't like the idea of
rebuilding all packages with the new flags, then maybe it was a mistake to require
developers to do just that if they want to profile applications successfully.
2:02 a.m.
On 06/12/2022 23:20, Michael Catanzaro via devel wrote:
Even if extra bounds checking makes code 10% slower, which seems very
unlikely, the benefit of the extra hardening would still be worth it. _FORTIFY_SOURCE=3 is
going to make it harder to hack Fedora users, converting code execution vulnerabilities
into denial of service vulnerabilities.
No, it doesn't. 3% maximum is acceptable in this case.
Me and all my friends use mitigations=off. A huge performance penalty
for the sake of potential vulnerabilities that still need to be able to
exploit.
--
Sincerely,
Vitaly Zaitsev (vitaly(a)easycoding.org)
2:53 a.m.
On Wed, Dec 07, 2022 at 09:02:36AM +0100, Vitaly Zaitsev via devel wrote:
On 06/12/2022 23:20, Michael Catanzaro via devel wrote:
> Even if extra bounds checking makes code 10% slower, which seems very unlikely, the
benefit of the extra hardening would still be worth it. _FORTIFY_SOURCE=3 is going to make
it harder to hack Fedora users, converting code execution vulnerabilities into denial of
service vulnerabilities.
No, it doesn't. 3% maximum is acceptable in this case.
Me and all my friends use mitigations=off. A huge performance penalty for
the sake of potential vulnerabilities that still need to be able to exploit.
Don't assume that your insecure usage is typical of Fedora's userbase
as a whole. I doubt most people even know that mitigations=off is a
thing, nor even notice the performance penalty of the mitigations
enough to want to turn it off.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:53 p.m.
Anyway, the effort that
went into that change proposal has established new expectations for any change that will
impact system performance: the new flags should be benchmarked in an environment where
all
Fedora packages have been rebuilt with the new flags, so we can critique the change
based
on benchmarks that are not representative of real-world usage and reject it if they show
a
2% performance hit, regardless of value to Fedora. If you don't like the idea of
rebuilding all packages with the new flags, then maybe it was a mistake to require
developers to do just that if they want to profile applications successfully.
So I was quite upset when I wrote the above mail, seeing a new change proposal that admits
it adds register pressure a few days after the frame pointer proposal was rejected for
adding register pressure. Probably I should not send mails when angry. I don't
actually support requiring the proposal authors to do the above work. `_FORTIFY_SOURCE=3`
seems certain to make Fedora users safer. Regardless of what happened to the frame pointer
proposal, it wouldn't make sense to block it from benefiting Fedora users. If we were
politicians, then it would totally make sense to block Other Team's proposal unless
Our Team's proposal gets accepted. (I support Team Frame Pointer. ;) But Fedora change
proposals should not operate that way. (We all support Team Fedora here, after all.) This
should go through.
One more note regarding frame pointers. The `_FORTIFY_SOURCE=3` change proposal says
"Per-application performance benchmarks may be useful in understanding the impact for
those specific use cases." But this is not useful at all without an actionable way to
figure out where performance problems are occurring, which requires functional profiling
tools, which require frame pointers. Thank you _very much_ Neal, Fabio, and Zbigniew for
your efforts to revisit that decision.
2:06 a.m.
On 14/12/2022 00:53, Michael Catanzaro via devel wrote:
Thank you _very much_ Neal, Fabio, and Zbigniew for your efforts to
revisit that decision.
This proposal was rejected and you don't like it. So please please stop
attacking other people.
--
Sincerely,
Vitaly Zaitsev (vitaly(a)easycoding.org)
1:34 p.m.
Andrii,
copilot to pilot, you are responding to Jakub Jelinek's points, not
Copilot? Pilot? I don't understand this euphemism. And yes, I'm well aware who
I'm replying to, thank you.
Neal's. Jakub is a compiler/toolchain engineer with considerable
And not sure why you are implying that Neal is somehow less qualified than Jakub?..
experience, so when he talks about compiler technology involved in
tracing execution flow, I am inclined to believe him.
Up to you who to believe and for what reason. I'm inclined to argue based on facts and
what people are saying, not based on their alleged qualifications or reputation.
I understand that your experience lies in running (and
tracing/profiling) production applications, but please consider that
your viewpoint may be biased by your experience with existing frame
pointer-based instrumentation. This means that you just know from
experience when your results are solid and when you have to be careful
because of e.g. a particular application's large number of small
prolog/epilog-dominated functions or complex and intensive flow of
memory allocations. Jakub is saying that DWARF is a superior technology
that provide the frame pointer information more reliably, so the real
issue is that frame pointer based infrastructure is already here and
DWARF handling requires more development. I haven't heard anyone
question the solidity of the DWARF approach outlined by Jakub and other
people involved with the toolchain, so I think it is reasonable to
expect that it will get implemented.
I'm biased towards looking at real world experiences, instead of using hypotheticals
and some particular low-probability corner cases to make a misleading generalized
statements like "Even with -fno-omit-frame-pointers, you can't rely on frame
pointers". In practice, yes we can, and yes we do, across millions of machines, tons
of various applications. They are reliable enough to drive multi-million efficiency wins
done by large amount of engineers (and they don't even have to be performance experts
to get a lot of use from frame pointer-based profiling data).
Are you actually against using the DWARF approach for technical reasons,
or is your argument based on the practical consideration of what's
available right now?
Technical reasons, and it can't be mitigated with better tooling support. DWARF-based
stack unwinding doesn't work well in practice and is very expensive, to the point that
we can't and don't use it in practice for fleet-wide profiling. There are many
technical reasons why this approach is not adequate. I'm not questioning of usefulness
of DWARF data, I'm saying for profiling production workloads this is not appropriate
alternative to frame pointers. Many people, both in this thread and in
https://pagure.io/fesco/issue/2817 and related mailing discussions agree with me, coming
from different backgrounds and environments.
Very Respectfully
p.k.
6:46 a.m.
On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton <bcotton(a)redhat.com> wrote:
>
>
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
>
It is my vague recollection (I could easily be wrong, so
correct me as appropriate) that _FORTIFY_SOURCE=3
adds some runtime overhead that did not apply in
previous levels.
If that is correct, has the potential performance impact
been evaluated and documented somewhere? And, if
correct, the change proposal should probably be modified
to mention the potential performance impacts.
There is indeed a theoretical concern over performance due to size
expressions vs constants, but none have been reported in practice.
OpenSUSE and Gentoo (at least) have had _FORTIFY_SOURCE=3 enabled by
default for nearly a year and there haven't seen any reports of
performance degradation. Besides, the magnitude of mitigation
coverage is *immense*. For example with bash, where only 3% of the
calls were fortified, now nearly half of the calls are fortified.
Likewise, sudo has gone from about 1% to nearly half.
Note that it doesn't mean that all those new calls have an additional
overhead; the compiler and glibc can also detect which of these
accesses are always safe and it simplifies the calls to the regular
ones. Overall even if there is a miniscule performance overhead, I
reckon the reward is much higher. Just ask the folks over at
OpenSUSE, they've uncovered a bunch of bugs over the last year thanks
to this feature.
I did a code size analysis though (since it's a much clearler problem
to analyze) and funnily, _FORTIFY_SOURCE=3 ended up *reducing* code
size by a tiny bit on average. Very few packages saw code size
increases beyond 1%, most were in the nearly negligible range. The
numbers are in the Google spreadsheet I linked to in the proposal,
under the "size summary" tab.
Sid
9:05 a.m.
On Tue, Dec 6, 2022 at 12:47 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
Overall even if there is a miniscule performance overhead, I
reckon the reward is much higher.
I am curious how you can claim there is only a
minuscule performance overhead without doing
any benchmarks?
I am not claiming the overheads are high, I
don't know, but I know enough to trust that
when you (yourself) said in the article:
"We need a proper study of performance
and code size to understand the magnitude
of the impact"
that you are making a very clear statement
that you believe that such a study is needed.
If you do not believe that that statement
is correct, will you be issuing a correction
to the posted article saying that such a study
is no longer needed?
And without any benchmarks, the proposal
should acknowledge explicitly that some
unknown performance penalty may be seen,
and needs to be accepted as a trade-off for
security reasons (some may be fine with that,
others less so, but that is a different debate
to have).
9:15 a.m.
On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
On Tue, Dec 6, 2022 at 12:47 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
> Overall even if there is a miniscule performance overhead, I
> reckon the reward is much higher.
I am curious how you can claim there is only a
minuscule performance overhead without doing
any benchmarks?
I am not claiming the overheads are high, I
don't know, but I know enough to trust that
when you (yourself) said in the article:
"We need a proper study of performance
and code size to understand the magnitude
of the impact"
that you are making a very clear statement
that you believe that such a study is needed.
If you do not believe that that statement
is correct, will you be issuing a correction
to the posted article saying that such a study
is no longer needed?
My full comment in that blog post is:
"We need a proper study of performance and code size to understand the
magnitude of the impact created by _FORTIFY_SOURCE=3 additional
runtime code generation. However the performance and code size
overhead may well be worth it due to the magnitude of improvement in
security coverage."
To elaborate, I think the performance and code size study is an
interesting academic concern, but the magnitude of improvement
justifies whatever little performance impact this may introduce and it
should not be a blocker for the improvement.
And without any benchmarks, the proposal
should acknowledge explicitly that some
unknown performance penalty may be seen,
and needs to be accepted as a trade-off for
security reasons (some may be fine with that,
others less so, but that is a different debate
to have).
Sure, I could add that as a comment in the proposal.
Thanks,
Sid
9:25 a.m.
On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
My full comment in that blog post is:
"We need a proper study of performance and code size to understand the
magnitude of the impact created by _FORTIFY_SOURCE=3 additional
runtime code generation. However the performance and code size
overhead may well be worth it due to the magnitude of improvement in
security coverage."
The key word is *MAY*. That is not considered
to be a conclusion supported by the evidence
presented (at least in any scientific paper I
have reviewed).
9:41 a.m.
On Tue, Dec 6, 2022 at 10:26 AM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
> My full comment in that blog post is:
>
> "We need a proper study of performance and code size to understand the
> magnitude of the impact created by _FORTIFY_SOURCE=3 additional
> runtime code generation. However the performance and code size
> overhead may well be worth it due to the magnitude of improvement in
> security coverage."
The key word is *MAY*. That is not considered
to be a conclusion supported by the evidence
presented (at least in any scientific paper I
have reviewed).
I have added a performance note[1] in the proposal.
Thanks,
Sid
[1]
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
3:40 p.m.
On Tue, Dec 6, 2022 at 10:41 AM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
On Tue, Dec 6, 2022 at 10:26 AM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
>
> On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar <siddhesh(a)redhat.com>
wrote:
>
> > My full comment in that blog post is:
> >
> > "We need a proper study of performance and code size to understand the
> > magnitude of the impact created by _FORTIFY_SOURCE=3 additional
> > runtime code generation. However the performance and code size
> > overhead may well be worth it due to the magnitude of improvement in
> > security coverage."
>
> The key word is *MAY*. That is not considered
> to be a conclusion supported by the evidence
> presented (at least in any scientific paper I
> have reviewed).
I have added a performance note[1] in the proposal.
SPEC2000 and SPEC2017 results with _FORTIFY_SOURCE=2 vs
_FORTIFY_SOURCE=3 show practically no difference in performance. I
have updated the wiki to note this and the fact that this should
alleviate any concerns of a general slowdown. However I do request
package maintainers to report any slowdown they experience due to
building with _FORTIFY_SOURCE=3 so that we get a better understanding.
Always happy to help keep performance up to par even as we improve
security mitigations.
Thanks,
Sid
11:55 a.m.
On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
<gary.buhrmaster(a)gmail.com> wrote:
My full comment in that blog post is:
"We need a proper study of performance and code size to understand the
magnitude of the impact created by _FORTIFY_SOURCE=3 additional
runtime code generation. However the performance and code size
overhead may well be worth it due to the magnitude of improvement in
security coverage."
To elaborate, I think the performance and code size study is an
interesting academic concern, but the magnitude of improvement
justifies whatever little performance impact this may introduce and it
should not be a blocker for the improvement.
It's interesting how now it's just an academic concern. Please hold yourself to at
least the standards set for https://pagure.io/fesco/issue/2817 in terms of benchmarking
and persuading everyone that performance degradation across entire ecosystem is not a
concern.
Sure, I could add that as a comment in the proposal.
We need benchmarks, not a comment. Thank you.
>
> Thanks,
> Sid
1:01 p.m.
On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
<andrii.nakryiko(a)gmail.com> wrote:
> On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
> <gary.buhrmaster(a)gmail.com> wrote:
>
> My full comment in that blog post is:
>
> "We need a proper study of performance and code size to understand the
> magnitude of the impact created by _FORTIFY_SOURCE=3 additional
> runtime code generation. However the performance and code size
> overhead may well be worth it due to the magnitude of improvement in
> security coverage."
>
> To elaborate, I think the performance and code size study is an
> interesting academic concern, but the magnitude of improvement
> justifies whatever little performance impact this may introduce and it
> should not be a blocker for the improvement.
It's interesting how now it's just an academic concern. Please hold yourself to
at least the standards set for https://pagure.io/fesco/issue/2817 in terms of benchmarking
and persuading everyone that performance degradation across entire ecosystem is not a
concern.
I don't think the two are comparable at all, neither in terms of
potential performance impact (register pressure across an entire
program vs at specific API call points in some unique cases) nor in
terms of the benefits it provides.
Thanks,
Sid
1:37 p.m.
On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
<andrii.nakryiko(a)gmail.com> wrote:
I don't think the two are comparable at all, neither in terms of
potential performance impact (register pressure across an entire
program vs at specific API call points in some unique cases) nor in
terms of the benefits it provides.
It's irrelevant if they are comparable. This is a system-wide change that has
potential to cause performance regression. By how much -- not clear. Hand-waiving such
concerns is extremely disappointing in the face of the scrutiny given to
https://pagure.io/fesco/issue/2817. So, please get inspiration from that proposal and do
benchmarks.
>
> Thanks,
> Sid
3:41 p.m.
On Wed, Dec 7, 2022 at 3:15 PM Andrii Nakryiko
<andrii.nakryiko(a)gmail.com> wrote:
> On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
> <andrii.nakryiko(a)gmail.com> wrote:
>
> I don't think the two are comparable at all, neither in terms of
> potential performance impact (register pressure across an entire
> program vs at specific API call points in some unique cases) nor in
> terms of the benefits it provides.
It's irrelevant if they are comparable. This is a system-wide change that has
potential to cause performance regression. By how much -- not clear. Hand-waiving such
concerns is extremely disappointing in the face of the scrutiny given to
https://pagure.io/fesco/issue/2817. So, please get inspiration from that proposal and do
benchmarks.
You're basically making a political statement, so I'll just leave that
bit for FESCO to deal with.
Thanks,
Sid
11:26 a.m.
On Wed, Dec 07, 2022 at 04:41:09PM -0500, Siddhesh Poyarekar wrote:
On Wed, Dec 7, 2022 at 3:15 PM Andrii Nakryiko
<andrii.nakryiko(a)gmail.com> wrote:
>
> > On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
> > <andrii.nakryiko(a)gmail.com> wrote:
> >
> > I don't think the two are comparable at all, neither in terms of
> > potential performance impact (register pressure across an entire
> > program vs at specific API call points in some unique cases) nor in
> > terms of the benefits it provides.
>
> It's irrelevant if they are comparable. This is a system-wide change that has
potential to cause performance regression. By how much -- not clear. Hand-waiving such
concerns is extremely disappointing in the face of the scrutiny given to
https://pagure.io/fesco/issue/2817. So, please get inspiration from that proposal and do
benchmarks.
You're basically making a political statement, so I'll just leave that
bit for FESCO to deal with.
Right. I'll say right away that I will NOT ask for an additional test
mass rebuild and benchmarks. The size measurements that were posted in
the google docs spreadsheet are enough for me. The size change is
negative for most packages, and this generally means that the
execution time will not go up. (To get noticeable slowdowns the dynamic
checks would need to be called often, and the instructions for a call
are fairly verbose, so a large number of added runtime checks would
have to be visible in the instruction count. When we get a smaller code
size, this strongly implies that the compiler was able to deduce that
the constrains are satisfied in more cases and actually remove
checks. I know this is not a "proof", but I expect those expectation
to be confirmed after we have done the mass rebuild.)
There are some packages for which runtime might go up.
But this is not a blocker: either they can be "fixed" to avoid the
issue, or they can opt out, or we can just ignore the difference
because for most programs a slowdown of 10% is completely irrelevant.
So for *this* change, I think the Owner has provided enough information
and justification and I'll be voting +1. We could ask the Owner to
do much more benchmarking, but it wouldn't really do much except burn
their time.
That said, I think the same reasoning applies to -fno-omit-framepointer.
The Owners of *that* change provided benchmarking that showed that
the impact is negligible for most packages, and if it were to turn out
that there are some packages which are noticeably negatively affected,
the same fix-or-opt-out-or-ignore trifecta would be applicable.
I disagree with the decision of -fno-omit-framepointer and hope we can
revisit it. Nevertheless, that is not a reason to block this change.
The bar was raised unreasonably high for one proposal, but instead of
trying to treat the next proposal the same and block it this way,
let's just return the bar to a reasonable height.
Zbyszek
6:35 p.m.
Hi,
This is underwhelming and I have several questions inline
On 12/5/22 20:58, Ben Cotton wrote:
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
This document represents a proposed Change. As part of the Changes
process, proposals are publicly announced in order to receive
community feedback. This proposal will only be implemented if approved
by the Fedora Engineering Steering Committee.
== Summary ==
Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
improve mitigation of security issues arising from buffer overflows in
packages in Fedora.
== Owner ==
* Name: [[User:siddhesh| Siddhesh Poyarekar]]
* Email:sipoyare@redhat.com
== Detailed Description ==
Default C and C++ compiler flags to build packages in Fedora currently
includes `-Wp,-D_FORTIFY_SOURCE=2`, which enables fortification of
some functions in glibc, thus providing some mitigation against buffer
overflows. Since glibc 2.34 and GCC 12, there has been a new
fortification level (`_FORTIFY_SOURCE=3`) which improves the coverage
of this mitigation.
The core change to bring in this mitigation is to change the default
build flags in `redhat-rpm-config` so that packages build by default
with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
that do not interact well with `_FORTIFY_SOURCE` and will also need a
workaround to downgrade fortification to level 2. The change will also
include this override.
How come systemd gets an exception? If it is a security option, it
should be enabled everywhere.
I do not see benefit in a security change that ignores PID 1 process,
If the feature, on the GCC side, is not 100% done.
How do I tell a difference of a bug with the _FORTIFY_SOURCE which I
will ignore and a bug with my package?
I do not have the knowledge or the time to be able to say that GCC
generated the wrong machine code and therefore it is not a bug with my
package.
If my program was not complaining before the change and is now
complaining with the change, I am opting out of the change, and filing a
bug against GCC on Fedora.
I assume that by the package providing the exception, packagers get to
choose themselves and we do not need to go through FESCO
to disable a security feature?
== Benefit to Fedora ==
[https://docs.google.com/spreadsheets/d/1nPSmbEf3HVB91zI8yBraMqVry3_ILmlV2...
Analysis of packages] in Fedora rawhide indicate that the improvement
of mitigation coverage is on average over 2.4x, in some cases
protecting more than half of the fortified glibc calls in the target
application.
This change will thus harden Fedora to a significant extent, thus
making it a more secure distribution out of the box.
1) Is there some complete source for all these findings? If google
sheets cannot handle all the "raw data" as noted in the comment,
maybe it is the wrong medium.
2) What does *anything* on that google sheet mean. I have managed to
figure out, from the article, that bos and bdos correspond to level 2
and 3 of _FORTIFY_SOURCE.
However, total of /.*/? Violated accesses? Segfaults? Then followed by
"Sum of total". For rubygem-ffi, this reaches into hundreds while
"bdos"
is 2. That is the only sum I can make, with the data provided.
I am no wiser from looking at it, what do the data mean?
3) I cannot speak to much else than Ruby, I do not see ruby in neither
the failures or "All x86_64". Should we attempt to test it ourselves?
Please provide a proper "how to test" section, I cannot fix what I
cannot test or compare results when I have no idea what I am seeing.
Actually, last time I heard about number of packages, it was around 50k
(not source, build result), and as I stated,
Ruby is missing, and so are quite a few dependent packages that should
have GCC involved somewhere:
~~~
$ dnf repoquery --whatrequires "*libruby.so*" --disablerepo="*"
--enablerepo="fedora" 2>/dev/null | grep -v "i686" | uniq | wc -l
115
~~~
If we also filter rubygem-* packages that depend on the *libruby.so*
(and most probably contain a binary extension written in C/C++ that
links to Ruby), I get 68 packages.
When I search "All x86_64" for "ruby" I get 28 packages. That is...
not
adding up.
== Scope ==
* Proposal owners: Post a merge request to redhat-rpm-config with the
actual change to build flags.
* Other developers:
Resolve bugs filed for build failures, either by fixing the bug
exposed by `_FORTIFY_SOURCE=3` or by disabling `_FORTIFY_SOURCE=3` for
the package if it is a false positive or if the package is unable to
adapt to the change.
* Release engineering: Mass rebuild required
* Policies and guidelines: Guidelines should include workaround for
packages that fail to build with `-Wp,-D_FORTIFY_SOURCE=3` due to a
false positive.
I'll just ask, what is a false positive. How can I tell. What
are the
steps for this.
* Trademark approval: N/A (not needed for this Change)
== Upgrade/compatibility impact ==
No ABI change, so there should be no impact on compatibility in a
mixed environment.
== How To Test ==
* Smoke testing of packages to ensure that they continue to work
correctly. Some packages may have overflows exposed at runtime, which
may need to be fixed.
This does not describe how to test, e.g. ahead of time, just
that the
packagers are to deal with the fallout...
== User Experience ==
No noticeable change to users.
I beg to differ. From what I am able to tell, from the limited provided
sources, packages can have a runtime crash.
A LOT of GUI packages use C or the likes that respond to the flags of
"fortify source".
This can have a serious impact to users by the recompiled packages:
* not suddenly working, package's tests passed, but user triggered just
the right code branch
* performance impacted due to overhead.
== Dependencies ==
None.
== Contingency Plan ==
* Contingency mechanism: (What to do? Who will do it?) If too many
packages are found to be broken at runtime, the default for
fortification will be left at `_FORTIFY_SOURCE=2` for Fedora 38.
Change owner will do this in `redhat-rpm-config`
* Contingency deadline: Beta freeze
* Blocks release? Yes
* Blocks product? No
== Documentation ==
[https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
More context on `_FORTIFY_SOURCE=3` improvements].
== Release Notes ==
Regards,
Jarek Prokop
2:59 a.m.
On Tue, Dec 06, 2022 at 01:35:04AM +0100, Jaroslav Prokop wrote:
On 12/5/22 20:58, Ben Cotton wrote:
The core change to bring in this mitigation is to change the default
build flags in `redhat-rpm-config` so that packages build by default
with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
that do not interact well with `_FORTIFY_SOURCE` and will also need a
workaround to downgrade fortification to level 2. The change will also
include this override.
How come systemd gets an exception? If it is a security option, it should be
enabled everywhere.
I don't believe the proposal is that everyone *has* to use this (or at
least, I hope not). Even existing _FORTIFY_SOURCE=2 is optional. I'd
like to know what the problems are that affect systemd however.
I do not see benefit in a security change that ignores PID 1 process,
I agree we should try to cover it.
If the feature, on the GCC side, is not 100% done.
How do I tell a difference of a bug with the _FORTIFY_SOURCE which I will
ignore and a bug with my package?
By looking at the message printed out when the program crashes, I
guess? And if that's not enough information, then asking here.
I do not have the knowledge or the time to be able to say that GCC
generated the wrong machine code and therefore it is not a bug with
my package. If my program was not complaining before the change and
is now complaining with the change, I am opting out of the change,
and filing a bug against GCC on Fedora.
GCC & Fedora developers have been very responsive on these kinds of
issues in the past. No one wants a compiler with code gen problems.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
3:08 a.m.
On Tue, Dec 06, 2022 at 08:59:03AM +0000, Richard W.M. Jones wrote:
I don't believe the proposal is that everyone *has* to use this
(or at
least, I hope not). Even existing _FORTIFY_SOURCE=2 is optional. I'd
like to know what the problems are that affect systemd however.
It's mentioned in this document:
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
_FORTIFY_SOURCE=3 revealed another pattern. Applications such as
systemd used malloc_usable_size to determine available space in
objects and then used the residual space. The glibc manual
discourages this type of usage, dictating that malloc_usable_size is
for diagnostic purposes only. But applications use the function as a
hack to avoid reallocating buffers when there is space in the
underlying malloc chunk. The implementation of malloc_usable_size
needs to be fixed to return the allocated object size instead of the
chunk size in non-diagnostic use. Alternatively, another solution is
to deprecate the function. But that is a topic for discussion by the
glibc community.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
3:25 a.m.
On 12/6/22 10:08, Richard W.M. Jones wrote:
On Tue, Dec 06, 2022 at 08:59:03AM +0000, Richard W.M. Jones wrote:
> I don't believe the proposal is that everyone *has* to use this (or at
> least, I hope not). Even existing _FORTIFY_SOURCE=2 is optional. I'd
> like to know what the problems are that affect systemd however.
It's mentioned in this document:
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-...
_FORTIFY_SOURCE=3 revealed another pattern. Applications such as
systemd used malloc_usable_size to determine available space in
objects and then used the residual space. The glibc manual
discourages this type of usage, dictating that malloc_usable_size is
for diagnostic purposes only. But applications use the function as a
hack to avoid reallocating buffers when there is space in the
underlying malloc chunk. The implementation of malloc_usable_size
needs to be fixed to return the allocated object size instead of the
chunk size in non-diagnostic use. Alternatively, another solution is
to deprecate the function. But that is a topic for discussion by the
glibc community.
Rich.
Thanks for sharing. I missed that one.
Jarek
7:21 a.m.
On Tue, Dec 6, 2022 at 4:05 AM Richard W.M. Jones <rjones(a)redhat.com> wrote:
On Tue, Dec 06, 2022 at 01:35:04AM +0100, Jaroslav Prokop wrote:
> On 12/5/22 20:58, Ben Cotton wrote:
>
> The core change to bring in this mitigation is to change the default
> build flags in `redhat-rpm-config` so that packages build by default
> with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
> that do not interact well with `_FORTIFY_SOURCE` and will also need a
> workaround to downgrade fortification to level 2. The change will also
> include this override.
>
> How come systemd gets an exception? If it is a security option, it should be
> enabled everywhere.
I don't believe the proposal is that everyone *has* to use this (or at
least, I hope not). Even existing _FORTIFY_SOURCE=2 is optional. I'd
like to know what the problems are that affect systemd however.
Yes, I intend it to be the same as _FORTIFY_SOURCE=2. In fact, I'm
thinking of a %fortify_level macro override that allows packages to
override this without fiddling directly with cflags.
Thanks,
Sid
7:14 a.m.
On Mon, Dec 5, 2022 at 7:35 PM Jaroslav Prokop <jar.prokop(a)volny.cz> wrote:
Default C and C++ compiler flags to build packages in Fedora
currently
includes `-Wp,-D_FORTIFY_SOURCE=2`, which enables fortification of
some functions in glibc, thus providing some mitigation against buffer
overflows. Since glibc 2.34 and GCC 12, there has been a new
fortification level (`_FORTIFY_SOURCE=3`) which improves the coverage
of this mitigation.
The core change to bring in this mitigation is to change the default
build flags in `redhat-rpm-config` so that packages build by default
with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
that do not interact well with `_FORTIFY_SOURCE` and will also need a
workaround to downgrade fortification to level 2. The change will also
include this override.
How come systemd gets an exception? If it is a security option, it should be enabled
everywhere.
It's an unfortunate (although hopefully temporary) exclusion, see this
issue for more context:
https://github.com/systemd/systemd/issues/22801
In summary, systemd uses malloc_usable_size in a way that it's
discouraged, which results in behaviour that's undefined by the
standard, which is now caught out by the compiler. We're hoping to
convince the systemd community to fix this so that we can actually
build it with fortification.
I do not see benefit in a security change that ignores PID 1 process,
Do you mean that factually or rhetorically? I've shared statistics of
improvement in the proposal, surely improvements to packages like
bash, sudo and coreutils would count for something :)
If the feature, on the GCC side, is not 100% done.
How do I tell a difference of a bug with the _FORTIFY_SOURCE which I will ignore and a
bug with my package?
_FORTIFY_SOURCE=3 has been enabled on OpenSUSE for nearly a year. If
it causes a crash in your package, it's extremely likely that your
package has a bug.
I do not have the knowledge or the time to be able to say that GCC
generated the wrong machine code and therefore it is not a bug with my package.
If my program was not complaining before the change and is now complaining with the
change, I am opting out of the change, and filing a bug against GCC on Fedora.
I assume that by the package providing the exception, packagers get to choose themselves
and we do not need to go through FESCO
to disable a security feature?
I think they should have to, but that's something for FESCO to decide.
_FORTIFY_SOURCE is not a new feature, the new level is just better at
catching bugs. Significantly better.
== Benefit to Fedora ==
[https://docs.google.com/spreadsheets/d/1nPSmbEf3HVB91zI8yBraMqVry3_ILmlV2...
Analysis of packages] in Fedora rawhide indicate that the improvement
of mitigation coverage is on average over 2.4x, in some cases
protecting more than half of the fortified glibc calls in the target
application.
This change will thus harden Fedora to a significant extent, thus
making it a more secure distribution out of the box.
1) Is there some complete source for all these findings? If google sheets cannot handle
all the "raw data" as noted in the comment,
maybe it is the wrong medium.
I've literally posted the raw data in the sheets, please take a look.
2) What does *anything* on that google sheet mean. I have managed to
figure out, from the article, that bos and bdos correspond to level 2 and 3 of
_FORTIFY_SOURCE.
However, total of /.*/? Violated accesses? Segfaults? Then followed by "Sum of
total". For rubygem-ffi, this reaches into hundreds while "bdos" is 2. That
is the only sum I can make, with the data provided.
I am no wiser from looking at it, what do the data mean?
I've mentioned this in the article, but it's a compile-time statistic
of the number of success of __builtin_object_size vs
__builtin_dynamic_object_size, using the fortify-metrics plugin:
https://github.com/siddhesh/fortify-metrics
The numbers you should be looking at are the "coverage" columns, which
tells you what percentage of the calls were successful. For
rubygem-ffi, the fortification is negligible regardless of whether it
is built at _FORTIFY_SOURCE=2 or _FORTIFY_SOURCE=3. This means that
it is unaffected by this change.
3) I cannot speak to much else than Ruby, I do not see ruby in
neither the failures or "All x86_64". Should we attempt to test it ourselves?
That's odd, thanks for pointing out.
Please provide a proper "how to test" section, I cannot fix
what I cannot test or compare results when I have no idea what I am seeing.
Actually, last time I heard about number of packages, it was around 50k (not source,
build result), and as I stated,
Ruby is missing, and so are quite a few dependent packages that should have GCC involved
somewhere:
~~~
$ dnf repoquery --whatrequires "*libruby.so*" --disablerepo="*"
--enablerepo="fedora" 2>/dev/null | grep -v "i686" | uniq | wc -l
115
~~~
If we also filter rubygem-* packages that depend on the *libruby.so* (and most probably
contain a binary extension written in C/C++ that links to Ruby), I get 68 packages.
When I search "All x86_64" for "ruby" I get 28 packages. That is...
not adding up.
Yeah, I have missed packages; I had looked at revdeps glibc-devel,
which turned out around 9k packages, of which 6k had any
_FORTIFY_SOURCE use at all. I can redo with *all* packages and
generate a report.
== Scope ==
* Proposal owners: Post a merge request to redhat-rpm-config with the
actual change to build flags.
* Other developers:
Resolve bugs filed for build failures, either by fixing the bug
exposed by `_FORTIFY_SOURCE=3` or by disabling `_FORTIFY_SOURCE=3` for
the package if it is a false positive or if the package is unable to
adapt to the change.
* Release engineering: Mass rebuild required
* Policies and guidelines: Guidelines should include workaround for
packages that fail to build with `-Wp,-D_FORTIFY_SOURCE=3` due to a
false positive.
I'll just ask, what is a false positive. How can I tell. What are the steps for this.
If _FORTIFY_SOURCE causes crashes in your package, it's most likely a
bug in your package. In the rare case of a false positive (or even if
in doubt or if you can't figure out what the bug is), cc me and I'll
help you determine what to do there.
* Trademark approval: N/A (not needed for this Change)
== Upgrade/compatibility impact ==
No ABI change, so there should be no impact on compatibility in a
mixed environment.
== How To Test ==
* Smoke testing of packages to ensure that they continue to work
correctly. Some packages may have overflows exposed at runtime, which
may need to be fixed.
This does not describe how to test, e.g. ahead of time, just that the packagers are to
deal with the fallout...
cc me, I'll be happy to help you out. I did a rebuild of nearly 10k
packages and there were few failures (see the failures tab in the
doc). I'll expand that list to make sure that the pain on maintainers
is minimal.
== User Experience ==
No noticeable change to users.
I beg to differ. From what I am able to tell, from the limited provided sources, packages
can have a runtime crash.
A LOT of GUI packages use C or the likes that respond to the flags of "fortify
source".
This can have a serious impact to users by the recompiled packages:
* not suddenly working, package's tests passed, but user triggered just the right
code branch
* performance impacted due to overhead.
I don't think that's the intended definition of "User Experience"
here, but I'll be happy to add to it if needed.
Thanks,
Sid
8:55 a.m.
On Tue, Dec 6, 2022 at 8:14 AM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
> Please provide a proper "how to test" section, I
cannot fix what I cannot test or compare results when I have no idea what I am seeing.
>
> Actually, last time I heard about number of packages, it was around 50k (not source,
build result), and as I stated,
> Ruby is missing, and so are quite a few dependent packages that should have GCC
involved somewhere:
> ~~~
> $ dnf repoquery --whatrequires "*libruby.so*" --disablerepo="*"
--enablerepo="fedora" 2>/dev/null | grep -v "i686" | uniq | wc -l
> 115
> ~~~
>
> If we also filter rubygem-* packages that depend on the *libruby.so* (and most
probably contain a binary extension written in C/C++ that links to Ruby), I get 68
packages.
> When I search "All x86_64" for "ruby" I get 28 packages. That
is... not adding up.
Yeah, I have missed packages; I had looked at revdeps glibc-devel,
which turned out around 9k packages, of which 6k had any
_FORTIFY_SOURCE use at all. I can redo with *all* packages and
generate a report.
I checked again and it looks like ruby had failed in that mass
rebuild[1] but I can't see the logs anymore; they've probably been
garbage collected. I reckon it was one of those with a transient
failure that I reran by hand later. I'm going to run a full rebuild
anyway (should take about 8 days, about 23k source packages AFAICT)
and will share the results here.
Thanks,
Sid
[1] https://copr.fedorainfracloud.org/coprs/siddhesh/mpb.41/builds/
9:03 a.m.
On Tue, Dec 6, 2022 at 9:55 AM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
On Tue, Dec 6, 2022 at 8:14 AM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
> > Please provide a proper "how to test" section, I cannot fix what I
cannot test or compare results when I have no idea what I am seeing.
> >
> > Actually, last time I heard about number of packages, it was around 50k (not
source, build result), and as I stated,
> > Ruby is missing, and so are quite a few dependent packages that should have GCC
involved somewhere:
> > ~~~
> > $ dnf repoquery --whatrequires "*libruby.so*"
--disablerepo="*" --enablerepo="fedora" 2>/dev/null | grep -v
"i686" | uniq | wc -l
> > 115
> > ~~~
> >
> > If we also filter rubygem-* packages that depend on the *libruby.so* (and most
probably contain a binary extension written in C/C++ that links to Ruby), I get 68
packages.
> > When I search "All x86_64" for "ruby" I get 28 packages.
That is... not adding up.
>
> Yeah, I have missed packages; I had looked at revdeps glibc-devel,
> which turned out around 9k packages, of which 6k had any
> _FORTIFY_SOURCE use at all. I can redo with *all* packages and
> generate a report.
I checked again and it looks like ruby had failed in that mass
rebuild[1] but I can't see the logs anymore; they've probably been
garbage collected. I reckon it was one of those with a transient
failure that I reran by hand later. I'm going to run a full rebuild
anyway (should take about 8 days, about 23k source packages AFAICT)
and will share the results here.
To be clear, ruby *builds* successfully[1] with _FORTIFY_SOURCE=3, it
just didn't build successfully when running the metrics.
Sid
[1] https://copr.fedorainfracloud.org/coprs/siddhesh/mpb.41/builds/
10:07 a.m.
Hi,
first off, sorry if my write-up seemed a bit harsh, this was the last
time I am trying to respond to a change proposal late at night :).
On 12/6/22 14:14, Siddhesh Poyarekar wrote:
On Mon, Dec 5, 2022 at 7:35 PM Jaroslav
Prokop<jar.prokop(a)volny.cz> wrote:
> Default C and C++ compiler flags to build packages in Fedora currently
> includes `-Wp,-D_FORTIFY_SOURCE=2`, which enables fortification of
> some functions in glibc, thus providing some mitigation against buffer
> overflows. Since glibc 2.34 and GCC 12, there has been a new
> fortification level (`_FORTIFY_SOURCE=3`) which improves the coverage
> of this mitigation.
>
> The core change to bring in this mitigation is to change the default
> build flags in `redhat-rpm-config` so that packages build by default
> with `-Wp,-D_FORTIFY_SOURCE=3`. There are packages (e.g. `systemd`)
> that do not interact well with `_FORTIFY_SOURCE` and will also need a
> workaround to downgrade fortification to level 2. The change will also
> include this override.
>
> How come systemd gets an exception? If it is a security option, it should be enabled
everywhere.
It's an unfortunate (although hopefully temporary) exclusion, see this
issue for more context:
https://github.com/systemd/systemd/issues/22801
In summary, systemd uses malloc_usable_size in a way that it's
discouraged, which results in behaviour that's undefined by the
standard, which is now caught out by the compiler. We're hoping to
convince the systemd community to fix this so that we can actually
build it with fortification.
> I do not see benefit in a security change that ignores PID 1 process,
Do you mean that factually or rhetorically? I've shared statistics of
improvement in the proposal, surely improvements to packages like
bash, sudo and coreutils would count for something :)
> If the feature, on the GCC side, is not 100% done.
> How do I tell a difference of a bug with the _FORTIFY_SOURCE which I will ignore and
a bug with my package?
_FORTIFY_SOURCE=3 has been enabled on OpenSUSE for nearly a year. If
it causes a crash in your package, it's extremely likely that your
package has a bug.
This is interesting to know. I was not aware of this as I do not
follow
other distributions closely.
I would have appreciated seeing this noted on the proposal.
I then know I can compare spec files / build results with the relevant
counterparts if it works for them but not for us.
> I do not have the knowledge or the time to be able to say that
GCC generated the wrong machine code and therefore it is not a bug with my package.
> If my program was not complaining before the change and is now complaining with the
change, I am opting out of the change, and filing a bug against GCC on Fedora.
>
> I assume that by the package providing the exception, packagers get to choose
themselves and we do not need to go through FESCO
> to disable a security feature?
I think they should have to, but that's something for FESCO to decide.
_FORTIFY_SOURCE is not a new feature, the new level is just better at
catching bugs. Significantly better.
To this point, including and opt-out
mechanism is IMO reasonable.
Even including a "reference" to the opt out mechanism of the current
_FORTIFY_SOURCE level,
should it be the same, is valuable.
> == Benefit to Fedora ==
>
>
[https://docs.google.com/spreadsheets/d/1nPSmbEf3HVB91zI8yBraMqVry3_ILmlV2...
> Analysis of packages] in Fedora rawhide indicate that the improvement
> of mitigation coverage is on average over 2.4x, in some cases
> protecting more than half of the fortified glibc calls in the target
> application.
>
> This change will thus harden Fedora to a significant extent, thus
> making it a more secure distribution out of the box.
>
>
> 1) Is there some complete source for all these findings? If google sheets cannot
handle all the "raw data" as noted in the comment,
> maybe it is the wrong medium.
I've literally posted the raw data in the sheets, please take a look.
Ah, I was
starting to get mixed up the sheet pages. The "Raw data - All
x86_64" include the sentence "Note: Summary data since Google sheets
didn't like the raw data".
Other raw data variants seem to be correctly "raw".
> 2) What does *anything* on that google sheet mean. I have managed to figure out, from
the article, that bos and bdos correspond to level 2 and 3 of _FORTIFY_SOURCE.
> However, total of /.*/? Violated accesses? Segfaults? Then followed by "Sum of
total". For rubygem-ffi, this reaches into hundreds while "bdos" is 2. That
is the only sum I can make, with the data provided.
> I am no wiser from looking at it, what do the data mean?
I've mentioned this in the article, but it's a compile-time statistic
of the number of success of __builtin_object_size vs
__builtin_dynamic_object_size, using the fortify-metrics plugin:
https://github.com/siddhesh/fortify-metrics
The numbers you should be looking at are the "coverage" columns, which
tells you what percentage of the calls were successful. For
rubygem-ffi, the fortification is negligible regardless of whether it
is built at _FORTIFY_SOURCE=2 or _FORTIFY_SOURCE=3. This means that
it is unaffected by this change.
What I was trying to say with that paragraph is
that the format is hard
to understand for me and I really did not expect
the description or link to it under "More trapped buffer overflows".
A line next to the link in the Fedora Change along the lines of
"Collected via fortify-metrics, see <fortify-metrics repo>."
provides me with the required context.
> 3) I cannot speak to much else than Ruby, I do not see ruby in
neither the failures or "All x86_64". Should we attempt to test it ourselves?
That's odd, thanks for pointing out.
> Please provide a proper "how to test" section, I cannot fix what I cannot
test or compare results when I have no idea what I am seeing.
>
> Actually, last time I heard about number of packages, it was around 50k (not source,
build result), and as I stated,
> Ruby is missing, and so are quite a few dependent packages that should have GCC
involved somewhere:
> ~~~
> $ dnf repoquery --whatrequires "*libruby.so*" --disablerepo="*"
--enablerepo="fedora" 2>/dev/null | grep -v "i686" | uniq | wc -l
> 115
> ~~~
>
> If we also filter rubygem-* packages that depend on the *libruby.so* (and most
probably contain a binary extension written in C/C++ that links to Ruby), I get 68
packages.
> When I search "All x86_64" for "ruby" I get 28 packages. That
is... not adding up.
Yeah, I have missed packages; I had looked at revdeps glibc-devel,
which turned out around 9k packages, of which 6k had any
_FORTIFY_SOURCE use at all. I can redo with *all* packages and
generate a report.
I'll be honest, I do not expect that much of an improvement
if the
package using only Ruby provided memory C-API (which is why you probably
searched only for glibc-devel).
> == Scope ==
> * Proposal owners: Post a merge request to redhat-rpm-config with the
> actual change to build flags.
>
> * Other developers:
> Resolve bugs filed for build failures, either by fixing the bug
> exposed by `_FORTIFY_SOURCE=3` or by disabling `_FORTIFY_SOURCE=3` for
> the package if it is a false positive or if the package is unable to
> adapt to the change.
>
> * Release engineering: Mass rebuild required
> * Policies and guidelines: Guidelines should include workaround for
> packages that fail to build with `-Wp,-D_FORTIFY_SOURCE=3` due to a
> false positive.
>
> I'll just ask, what is a false positive. How can I tell. What are the steps for
this.
If _FORTIFY_SOURCE causes crashes in your package, it's most likely a
bug in your package. In the rare case of a false positive (or even if
in doubt or if you can't figure out what the bug is), cc me and I'll
help you determine what to do there.
Thank you and Florian for speaking up to this
point. Again, I would
appreciate if this would be noted in the change proposal
as that will be the document I consult first if I suspect that
_FORTIFY_SOURCE might be causing problems.
> * Trademark approval: N/A (not needed for this Change)
>
>
> == Upgrade/compatibility impact ==
> No ABI change, so there should be no impact on compatibility in a
> mixed environment.
>
> == How To Test ==
> * Smoke testing of packages to ensure that they continue to work
> correctly. Some packages may have overflows exposed at runtime, which
> may need to be fixed.
>
> This does not describe how to test, e.g. ahead of time, just that the packagers are
to deal with the fallout...
cc me, I'll be happy to help you out.
What I had in mind here are use cases
like Ruby C extensions.
If Ruby's GC goes anywhere close to the related calls and there is a bug
in the API, we cannot validate it without first rebuilding Ruby
and then all the related binary extensions with the option to cover most
ground.
Maybe I am chasing a red herring with this, but I generally err on the
side of caution.
I did a rebuild of nearly 10k
packages and there were few failures (see the failures tab in the
doc). I'll expand that list to make sure that the pain on maintainers
is minimal.
Thank you, I'd appreciate that.
> == User Experience ==
> No noticeable change to users.
>
> I beg to differ. From what I am able to tell, from the limited provided sources,
packages can have a runtime crash.
>
> A LOT of GUI packages use C or the likes that respond to the flags of "fortify
source".
> This can have a serious impact to users by the recompiled packages:
> * not suddenly working, package's tests passed, but user triggered just the right
code branch
> * performance impacted due to overhead.
I don't think that's the intended definition of "User Experience"
here, but I'll be happy to add to it if needed.
I agree that this one seems
more open to interpretation what belongs where.
What I latch on to is the following sentence in the change template
"Describe what Users will see or notice, for example:"
As a user I will definitely notice app crashing :) .
I am not as worried about this since I found out, via the discussion,
that other distributions have this as a default for some time now.
This inspires a bit more confidence in general.
Thanks,
Sid
Thanks for the response,
Jarek
4:13 a.m.
On 05/12/2022 20:58, Ben Cotton wrote:
Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
improve mitigation of security issues arising from buffer overflows in
packages in Fedora.
AFAIK, _FORTIFY_SOURCE=3 enables runtime checks for every mem*()
function call. This should result in significant performance degradation.
To proposal owner: add information about potential performance
degradation, including benchmark results.
--
Sincerely,
Vitaly Zaitsev (vitaly(a)easycoding.org)
4:37 a.m.
On Tue, Dec 06, 2022 at 11:13:38AM +0100, Vitaly Zaitsev via devel wrote:
On 05/12/2022 20:58, Ben Cotton wrote:
> Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
> improve mitigation of security issues arising from buffer overflows in
> packages in Fedora.
AFAIK, _FORTIFY_SOURCE=3 enables runtime checks for every mem*() function
call. This should result in significant performance degradation.
That is misunderstanding.
The way _FORTIFY_SOURCE works is that say for memcpy if the destination
has known upper bound on size (__builtin_object_size (dst, 0) returns
something other than ~(size_t)0), then __memcpy_chk is used instead of
memcpy, unless the compiler can prove that the length will always fit into
that destination (in that case it is folded back to (builtin) memcpy).
So, "every" is definitely not the case. In many cases nothing is known
about the size of the destination, and in a lot of cases it is provable that
no overflow will happen.
#define _FORTIFY_SOURCE 2 // or 3
#include <string.h>
void qux (void *);
void foo (void *dst, void *src, size_t len) { memcpy (dst, src, len); }
// Nothing is known about the length of dst above, so it will be normal memcpy
void bar (void *src) { char dst[64]; memcpy (dst, src, 32); qux (dst); }
// __builtin_object_size (dst, 0) is 64, but 32 <= 64, so again it will be
// normal memcpy.
void baz (void *src, size_t len) { char dst[64]; memcpy (dst, src, len); qux (dst); }
// __builtin_object_size (dst, 0) is 64, we don't know if len <= 64, so
// with _FORTIFY_SOURCE 1 and higher there will be __memcpy_chk (dst, src, len, 64);
// call.
The only change that happens between _FORTIFY_SOURCE 2 and 3 is that in the
former case only constant upper bounds are computed, while
__builtin_dynamic_object_size can compute either constant (including
~(size_t) 0 for don't know, no upper bound) or something computed at
runtime.
void quux (void *src, size_t len) { char dst = malloc (64); memcpy (dst, src, len); qux
(dst); }
// The above will still have even with _FORTIFY_SOURCE 2 __bos0 (dst) 64, so
// again __memcpy_chk (dst, src, len, 64);
void corge (void *src, size_t sz, size_t len) { char dst = malloc (sz); memcpy (dst, src,
len); qux (dst); }
// But the above case changes with -D_FORTIFY_SOURCE=3, with 2 __bos0 (dst)
// is -1, with 3 __bdos0 (dst) is sz. In this case it is still quite cheap
// to compute, just means extending the lifetime of sz until the
// __memcpy_chk (dst, src, len, sz); call. True, in some other cases it
// might be more expensive to compute, but the goal is still that the
// compiler can punt at any time to -1 if the computation of the length
// would be too expensive.
To proposal owner: add information about potential performance
degradation,
including benchmark results.
Deferring that to Siddhesh, I haven't done that benchmarking myself.
Jakub
7:32 a.m.
On Tue, Dec 6, 2022 at 5:45 AM Jakub Jelinek <jakub(a)redhat.com> wrote:
On Tue, Dec 06, 2022 at 11:13:38AM +0100, Vitaly Zaitsev via devel wrote:
> On 05/12/2022 20:58, Ben Cotton wrote:
> > Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
> > improve mitigation of security issues arising from buffer overflows in
> > packages in Fedora.
>
> AFAIK, _FORTIFY_SOURCE=3 enables runtime checks for every mem*() function
> call. This should result in significant performance degradation.
That is misunderstanding.
The way _FORTIFY_SOURCE works is that say for memcpy if the destination
has known upper bound on size (__builtin_object_size (dst, 0) returns
something other than ~(size_t)0), then __memcpy_chk is used instead of
memcpy, unless the compiler can prove that the length will always fit into
that destination (in that case it is folded back to (builtin) memcpy).
So, "every" is definitely not the case. In many cases nothing is known
about the size of the destination, and in a lot of cases it is provable that
no overflow will happen.
To add to this, I twiddled the macros to make sure that even if the
sizes are not known, we can infer safety from ranges of values that
they could have. For example if the destination object size in memcpy
is known to be at least 1024 bytes and the write size is known to be
at most 64 bytes, the copy is deemed safe at compile time.
> To proposal owner: add information about potential performance
degradation,
> including benchmark results.
Deferring that to Siddhesh, I haven't done that benchmarking myself.
I haven't run any benchmarks because (1) the magnitude of coverage is
immense, making a small performance impact (IMO) worthwhile and (2)
other distributions have enabled _FORTIFY_SOURCE=3 by default for
nearly a year without any reports of performance degradation from
their users and (3) the code size tests show negligible impact.
However if it is considered a blocker by FESCO, I'll be happy to run a
benchmark they suggest to post numbers.
Thanks,
Sid
11:45 a.m.
On Mon, 2022-12-05 at 14:58 -0500, Ben Cotton wrote:
https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribu...
This document represents a proposed Change. As part of the Changes
process, proposals are publicly announced in order to receive
community feedback. This proposal will only be implemented if
approved
by the Fedora Engineering Steering Committee.
== Summary ==
Replace the current `_FORTIFY_SOURCE=2` with `_FORTIFY_SOURCE=3` to
improve mitigation of security issues arising from buffer overflows
in
packages in Fedora.
Is this supported yet? I'm doing a build of rubberband for Rawhide, and
every file generates this warning:
../src/test/TestStretcher.cpp: warning: -D_FORTIFY_SOURCE defined but
value is too low
Thanks,
--
Michel Alexandre Salim
identities:
https://keyoxide.org/5dce2e7e9c3b1cffd335c1d78b229d2f7ccc04f2
12:04 p.m.
On Wed, Jan 11, 2023 at 12:45 PM Michel Alexandre Salim
<michel(a)michel-slm.name> wrote:
Is this supported yet? I'm doing a build of rubberband for
Rawhide, and
every file generates this warning:
../src/test/TestStretcher.cpp: warning: -D_FORTIFY_SOURCE defined but
value is too low
Yes the change is in, it even broke systemd once ;) Could you please
share the full build log? That looks like an annobin warning, which
may suggest an annobin bug but maybe something else is wrong.
Thanks,
Sid
12:14 p.m.
On Wed, Jan 11, 2023 at 01:04:45PM -0500, Siddhesh Poyarekar wrote:
On Wed, Jan 11, 2023 at 12:45 PM Michel Alexandre Salim
<michel(a)michel-slm.name> wrote:
> Is this supported yet? I'm doing a build of rubberband for Rawhide, and
> every file generates this warning:
>
> ../src/test/TestStretcher.cpp: warning: -D_FORTIFY_SOURCE defined but
> value is too low
Yes the change is in, it even broke systemd once ;) Could you please
share the full build log? That looks like an annobin warning, which
may suggest an annobin bug but maybe something else is wrong.
The warning seems to happen only in mock, the Koji build is clean:
https://koji.fedoraproject.org/koji/taskinfo?taskID=96015653
Let me paste the root.log and build.log from the local build (I'm now
working on upgrading a second package that uses -Werror, so on that the
FORTIFY_SOURCE issue actually fails the build - will have to locally
test against fedora-37-x86_64 instead of Rawhide for now).
build.log: https://paste.centos.org/view/ea89a51c
root.log: https://paste.centos.org/view/c25c9b3f
Thanks for looking!
--
Michel Alexandre Salim
identities: https://keyoxide.org/5dce2e7e9c3b1cffd335c1d78b229d2f7ccc04f2
1:29 p.m.
On Wed, Jan 11, 2023 at 1:15 PM Michel Alexandre Salim
<salimma(a)fedoraproject.org> wrote:
The warning seems to happen only in mock, the Koji build is clean:
https://koji.fedoraproject.org/koji/taskinfo?taskID=96015653
Let me paste the root.log and build.log from the local build (I'm now
working on upgrading a second package that uses -Werror, so on that the
FORTIFY_SOURCE issue actually fails the build - will have to locally
test against fedora-37-x86_64 instead of Rawhide for now).
build.log: https://paste.centos.org/view/ea89a51c
root.log: https://paste.centos.org/view/c25c9b3f
Can you tell me how you're doing the mock build? With my fedora 37
laptop a `fedpkg mockbuild` went through just fine for rubberband and
I don't see any of the superfluous annobin messages:
https://paste.centos.org/view/86ec6416
If I find a way to reproduce this I could figure out why annobin is
behaving that way.
Thanks,
Sid
2:04 p.m.
On Wed, 2023-01-11 at 14:29 -0500, Siddhesh Poyarekar wrote:
On Wed, Jan 11, 2023 at 1:15 PM Michel Alexandre Salim
<salimma(a)fedoraproject.org> wrote:
> The warning seems to happen only in mock, the Koji build is clean:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=96015653
>
> Let me paste the root.log and build.log from the local build (I'm
> now
> working on upgrading a second package that uses -Werror, so on that
> the
> FORTIFY_SOURCE issue actually fails the build - will have to
> locally
> test against fedora-37-x86_64 instead of Rawhide for now).
>
> build.log: https://paste.centos.org/view/ea89a51c
> root.log: https://paste.centos.org/view/c25c9b3f
Can you tell me how you're doing the mock build? With my fedora 37
laptop a `fedpkg mockbuild` went through just fine for rubberband and
I don't see any of the superfluous annobin messages:
Just `mock -r fedora-rawhide-x86_64 ./*.src.rpm`
I have these additional knobs in ~/.config/mock.cfg:
config_opts['plugin_conf']['ccache_enable'] = True
config_opts['plugin_conf']['ccache_opts']['max_cache_size'] =
'10G'
# False: build from Koji, e.g. to grab packages just put into Rawhide
# config_opts['mirrored'] = False
HTH,
--
Michel Alexandre Salim
identities:
https://keyoxide.org/5dce2e7e9c3b1cffd335c1d78b229d2f7ccc04f2
3:37 p.m.
On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
<salimma(a)fedoraproject.org> wrote:
Just `mock -r fedora-rawhide-x86_64 ./*.src.rpm`
I have these additional knobs in ~/.config/mock.cfg:
config_opts['plugin_conf']['ccache_enable'] = True
config_opts['plugin_conf']['ccache_opts']['max_cache_size'] =
'10G'
# False: build from Koji, e.g. to grab packages just put into Rawhide
# config_opts['mirrored'] = False
OK I see the warning now. Let me dig deeper.
Thanks,
Sid
3:43 p.m.
On Wed, Jan 11, 2023 at 4:37 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
<salimma(a)fedoraproject.org> wrote:
> Just `mock -r fedora-rawhide-x86_64 ./*.src.rpm`
>
> I have these additional knobs in ~/.config/mock.cfg:
>
>
> config_opts['plugin_conf']['ccache_enable'] = True
> config_opts['plugin_conf']['ccache_opts']['max_cache_size']
= '10G'
>
> # False: build from Koji, e.g. to grab packages just put into Rawhide
> # config_opts['mirrored'] = False
>
OK I see the warning now. Let me dig deeper.
So if you want the quick answer to unblock you, it is ccache; I
disabled it and the warning went away. That should help you get your
-Werror going. I'll look closer at why that's happening.
Sid
5:44 p.m.
On Wed, Jan 11, 2023 at 4:43 PM Siddhesh Poyarekar <siddhesh(a)redhat.com> wrote:
On Wed, Jan 11, 2023 at 4:37 PM Siddhesh Poyarekar
<siddhesh(a)redhat.com> wrote:
>
> On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
> <salimma(a)fedoraproject.org> wrote:
> > Just `mock -r fedora-rawhide-x86_64 ./*.src.rpm`
> >
> > I have these additional knobs in ~/.config/mock.cfg:
> >
> >
> > config_opts['plugin_conf']['ccache_enable'] = True
> >
config_opts['plugin_conf']['ccache_opts']['max_cache_size'] =
'10G'
> >
> > # False: build from Koji, e.g. to grab packages just put into Rawhide
> > # config_opts['mirrored'] = False
> >
>
> OK I see the warning now. Let me dig deeper.
So if you want the quick answer to unblock you, it is ccache; I
disabled it and the warning went away. That should help you get your
-Werror going. I'll look closer at why that's happening.
I've filed a ccache bug. It looks like ccache is moving the compiler
arguments around, causing one of the -U_FORTIFY_SOURCE to the end.
https://bugzilla.redhat.com/show_bug.cgi?id=2160275
Sid
11:41 a.m.
On Wed, Jan 11, 2023 at 06:44:22PM -0500, Siddhesh Poyarekar wrote:
On Wed, Jan 11, 2023 at 4:43 PM Siddhesh Poyarekar
<siddhesh(a)redhat.com> wrote:
> On Wed, Jan 11, 2023 at 4:37 PM Siddhesh Poyarekar <siddhesh(a)redhat.com>
wrote:
> >
> > On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
> > <salimma(a)fedoraproject.org> wrote:
> > > Just `mock -r fedora-rawhide-x86_64 ./*.src.rpm`
> > >
> > > I have these additional knobs in ~/.config/mock.cfg:
> > >
> > >
> > > config_opts['plugin_conf']['ccache_enable'] = True
> > >
config_opts['plugin_conf']['ccache_opts']['max_cache_size'] =
'10G'
> > >
> > > # False: build from Koji, e.g. to grab packages just put into Rawhide
> > > # config_opts['mirrored'] = False
> > >
> >
> > OK I see the warning now. Let me dig deeper.
>
> So if you want the quick answer to unblock you, it is ccache; I
> disabled it and the warning went away. That should help you get your
> -Werror going. I'll look closer at why that's happening.
I've filed a ccache bug. It looks like ccache is moving the compiler
arguments around, causing one of the -U_FORTIFY_SOURCE to the end.
https://bugzilla.redhat.com/show_bug.cgi?id=2160275
Why we do have those -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE options
in there at all? Previously we just had -Wp,-D_FORTIFY_SOURCE=2
and I think just changing it to -Wp,-D_FORTIFY_SOURCE=3 is more than enough.
If you think some programs are adding -D_FORTIFY_SOURCE=2 to their *FLAGS
make vars etc., then perhaps -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3
But having both -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE seems completely
pointless to me.
Jakub
5:29 p.m.
On Thu, Jan 12, 2023 at 12:41 PM Jakub Jelinek <jakub(a)redhat.com> wrote:
> I've filed a ccache bug. It looks like ccache is moving the
compiler
> arguments around, causing one of the -U_FORTIFY_SOURCE to the end.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2160275
Why we do have those -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE options
in there at all? Previously we just had -Wp,-D_FORTIFY_SOURCE=2
and I think just changing it to -Wp,-D_FORTIFY_SOURCE=3 is more than enough.
If you think some programs are adding -D_FORTIFY_SOURCE=2 to their *FLAGS
make vars etc., then perhaps -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3
But having both -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE seems completely
pointless to me.
A number of packages have -D_FORTIFY_SOURCE=2 in their default build
flags, which was fine before because the redefinition didn't actually
change the value of the macro. That unfortunately fails with
-D_FORTIFY_SOURCE=3 since the macro now gets redefined to a different
value, resulting in a compile time warning.
As a result we need to undefine any previous macro definition and
define it to 3. There were more than 20 packages IIRC and asking the
upstreams to change for this didn't make sense, but I'm happy to
explore other ideas, if any.
Sid
5:38 p.m.
On Thu, Jan 12, 2023 at 06:29:10PM -0500, Siddhesh Poyarekar wrote:
On Thu, Jan 12, 2023 at 12:41 PM Jakub Jelinek
<jakub(a)redhat.com> wrote:
> > I've filed a ccache bug. It looks like ccache is moving the compiler
> > arguments around, causing one of the -U_FORTIFY_SOURCE to the end.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=2160275
>
> Why we do have those -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE options
> in there at all? Previously we just had -Wp,-D_FORTIFY_SOURCE=2
> and I think just changing it to -Wp,-D_FORTIFY_SOURCE=3 is more than enough.
> If you think some programs are adding -D_FORTIFY_SOURCE=2 to their *FLAGS
> make vars etc., then perhaps -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3
> But having both -U_FORTIFY_SOURCE and -Wp,-U_FORTIFY_SOURCE seems completely
> pointless to me.
A number of packages have -D_FORTIFY_SOURCE=2 in their default build
flags, which was fine before because the redefinition didn't actually
change the value of the macro. That unfortunately fails with
-D_FORTIFY_SOURCE=3 since the macro now gets redefined to a different
value, resulting in a compile time warning.
As a result we need to undefine any previous macro definition and
define it to 3. There were more than 20 packages IIRC and asking the
upstreams to change for this didn't make sense, but I'm happy to
explore other ideas, if any.
But at least you don't need both -U_FORTIFY_SOURCE and
-Wp,-U_FORTIFY_SOURCE, one of them is enough. And the latter I think
better gets through libtool and other tools; especially if you put it
into a single -Wp, option together with the redefinition...
Jakub
5:46 p.m.
On Thu, Jan 12, 2023 at 6:38 PM Jakub Jelinek <jakub(a)redhat.com> wrote:
But at least you don't need both -U_FORTIFY_SOURCE and
-Wp,-U_FORTIFY_SOURCE, one of them is enough. And the latter I think
better gets through libtool and other tools; especially if you put it
into a single -Wp, option together with the redefinition...
Uhmm, I could have sworn that I had needed -Wp,-U... for the -Wp-D...
and -U for the -D when I was fixing the test builds but you're right,
simply -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=3 appears to work
for both -D_FORTIFY_SOURCE=2 as well as -Wp,-D_FORTIFY_SOURCE=2. I'll
test this some more and post a PR for redhat-rpm-config.
Thanks,
Sid
9:09 p.m.
On Thu, Jan 12, 2023 at 06:46:17PM -0500, Siddhesh Poyarekar wrote:
On Thu, Jan 12, 2023 at 6:38 PM Jakub Jelinek
<jakub(a)redhat.com> wrote:
> But at least you don't need both -U_FORTIFY_SOURCE and
> -Wp,-U_FORTIFY_SOURCE, one of them is enough. And the latter I think
> better gets through libtool and other tools; especially if you put it
> into a single -Wp, option together with the redefinition...
Uhmm, I could have sworn that I had needed -Wp,-U... for the -Wp-D...
and -U for the -D when I was fixing the test builds but you're right,
simply -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=3 appears to work
for both -D_FORTIFY_SOURCE=2 as well as -Wp,-D_FORTIFY_SOURCE=2. I'll
test this some more and post a PR for redhat-rpm-config.
Thanks, Sid and Jakub, for digging in and identifying the issue! Looking
forward to being able to use ccache for my local Rawhide builds again.
Glad this will end up making the change less brittle too.
Best regards,
--
Michel Alexandre Salim
identities: https://keyoxide.org/5dce2e7e9c3b1cffd335c1d78b229d2f7ccc04f2
465
days inactive
505
days old
62 comments
17 participants
participants (17)
-
Andrii Nakryiko -
Ben Cotton -
Daniel P. Berrangé -
Demi Marie Obenour -
Gary Buhrmaster -
Jakub Jelinek -
Jarek Prokop -
Jaroslav Prokop -
Michael Catanzaro -
Michel Alexandre Salim -
Michel Alexandre Salim -
Neal Gompa -
przemek klosowski -
Richard W.M. Jones -
Siddhesh Poyarekar -
Vitaly Zaitsev -
Zbigniew Jędrzejewski-Szmek