On Tue, 2023-07-18 at 09:17 +0200, Ondrej Mosnacek wrote:
> On Mon, Jul 17, 2023 at 4:30 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> >
> > Perf traces of network-related workload shows a measurable overhead
> > inside the network-related selinux hooks while zeroing the
> > lsm_network_audit struct.
> >
> > In most cases we can delay the initialization of such structure to the
> > usage point, avoiding such overhead in a few cases.
> >
> > Additionally, the audit code accesses the IP address information only
> > for AF_INET* families, and selinux_parse_skb() will fill-out the
> > relevant fields in such cases. When the family field is zeroed or the
> > initialization is followed by the mentioned parsing, the zeroing can be
> > limited to the sk, family and netif fields.
> >
> > By factoring out the audit-data initialization to new helpers, this
> > patch removes some duplicate code and gives small but measurable
> > performance gain under UDP flood.
> >
> > Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> > ---
> > Note the performance gain is small, but measurable and let the selinux
> > hooks almost disappear from the perf traces I collect. The only
> > remaining perf-related pain-point I see is the indirect call at the
> > security_ level, and tackling it looks much more difficult... :(
> > ---
> > security/selinux/hooks.c | 84 ++++++++++++++++++++--------------------
> > 1 file changed, 43 insertions(+), 41 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index d06e350fedee..9a75b3bcff2b 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4495,18 +4495,41 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec,
> > secclass, NULL, socksid);
> > }
> >
> > +static void __ad_init_net(struct common_audit_data *ad,
> > + struct lsm_network_audit *net,
> > + int ifindex, struct sock *sk, u16 family)
> > +{
> > + ad->type = LSM_AUDIT_DATA_NET;
> > + ad->u.net = net;
> > + net->netif = ifindex;
> > + net->sk = sk;
> > + net->family = family;
> > +}
> > +
> > +static void ad_init_net_from_sk(struct common_audit_data *ad,
> > + struct lsm_network_audit *net,
> > + struct sock *sk)
> > +{
> > + __ad_init_net(ad, net, 0, sk, 0);
> > +}
> > +
> > +static void ad_init_net_from_netif_family(struct common_audit_data *ad,
> > + struct lsm_network_audit *net,
> > + int ifindex, u16 family)
> > +{
> > + __ad_init_net(ad, net, ifindex, 0, family);
> > +}
> > +
>
> Since there is nothing SELinux-specific in these helpers, maybe it
> would be better to move them into <linux/lsm_audit.h> and also convert
> the other users of lsm_network_audit (Smack and AppArmor) to use them.
> (In fact AppArmor already seems to do something similar using its own
> macros.)
I'm even less familiar with the internal of AppArmor and smack than
with selinux. After a quick look, I think AppArmor will be better off
without by the above, as there is a single possible user of such
helper, not much relevant from perf perspective: socket creation is
much less critical then packet reception, transmission or bh
processing. Additional hooking the common helper in the existing
macro's layers will probably require
Smacks uses lsm_network_audit in more places, I tried to give it a shot
but the diffstat looks quite not compelling to me. Additionally I had
to prepend the 'lsm_' prefix to Paul's suggested helpers names, to keep
them consistent with the common APIs.
The main point, I think, is that each lsm does net information parsing
in a slightly different way, makingthe common helpers for such task
quite not fitting, short of some refactory in each lsm module.
TL;DR: I would avoid extending the helpers usage at this point. If
there is interest from other other lsm's maintainers, I think such
extension could be a follow-up.
Cheers,
Paolo
