subject:"Re\: F38 proposal\: Add _FORTIFY_SOURCE=3 to distribution build flags $System\-Wide Change proposal$"

On Wed, Jan 11, 2023 at 4:43 PM Siddhesh Poyarekar  wrote:
> On Wed, Jan 11, 2023 at 4:37 PM Siddhesh Poyarekar  
> wrote:
> >
> > On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
> >  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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Wed, Jan 11, 2023 at 4:37 PM Siddhesh Poyarekar  wrote:
>
> On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
>  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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Wed, Jan 11, 2023 at 3:05 PM Michel Alexandre Salim
 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2023-01-11 Thread Michel Alexandre Salim

On Wed, 2023-01-11 at 14:29 -0500, Siddhesh Poyarekar wrote:
> On Wed, Jan 11, 2023 at 1:15 PM Michel Alexandre Salim
>  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


signature.asc
Description: This is a digitally signed message part
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Wed, Jan 11, 2023 at 1:15 PM Michel Alexandre Salim
 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2023-01-11 Thread Michel Alexandre Salim

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
>  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


signature.asc
Description: PGP signature
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Wed, Jan 11, 2023 at 12:45 PM Michel Alexandre Salim
 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2023-01-11 Thread Michel Alexandre Salim

On Mon, 2022-12-05 at 14:58 -0500, Ben Cotton wrote:
> https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags
> 
> 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


signature.asc
Description: This is a digitally signed message part
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-14 Thread Vitaly Zaitsev via devel


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 (vit...@easycoding.org)
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-13 Thread Michael Catanzaro via devel

> 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.
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-13 Thread Siddhesh Poyarekar

On Tue, Dec 6, 2022 at 10:41 AM Siddhesh Poyarekar  wrote:
>
> On Tue, Dec 6, 2022 at 10:26 AM Gary Buhrmaster
>  wrote:
> >
> > On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar  
> > 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-09 Thread Zbigniew Jędrzejewski-Szmek

On Wed, Dec 07, 2022 at 04:41:09PM -0500, Siddhesh Poyarekar wrote:
> On Wed, Dec 7, 2022 at 3:15 PM Andrii Nakryiko
>  wrote:
> >
> > > On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
> > >  > >
> > > 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-08 Thread Siddhesh Poyarekar

[Responding from alias that's actually registered to devel@]

On Thu, Dec 8, 2022 at 8:25 AM Siddhesh Poyarekar  wrote:
>
> On Mon, Dec 5, 2022 at 3:04 PM Ben Cotton  wrote:
> >
> > On Mon, Dec 5, 2022 at 2:58 PM Ben Cotton  wrote:
> > >
> > > * Release engineering: Mass rebuild required
> >
> > Please file a ticket with Release Engineering if you haven't already.
>
> Done and quoted in the proposal.
>
> > > == 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.
>
> I've fleshed out the contingency section a bit to make the contingency
> plan more conservative as you suggested.
>
> Thanks,
> Sid
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Demi Marie Obenour

On 12/6/22 14:02, Richard W.M. Jones wrote:
> On Tue, Dec 06, 2022 at 05:52:16PM -, Andrii Nakryiko wrote:
>>> On Tue, Dec 06, 2022 at 03:12:19AM +, 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)


OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Siddhesh Poyarekar

On Wed, Dec 7, 2022 at 3:15 PM Andrii Nakryiko
 wrote:
>
> > On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
> >  >
> > 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Andrii Nakryiko

> On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
>  
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Andrii Nakryiko

> 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.
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Andrii Nakryiko

> On Tue, Dec 06, 2022 at 05:46:11PM -, 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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:

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Daniel P . Berrangé

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 :|
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-07 Thread Vitaly Zaitsev via devel


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 (vit...@easycoding.org)
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Michael Catanzaro via devel

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.
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 7:04 PM Michael Catanzaro via devel
 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?
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Michael Catanzaro via devel

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.org/message/UV65DSMPINEE6KWNI5MBH3MBQ26JHNNJ/

