| Message ID | 73a810980a8452f0cb98d25698c4ae83285b7393.1689604030.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] selinux: introduce and use ad_init_net*() helpers | expand |
On Mon, Jul 17, 2023 at 10:28 AM Paolo Abeni <pabeni@redhat.com> 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@redhat.com> > --- > 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(-) Overall this looks good to me, I just have some bikeshed-y comments around function names (below) ... > 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; > +} Let's call this "__ad_net_init()". > +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); > +} Similarly, let's call this "ad_net_init_from_sk()" > +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); > +} ... and I think this should be "ad_net_init_from_iif()".
On Mon, Jul 17, 2023 at 4:30 PM Paolo Abeni <pabeni@redhat.com> 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@redhat.com> > --- > 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.) <snip>
On Tue, 2023-07-18 at 09:17 +0200, Ondrej Mosnacek wrote: > On Mon, Jul 17, 2023 at 4:30 PM Paolo Abeni <pabeni@redhat.com> 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@redhat.com> > > --- > > 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
On Wed, Jul 19, 2023 at 5:36 AM Paolo Abeni <pabeni@redhat.com> wrote: > On Tue, 2023-07-18 at 09:17 +0200, Ondrej Mosnacek wrote: ... > > 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.) ... > 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. I agree. Quite frankly there is a lot of stuff in the LSM common audit code that could be improved.
Hi all, On Mon, 2023-07-17 at 16:27 +0200, Paolo Abeni wrote: > The only > remaining perf-related pain-point I see is the indirect call at the > security_ level, and tackling it looks much more difficult... :( I spent a little more time on this latest topic. AFAICS recently such overhead has increased due to the bpf lsm introduction. My understanding is that a major LSM and BPF LSM simultaneously enabled is a common usage scenario. That means 2 indirect calls + 2 untrain trails and 3 additional cache-lines used per hook. Under the assumption than having multiple major lsms enabled concurrently is less common, I hacked some (not exactly spectacularly beautiful) code to avoid the above. Basically, after initialization, for a limited number of hooks, it checks if only the default major lsm and eventually the bpf lsm are registered and if so, converts such hooks to static call[s], using static branches. The idea would be to keep the above infra usage restricted to the most performance-relevant hooks (e.g. one-off initialization or configuration are not relevant from that perspective). For obvious reasons I started from a few of network related hooks ;) As said the code could be more nice: there is some quite heavy macro usage and some duplication I was not able to avoid). On the flip side it shows quite measurable benefit when enabled, 0 impact when disabled, and it's available to all major LSM, except tomoyo (but even the latter could be accommodated with some effort). If there is some shared interest for the above I can share the current status and try to cleanup the code. Any feedback more then appreciated! Cheers, Paolo
On Fri, Jul 21, 2023 at 11:35 AM Paolo Abeni <pabeni@redhat.com> wrote: > > Hi all, > > On Mon, 2023-07-17 at 16:27 +0200, Paolo Abeni wrote: > > The only > > remaining perf-related pain-point I see is the indirect call at the > > security_ level, and tackling it looks much more difficult... :( > > I spent a little more time on this latest topic. AFAICS recently such > overhead has increased due to the bpf lsm introduction. My > understanding is that a major LSM and BPF LSM simultaneously enabled is > a common usage scenario. That means 2 indirect calls + 2 untrain trails > and 3 additional cache-lines used per hook. > > Under the assumption than having multiple major lsms enabled > concurrently is less common, I hacked some (not exactly spectacularly > beautiful) code to avoid the above. Basically, after initialization, > for a limited number of hooks, it checks if only the default major lsm > and eventually the bpf lsm are registered and if so, converts such > hooks to static call[s], using static branches. > > The idea would be to keep the above infra usage restricted to the most > performance-relevant hooks (e.g. one-off initialization or > configuration are not relevant from that perspective). For obvious > reasons I started from a few of network related hooks ;) > > As said the code could be more nice: there is some quite heavy macro > usage and some duplication I was not able to avoid). On the flip side > it shows quite measurable benefit when enabled, 0 impact when disabled, > and it's available to all major LSM, except tomoyo (but even the latter > could be accommodated with some effort). > > If there is some shared interest for the above I can share the current > status and try to cleanup the code. > > Any feedback more then appreciated! KP, the BPF LSM maintainer, has posted patches to move the LSM hooks over to static calls, but to the best of my current understanding the patchset intermingles a bug fix with the performance improvements, which I want to avoid. There have been updated patchsets posted, but the original link (below) contains my comments: https://lore.kernel.org/linux-security-module/20230119231033.1307221-1-kpsingh@kernel.org/
On 7/21/2023 8:34 AM, Paolo Abeni wrote: > Hi all, > > On Mon, 2023-07-17 at 16:27 +0200, Paolo Abeni wrote: >> The only >> remaining perf-related pain-point I see is the indirect call at the >> security_ level, and tackling it looks much more difficult... :( > I spent a little more time on this latest topic. AFAICS recently such > overhead has increased due to the bpf lsm introduction. My > understanding is that a major LSM and BPF LSM simultaneously enabled is > a common usage scenario. That means 2 indirect calls + 2 untrain trails > and 3 additional cache-lines used per hook. > > Under the assumption than having multiple major lsms enabled > concurrently is less common, I hacked some (not exactly spectacularly > beautiful) code to avoid the above. Basically, after initialization, > for a limited number of hooks, it checks if only the default major lsm > and eventually the bpf lsm are registered and if so, converts such > hooks to static call[s], using static branches. K.P. Singh proposed similar changes recently, and Brendan Jackman had something in 2020. The performance benefit demonstrated has been encouraging. The approach has two serious problems. It doesn't lend itself very well to special cases, and the code is incredibly ugly. > > The idea would be to keep the above infra usage restricted to the most > performance-relevant hooks (e.g. one-off initialization or > configuration are not relevant from that perspective). For obvious > reasons I started from a few of network related hooks ;) > > As said the code could be more nice: there is some quite heavy macro > usage and some duplication I was not able to avoid). On the flip side > it shows quite measurable benefit when enabled, 0 impact when disabled, > and it's available to all major LSM, except tomoyo (but even the latter > could be accommodated with some effort). > > If there is some shared interest for the above I can share the current > status and try to cleanup the code. > > Any feedback more then appreciated! > > Cheers, > > Paolo >
Hi, On Fri, 2023-07-21 at 12:11 -0400, Paul Moore wrote: > There have been updated patchsets posted, but the original link > (below) contains my comments: > https://lore.kernel.org/linux-security-module/20230119231033.1307221-1-kpsingh@kernel.org/ Thanks for the reference. Interestingly there are both similarities and differences between my approach and the above. I'll try to study the above a little bit more and then possibly I'll share what I have, just for reference. Cheers, Paolo
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); +} + static int sock_has_perm(struct sock *sk, u32 perms) { struct sk_security_struct *sksec = sk->sk_security; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; if (sksec->sid == SECINITSID_KERNEL) return 0; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sk = sk; + ad_init_net_from_sk(&ad, &net, sk); return avc_has_perm(current_sid(), sksec->sid, sksec->sclass, perms, &ad); @@ -4899,12 +4922,10 @@ static int selinux_socket_unix_stream_connect(struct sock *sock, struct sk_security_struct *sksec_other = other->sk_security; struct sk_security_struct *sksec_new = newsk->sk_security; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; int err; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sk = other; + ad_init_net_from_sk(&ad, &net, other); err = avc_has_perm(sksec_sock->sid, sksec_other->sid, sksec_other->sclass, @@ -4931,11 +4952,9 @@ static int selinux_socket_unix_may_send(struct socket *sock, struct sk_security_struct *ssec = sock->sk->sk_security; struct sk_security_struct *osec = other->sk->sk_security; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sk = other->sk; + ad_init_net_from_sk(&ad, &net, other->sk); return avc_has_perm(ssec->sid, osec->sid, osec->sclass, SOCKET__SENDTO, &ad); @@ -4971,13 +4990,10 @@ static int selinux_sock_rcv_skb_compat(struct sock *sk, struct sk_buff *skb, struct sk_security_struct *sksec = sk->sk_security; u32 sk_sid = sksec->sid; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; char *addrp; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->netif = skb->skb_iif; - ad.u.net->family = family; + ad_init_net_from_netif_family(&ad, &net, skb->skb_iif, family); err = selinux_parse_skb(skb, &ad, &addrp, 1, NULL); if (err) return err; @@ -5004,7 +5020,7 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) u16 family = sk->sk_family; u32 sk_sid = sksec->sid; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; char *addrp; u8 secmark_active; u8 peerlbl_active; @@ -5028,10 +5044,7 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) if (!secmark_active && !peerlbl_active) return 0; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->netif = skb->skb_iif; - ad.u.net->family = family; + ad_init_net_from_netif_family(&ad, &net, skb->skb_iif, family); err = selinux_parse_skb(skb, &ad, &addrp, 1, NULL); if (err) return err; @@ -5201,7 +5214,7 @@ static int selinux_sctp_process_new_assoc(struct sctp_association *asoc, u16 family = sk->sk_family; struct sk_security_struct *sksec = sk->sk_security; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; int err; /* handle mapped IPv4 packets arriving via IPv6 sockets */ @@ -5237,9 +5250,7 @@ static int selinux_sctp_process_new_assoc(struct sctp_association *asoc, /* Other association peer SIDs are checked to enforce * consistency among the peer SIDs. */ - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sk = asoc->base.sk; + ad_init_net_from_sk(&ad, &net, asoc->base.sk); err = avc_has_perm(sksec->peer_sid, asoc->peer_secid, sksec->sclass, SCTP_SOCKET__ASSOCIATION, &ad); @@ -5584,7 +5595,7 @@ static unsigned int selinux_ip_forward(void *priv, struct sk_buff *skb, char *addrp; u32 peer_sid; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; int secmark_active, peerlbl_active; if (!selinux_policycap_netpeer()) @@ -5600,10 +5611,7 @@ static unsigned int selinux_ip_forward(void *priv, struct sk_buff *skb, return NF_DROP; ifindex = state->in->ifindex; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->netif = ifindex; - ad.u.net->family = family; + ad_init_net_from_netif_family(&ad, &net, ifindex, family); if (selinux_parse_skb(skb, &ad, &addrp, 1, NULL) != 0) return NF_DROP; @@ -5683,7 +5691,7 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb, struct sock *sk; struct sk_security_struct *sksec; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; u8 proto = 0; sk = skb_to_full_sk(skb); @@ -5691,10 +5699,7 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb, return NF_ACCEPT; sksec = sk->sk_security; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->netif = state->out->ifindex; - ad.u.net->family = state->pf; + ad_init_net_from_netif_family(&ad, &net, state->out->ifindex, state->pf); if (selinux_parse_skb(skb, &ad, NULL, 0, &proto)) return NF_DROP; @@ -5719,7 +5724,7 @@ static unsigned int selinux_ip_postroute(void *priv, int ifindex; struct sock *sk; struct common_audit_data ad; - struct lsm_network_audit net = {0,}; + struct lsm_network_audit net; char *addrp; int secmark_active, peerlbl_active; @@ -5816,10 +5821,7 @@ static unsigned int selinux_ip_postroute(void *priv, } ifindex = state->out->ifindex; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->netif = ifindex; - ad.u.net->family = family; + ad_init_net_from_netif_family(&ad, &net, ifindex, family); if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL)) return NF_DROP;
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@redhat.com> --- 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(-)