Hi Linus,
We've got an audit fix, and unfortunately it is two things I don't
like: big and based on a -rcX tag. The size of the patch is
(hopefully) explained well in the patch description, the -rcX base is
to get access to code not present in the v4.11 pull request
(audit/next is still based off v4.8; I'll be updating soon). While
I'm not excited that we need to be sending you something this large
during the -rcX phase, it does fix some very real, and very tangled,
problems relating to locking, backlog queues, and the audit daemon
connection.
This code has passed our testsuite without problem and it has held up
to my ad-hoc stress tests (arguably better than the existing code),
please consider pulling this as fix for the next v4.11-rcX tag.
Thanks,
-Paul
---
The following changes since commit 97da3854c526d3a6ee05c849c96e48d21527606c:
Linux 4.11-rc3 (2017-03-19 19:09:39 -0700)
are available in the git repository at:
git://git.infradead.org/users/pcmoore/audit stable-4.11
for you to fetch changes up to 5b52330bbfe63b3305765354d6046c9f7f89c011:
audit: fix auditd/kernel connection state tracking (2017-03-21 11:26:35 -0400)
----------------------------------------------------------------
Paul Moore (1):
audit: fix auditd/kernel connection state tracking
kernel/audit.c | 639 ++++++++++++++++++++++++++++++++++---------------------
kernel/audit.h | 9 +-
kernel/auditsc.c | 6 +-
3 files changed, 399 insertions(+), 255 deletions(-)
--
paul moore
security @ redhat
On Fri, Mar 24, 2017 at 5:39 PM, Paul Moore <[email protected]> wrote:
> Hi Linus,
>
> We've got an audit fix, and unfortunately it is two things I don't
> like: big and based on a -rcX tag. The size of the patch is
> (hopefully) explained well in the patch description, the -rcX base is
> to get access to code not present in the v4.11 pull request
> (audit/next is still based off v4.8; I'll be updating soon). While
> I'm not excited that we need to be sending you something this large
> during the -rcX phase, it does fix some very real, and very tangled,
> problems relating to locking, backlog queues, and the audit daemon
> connection.
>
> This code has passed our testsuite without problem and it has held up
> to my ad-hoc stress tests (arguably better than the existing code),
> please consider pulling this as fix for the next v4.11-rcX tag.
>
> Thanks,
> -Paul
>
> ---
> The following changes since commit 97da3854c526d3a6ee05c849c96e48d21527606c:
>
> Linux 4.11-rc3 (2017-03-19 19:09:39 -0700)
>
> are available in the git repository at:
>
> git://git.infradead.org/users/pcmoore/audit stable-4.11
>
> for you to fetch changes up to 5b52330bbfe63b3305765354d6046c9f7f89c011:
>
> audit: fix auditd/kernel connection state tracking (2017-03-21 11:26:35 -0400)
>
> ----------------------------------------------------------------
> Paul Moore (1):
> audit: fix auditd/kernel connection state tracking
>
> kernel/audit.c | 639 ++++++++++++++++++++++++++++++++++---------------------
> kernel/audit.h | 9 +-
> kernel/auditsc.c | 6 +-
> 3 files changed, 399 insertions(+), 255 deletions(-)
It would appear that I left off the magic "[GIT PULL]" subject prefix
in my last message. I suppose this is one more reason why one
shouldn't send pull requests past 5:00p on a Friday.
--
paul moore
security @ redhat
On Fri, Mar 24, 2017 at 2:39 PM, Paul Moore <[email protected]> wrote:
>
> This code has passed our testsuite without problem and it has held up
> to my ad-hoc stress tests (arguably better than the existing code),
> please consider pulling this as fix for the next v4.11-rcX tag.
Ok, pulled. However, looking at the changes in the patch it becomes
obvious that it is now completely bogus to inline the
"audit_signal_info()" function.
That silly inline in the header file now only generates bigger and
slower code, since that inlined function now calls another function
auditd_test_task() that is *not* inlined, so it ends up being a
function call anyway.
It would be much better to just unlinline audit_signal_info(), move it
into kernel/audit.c, and let the compiler then inline the
__audit_signal_info() helper function (or just fold it into that
function manually as part of the move).
The whole reason for that inlined part, and the uninlined
__audit_signal_info() helper was that the code *used* to be able to
avoid a function call entirely. That reason is now gone.
Linus
On Sat, Mar 25, 2017 at 6:23 PM, Linus Torvalds
<[email protected]> wrote:
> On Fri, Mar 24, 2017 at 2:39 PM, Paul Moore <[email protected]> wrote:
>>
>> This code has passed our testsuite without problem and it has held up
>> to my ad-hoc stress tests (arguably better than the existing code),
>> please consider pulling this as fix for the next v4.11-rcX tag.
>
> Ok, pulled. However, looking at the changes in the patch it becomes
> obvious that it is now completely bogus to inline the
> "audit_signal_info()" function.
>
> That silly inline in the header file now only generates bigger and
> slower code, since that inlined function now calls another function
> auditd_test_task() that is *not* inlined, so it ends up being a
> function call anyway.
Good catch. I was so worried about the other issues in this patch I
missed this obvious point.
> It would be much better to just unlinline audit_signal_info(), move it
> into kernel/audit.c, and let the compiler then inline the
> __audit_signal_info() helper function (or just fold it into that
> function manually as part of the move).
>
> The whole reason for that inlined part, and the uninlined
> __audit_signal_info() helper was that the code *used* to be able to
> avoid a function call entirely. That reason is now gone.
Agreed. Normally I would say let's just fix it in audit/next and I'll
send it to you during the next merge window; however, since we're
breaking the whole point of this inline in the -rcX stage, and the
uninline'ing patch would be rather trivial, would you prefer I send it
to you now for v4.11?
--
paul moore
security @ redhat
On Sun, Mar 26, 2017 at 8:18 AM, Paul Moore <[email protected]> wrote:
> On Sat, Mar 25, 2017 at 6:23 PM, Linus Torvalds
> <[email protected]> wrote:
>> The whole reason for that inlined part, and the uninlined
>> __audit_signal_info() helper was that the code *used* to be able to
>> avoid a function call entirely. That reason is now gone.
>
> Agreed. Normally I would say let's just fix it in audit/next and I'll
> send it to you during the next merge window; however, since we're
> breaking the whole point of this inline in the -rcX stage, and the
> uninline'ing patch would be rather trivial, would you prefer I send it
> to you now for v4.11?
It's fine if it does into some "next" branch for 4.12, I don't think
there is any hurry as long as this gets fixed eventually.
But I'll take the obvious cleanup for 4.11 too if you end up having
other things coming my way.
Linus
On Sun, Mar 26, 2017 at 1:23 PM, Linus Torvalds
<[email protected]> wrote:
> On Sun, Mar 26, 2017 at 8:18 AM, Paul Moore <[email protected]> wrote:
>> On Sat, Mar 25, 2017 at 6:23 PM, Linus Torvalds
>> <[email protected]> wrote:
>>> The whole reason for that inlined part, and the uninlined
>>> __audit_signal_info() helper was that the code *used* to be able to
>>> avoid a function call entirely. That reason is now gone.
>>
>> Agreed. Normally I would say let's just fix it in audit/next and I'll
>> send it to you during the next merge window; however, since we're
>> breaking the whole point of this inline in the -rcX stage, and the
>> uninline'ing patch would be rather trivial, would you prefer I send it
>> to you now for v4.11?
>
> It's fine if it does into some "next" branch for 4.12, I don't think
> there is any hurry as long as this gets fixed eventually.
>
> But I'll take the obvious cleanup for 4.11 too if you end up having
> other things coming my way.
Okay, sounds good. I'll work on it this week, assuming nothing blows
up in the process expect to see it later this week.
--
paul moore
security @ redhat