[2] 
https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/issues/1437#note_1191710138
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 06, 2022 at 05:52:16PM -, Andrii Nakryiko wrote:
> > On Tue, Dec 06, 2022 at 03:12:19AM +, 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@lists.fedoraproject.org
> To unsubscribe send an email to devel-le...@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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 12:56 PM Andrii Nakryiko
 wrote:
> > On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
> >  >
> > 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread przemek klosowski via devel

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.io%2Ffesco%2Fissue%2F2817data=05%7C01%7Cprzemek.klosowski%40nist.gov%7Ca591777f6c0249b566ff08dad7b1d043%7C2ab5d82fd8fa4797a93e054655c61dec%7C1%7C0%7C638059455959276624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=UB%2FsGNKWZQRvJiPPB7wmPwHXvtSFkMCZ0nltpyQmAl0%3Dreserved=0
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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Jakub Jelinek

On Tue, Dec 06, 2022 at 05:46:11PM -, 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Andrii Nakryiko

> On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
>  
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Andrii Nakryiko

> On Tue, Dec 06, 2022 at 03:12:19AM +, 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Andrii Nakryiko

> 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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Jarek Prokop

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 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_ILmlV2Z5K7FZeHZg/edit?usp=sharing
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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 3:42 PM Siddhesh Poyarekar  wrote:

> I have added a performance note[1] in the proposal.

Thank you.
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 10:26 AM Gary Buhrmaster
 wrote:
>
> On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar  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_distribution_build_flags#Performance_note
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 3:16 PM Siddhesh Poyarekar  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).
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 10:06 AM Gary Buhrmaster
 wrote:
>
> On Tue, Dec 6, 2022 at 12:47 PM Siddhesh Poyarekar  
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 12:47 PM Siddhesh Poyarekar  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).
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 9:55 AM Siddhesh Poyarekar  wrote:
>
> On Tue, Dec 6, 2022 at 8:14 AM Siddhesh Poyarekar  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/
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 8:14 AM Siddhesh Poyarekar  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/
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 5:45 AM Jakub Jelinek  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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Jakub Jelinek

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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 6, 2022 at 4:05 AM Richard W.M. Jones  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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Mon, Dec 5, 2022 at 10:20 PM Gary Buhrmaster
 wrote:
>
> On Mon, Dec 5, 2022 at 10:53 PM Neal Gompa  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-level#the_gains_of_improved_security_coverage_outweigh_the_cost
> >
>
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Mon, Dec 5, 2022 at 7:35 PM Jaroslav Prokop  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_ILmlV2Z5K7FZeHZg/edit?usp=sharing
> 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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Neal Gompa

On Tue, Dec 6, 2022 at 7:50 AM Siddhesh Poyarekar  wrote:
>
> On Mon, Dec 5, 2022 at 5:53 PM Neal Gompa  wrote:
> >
> > On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
> >  wrote:
> > >
> > > On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton  wrote:
> > > >
> > > > https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags
> > > >
> > >
> > > 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-level#the_gains_of_improved_security_coverage_outweigh_the_cost
> >
> > 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!
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Mon, Dec 5, 2022 at 5:53 PM Neal Gompa  wrote:
>
> On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
>  wrote:
> >
> > On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton  wrote:
> > >
> > > https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags
> > >
> >
> > 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-level#the_gains_of_improved_security_coverage_outweigh_the_cost
>
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Mon, Dec 5, 2022 at 3:17 PM Gary Buhrmaster
 wrote:
>
> On Mon, Dec 5, 2022 at 7:58 PM Ben Cotton  wrote:
> >
> > https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags
> >
>
> 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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Jakub Jelinek

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 
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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Vitaly Zaitsev via devel


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 (vit...@easycoding.org)
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

2022-12-06 Thread Jarek Prokop



On 12/6/22 10:08, Richard W.M. Jones wrote:

On Tue, Dec 06, 2022 at 08:59:03AM +, 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-level#2__better_fortification_coverage

   _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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

On Tue, Dec 06, 2022 at 08:59:03AM +, 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-level#2__better_fortification_coverage

  _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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)

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
___
devel mailing list -- devel@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@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

Re: F38 proposal: Add _FORTIFY_SOURCE=3 to distribution build flags (System-Wide Change proposal)