* unnecessary log output in selinux_status_updated
@ 2022-10-07 8:54 Petr Lautrbach
2022-10-07 12:46 ` Stephen Smalley
0 siblings, 1 reply; 5+ messages in thread
From: Petr Lautrbach @ 2022-10-07 8:54 UTC (permalink / raw)
To: selinux; +Cc: Mike Palmiotto, Stephen Smalley
Hi,
Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed
selinux_status_updated() so that it calls avc_process_policyload() and
avc_process_setenforce() and both functions call avc_log() and avc_log() logs to
stderr by default. So when a process like `rpm` checks whether there was a
change, it gets output on stderr which previously wasn't there.
Before this change:
>>> from selinux import *
>>> selinux_status_open(0);
0
>>>
>>> selinux_status_updated();
0
>>> selinux_mkload_policy(0);
0
>>> selinux_status_updated();
1
Current version:
>>> from selinux import *
elinux_status_updated();
selinux_mkload_policy(0);
selinux_status_updated();
>>> selinux_status_open(0);
0
>>> selinux_status_updated();
0
>>> selinux_mkload_policy(0);
0
>>> selinux_status_updated();
uavc: op=load_policy lsm=selinux seqno=2 res=11
The calling process could set its callback but it seems unnecessarily
complicated just for selinux_status_updated() which is supposed to check whether
something has changed or not. Also processing events in this function seems to
be unnecessary.
It looks like the reason for the new code added to selinux_status_updated() is
that there were several avc_netlink_check_nb() calls replaced by
selinux_status_updated(). Given the problem described above, I don't think it's
correct and I would like to change selinux_status_updated() back and use another
mechanism that would help with the replacement.
So what do you think about it?
Petr
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: unnecessary log output in selinux_status_updated 2022-10-07 8:54 unnecessary log output in selinux_status_updated Petr Lautrbach @ 2022-10-07 12:46 ` Stephen Smalley 2022-10-07 13:53 ` Petr Lautrbach 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2022-10-07 12:46 UTC (permalink / raw) To: Petr Lautrbach, Paul Moore; +Cc: selinux, Mike Palmiotto On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > Hi, > > > Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed > selinux_status_updated() so that it calls avc_process_policyload() and > avc_process_setenforce() and both functions call avc_log() and avc_log() logs to > stderr by default. So when a process like `rpm` checks whether there was a > change, it gets output on stderr which previously wasn't there. > > > Before this change: > >>> from selinux import * > >>> selinux_status_open(0); > 0 > >>> > >>> selinux_status_updated(); > 0 > >>> selinux_mkload_policy(0); > 0 > >>> selinux_status_updated(); > 1 > > Current version: > >>> from selinux import * > elinux_status_updated(); > selinux_mkload_policy(0); > selinux_status_updated(); > >>> selinux_status_open(0); > 0 > >>> selinux_status_updated(); > 0 > >>> selinux_mkload_policy(0); > 0 > >>> selinux_status_updated(); > uavc: op=load_policy lsm=selinux seqno=2 res=11 > > > The calling process could set its callback but it seems unnecessarily > complicated just for selinux_status_updated() which is supposed to check whether > something has changed or not. Also processing events in this function seems to > be unnecessary. > > It looks like the reason for the new code added to selinux_status_updated() is > that there were several avc_netlink_check_nb() calls replaced by > selinux_status_updated(). Given the problem described above, I don't think it's > correct and I would like to change selinux_status_updated() back and use another > mechanism that would help with the replacement. > > > So what do you think about it? The goal was to switch the AVC and all of its users (e.g. selinux_check_access) over to using the much more efficient SELinux kernel status page mechanism for setenforce and policy load notifications on newer kernels instead of the SELinux netlink mechanism (which imposed extra syscall overhead on the critical path). Understand your concern but unsure as to whether we can just change selinux_status_updated() back now. Would require an audit of all users of selinux_status_updated(), both direct and indirect, to ensure that none of them are relying on this behavior. We can obviously fix the callers within libselinux but addressing external callers is more problematic and is arguably an ABI change. Would need to look at all users of the AVC, selinux_check_access(), etc. This change happened 2 years ago so I have to wonder why it is only coming up now? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: unnecessary log output in selinux_status_updated 2022-10-07 12:46 ` Stephen Smalley @ 2022-10-07 13:53 ` Petr Lautrbach 2022-10-07 15:22 ` Stephen Smalley 0 siblings, 1 reply; 5+ messages in thread From: Petr Lautrbach @ 2022-10-07 13:53 UTC (permalink / raw) To: Stephen Smalley, Paul Moore; +Cc: selinux, Mike Palmiotto Stephen Smalley <stephen.smalley.work@gmail.com> writes: > On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> Hi, >> >> >> Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed >> selinux_status_updated() so that it calls avc_process_policyload() and >> avc_process_setenforce() and both functions call avc_log() and avc_log() logs to >> stderr by default. So when a process like `rpm` checks whether there was a >> change, it gets output on stderr which previously wasn't there. >> >> >> Before this change: >> >>> from selinux import * >> >>> selinux_status_open(0); >> 0 >> >>> >> >>> selinux_status_updated(); >> 0 >> >>> selinux_mkload_policy(0); >> 0 >> >>> selinux_status_updated(); >> 1 >> >> Current version: >> >>> from selinux import * >> elinux_status_updated(); >> selinux_mkload_policy(0); >> selinux_status_updated(); >> >>> selinux_status_open(0); >> 0 >> >>> selinux_status_updated(); >> 0 >> >>> selinux_mkload_policy(0); >> 0 >> >>> selinux_status_updated(); >> uavc: op=load_policy lsm=selinux seqno=2 res=11 >> >> >> The calling process could set its callback but it seems unnecessarily >> complicated just for selinux_status_updated() which is supposed to check whether >> something has changed or not. Also processing events in this function seems to >> be unnecessary. >> >> It looks like the reason for the new code added to selinux_status_updated() is >> that there were several avc_netlink_check_nb() calls replaced by >> selinux_status_updated(). Given the problem described above, I don't think it's >> correct and I would like to change selinux_status_updated() back and use another >> mechanism that would help with the replacement. >> >> >> So what do you think about it? > > The goal was to switch the AVC and all of its users (e.g. > selinux_check_access) over to using the much more efficient SELinux > kernel status page mechanism for setenforce and policy load > notifications on newer kernels instead of the SELinux netlink > mechanism (which imposed extra syscall overhead on the critical path). > > Understand your concern but unsure as to whether we can just change > selinux_status_updated() back now. > Would require an audit of all users of selinux_status_updated(), both > direct and indirect, to ensure that none of them are relying on this > behavior. We can obviously fix the callers within libselinux but > addressing external callers is more problematic and is arguably an ABI > change. Would need to look at all users of the AVC, > selinux_check_access(), etc. > This change happened 2 years ago so I have to wonder why it is only > coming up now? Nobody has noticed it. 83 avc_log(SELINUX_POLICYLOAD, 84 "%s: op=load_policy lsm=selinux seqno=%u res=1", 85 avc_prefix, seqno); There's missing '\n' and so this message is sooner or later overwritten by something else, see https://bugzilla.redhat.com/show_bug.cgi?id=2123637 and https://bugzilla.redhat.com/show_bug.cgi?id=2123719 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: unnecessary log output in selinux_status_updated 2022-10-07 13:53 ` Petr Lautrbach @ 2022-10-07 15:22 ` Stephen Smalley 2022-10-11 13:01 ` Petr Lautrbach 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2022-10-07 15:22 UTC (permalink / raw) To: Petr Lautrbach; +Cc: Paul Moore, selinux, Mike Palmiotto On Fri, Oct 7, 2022 at 9:53 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > Stephen Smalley <stephen.smalley.work@gmail.com> writes: > > > On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@redhat.com> wrote: > >> > >> Hi, > >> > >> > >> Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed > >> selinux_status_updated() so that it calls avc_process_policyload() and > >> avc_process_setenforce() and both functions call avc_log() and avc_log() logs to > >> stderr by default. So when a process like `rpm` checks whether there was a > >> change, it gets output on stderr which previously wasn't there. > >> > >> > >> Before this change: > >> >>> from selinux import * > >> >>> selinux_status_open(0); > >> 0 > >> >>> > >> >>> selinux_status_updated(); > >> 0 > >> >>> selinux_mkload_policy(0); > >> 0 > >> >>> selinux_status_updated(); > >> 1 > >> > >> Current version: > >> >>> from selinux import * > >> elinux_status_updated(); > >> selinux_mkload_policy(0); > >> selinux_status_updated(); > >> >>> selinux_status_open(0); > >> 0 > >> >>> selinux_status_updated(); > >> 0 > >> >>> selinux_mkload_policy(0); > >> 0 > >> >>> selinux_status_updated(); > >> uavc: op=load_policy lsm=selinux seqno=2 res=11 > >> > >> > >> The calling process could set its callback but it seems unnecessarily > >> complicated just for selinux_status_updated() which is supposed to check whether > >> something has changed or not. Also processing events in this function seems to > >> be unnecessary. > >> > >> It looks like the reason for the new code added to selinux_status_updated() is > >> that there were several avc_netlink_check_nb() calls replaced by > >> selinux_status_updated(). Given the problem described above, I don't think it's > >> correct and I would like to change selinux_status_updated() back and use another > >> mechanism that would help with the replacement. > >> > >> > >> So what do you think about it? > > > > The goal was to switch the AVC and all of its users (e.g. > > selinux_check_access) over to using the much more efficient SELinux > > kernel status page mechanism for setenforce and policy load > > notifications on newer kernels instead of the SELinux netlink > > mechanism (which imposed extra syscall overhead on the critical path). > > > > Understand your concern but unsure as to whether we can just change > > selinux_status_updated() back now. > > Would require an audit of all users of selinux_status_updated(), both > > direct and indirect, to ensure that none of them are relying on this > > behavior. We can obviously fix the callers within libselinux but > > addressing external callers is more problematic and is arguably an ABI > > change. Would need to look at all users of the AVC, > > selinux_check_access(), etc. > > This change happened 2 years ago so I have to wonder why it is only > > coming up now? > > Nobody has noticed it. > > 83 avc_log(SELINUX_POLICYLOAD, > 84 "%s: op=load_policy lsm=selinux seqno=%u res=1", > 85 avc_prefix, seqno); > > There's missing '\n' and so this message is sooner or later overwritten by > something else, see > https://bugzilla.redhat.com/show_bug.cgi?id=2123637 and > https://bugzilla.redhat.com/show_bug.cgi?id=2123719 Ok, regardless, we need to ensure that changing it won't break systemd, dbus, or any other userspace object managers. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: unnecessary log output in selinux_status_updated 2022-10-07 15:22 ` Stephen Smalley @ 2022-10-11 13:01 ` Petr Lautrbach 0 siblings, 0 replies; 5+ messages in thread From: Petr Lautrbach @ 2022-10-11 13:01 UTC (permalink / raw) To: Stephen Smalley; +Cc: Paul Moore, selinux, Mike Palmiotto Stephen Smalley <stephen.smalley.work@gmail.com> writes: > On Fri, Oct 7, 2022 at 9:53 AM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> Stephen Smalley <stephen.smalley.work@gmail.com> writes: >> >> > On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> >> >> Hi, >> >> >> >> >> >> Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed >> >> selinux_status_updated() so that it calls avc_process_policyload() and >> >> avc_process_setenforce() and both functions call avc_log() and avc_log() logs to >> >> stderr by default. So when a process like `rpm` checks whether there was a >> >> change, it gets output on stderr which previously wasn't there. >> >> >> >> >> >> Before this change: >> >> >>> from selinux import * >> >> >>> selinux_status_open(0); >> >> 0 >> >> >>> >> >> >>> selinux_status_updated(); >> >> 0 >> >> >>> selinux_mkload_policy(0); >> >> 0 >> >> >>> selinux_status_updated(); >> >> 1 >> >> >> >> Current version: >> >> >>> from selinux import * >> >> elinux_status_updated(); >> >> selinux_mkload_policy(0); >> >> selinux_status_updated(); >> >> >>> selinux_status_open(0); >> >> 0 >> >> >>> selinux_status_updated(); >> >> 0 >> >> >>> selinux_mkload_policy(0); >> >> 0 >> >> >>> selinux_status_updated(); >> >> uavc: op=load_policy lsm=selinux seqno=2 res=11 >> >> >> >> >> >> The calling process could set its callback but it seems unnecessarily >> >> complicated just for selinux_status_updated() which is supposed to check whether >> >> something has changed or not. Also processing events in this function seems to >> >> be unnecessary. >> >> >> >> It looks like the reason for the new code added to selinux_status_updated() is >> >> that there were several avc_netlink_check_nb() calls replaced by >> >> selinux_status_updated(). Given the problem described above, I don't think it's >> >> correct and I would like to change selinux_status_updated() back and use another >> >> mechanism that would help with the replacement. >> >> >> >> >> >> So what do you think about it? >> > >> > The goal was to switch the AVC and all of its users (e.g. >> > selinux_check_access) over to using the much more efficient SELinux >> > kernel status page mechanism for setenforce and policy load >> > notifications on newer kernels instead of the SELinux netlink >> > mechanism (which imposed extra syscall overhead on the critical path). >> > >> > Understand your concern but unsure as to whether we can just change >> > selinux_status_updated() back now. >> > Would require an audit of all users of selinux_status_updated(), both >> > direct and indirect, to ensure that none of them are relying on this >> > behavior. We can obviously fix the callers within libselinux but >> > addressing external callers is more problematic and is arguably an ABI >> > change. Would need to look at all users of the AVC, >> > selinux_check_access(), etc. >> > This change happened 2 years ago so I have to wonder why it is only >> > coming up now? >> >> Nobody has noticed it. >> >> 83 avc_log(SELINUX_POLICYLOAD, >> 84 "%s: op=load_policy lsm=selinux seqno=%u res=1", >> 85 avc_prefix, seqno); >> >> There's missing '\n' and so this message is sooner or later overwritten by >> something else, see >> https://bugzilla.redhat.com/show_bug.cgi?id=2123637 and >> https://bugzilla.redhat.com/show_bug.cgi?id=2123719 > > Ok, regardless, we need to ensure that changing it won't break > systemd, dbus, or any other userspace object managers. rpm maintainer decided to implement a logging callback [1], and so there's no need to change this back. [1] https://github.com/rpm-software-management/rpm/pull/2201 Petr ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-11 13:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-07 8:54 unnecessary log output in selinux_status_updated Petr Lautrbach 2022-10-07 12:46 ` Stephen Smalley 2022-10-07 13:53 ` Petr Lautrbach 2022-10-07 15:22 ` Stephen Smalley 2022-10-11 13:01 ` Petr Lautrbach
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.