[PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[Leesoo Ahn]

The major part, the conditional branch in selinux_mmap_addr() is always to be
false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time.

This usually happens in some linux distros, for instance Ubuntu, which
the config is set to zero in release version. Therefore it could be a bit
optimized with '#if <expr>' at compile time.

Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com>
---
 security/selinux/hooks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d06e350fedee..a049aab6524b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr)
 {
 	int rc = 0;
 
+#if CONFIG_LSM_MMAP_MIN_ADDR > 0
 	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
 		u32 sid = current_sid();
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
 	}
+#endif
 
 	return rc;
 }
-- 
2.34.1

Re: [PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[Paul Moore]

On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote:
>
> The major part, the conditional branch in selinux_mmap_addr() is always to be
> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time.
>
> This usually happens in some linux distros, for instance Ubuntu, which
> the config is set to zero in release version. Therefore it could be a bit
> optimized with '#if <expr>' at compile time.
>
> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com>
> ---
>  security/selinux/hooks.c | 2 ++
>  1 file changed, 2 insertions(+)

First, I agree with Stephen's comments that you should ask your distro
(you mentioned Debian) to move MIN_ADDR higher.  Beyond that, I have
one request, see below ...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d06e350fedee..a049aab6524b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr)
>  {
>         int rc = 0;
>
> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0
>         if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
>                 u32 sid = current_sid();
>                 rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>                                   MEMPROTECT__MMAP_ZERO, NULL);
>         }
> +#endif
>
>         return rc;
>  }

Pre-processor conditionals inside a function are generally something
we don't recommend.  In this case I would suggest doing something like
this:

#if (MMAP_MIN_ADDR > 0)
static int selinux_mmap_addr(...)
{
  /* current func definition */
}
#else /* MMAP_MIN_ADDR > 0 */
static int selinux_mmap_addr(...)
{
  return 0;
}
#endif /* MMAP_MIN_ADDR > 0 */

-- 
paul-moore.com

Re: [PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[Casey Schaufler]

On 7/17/2023 1:13 PM, Paul Moore wrote:
> On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote:
>> The major part, the conditional branch in selinux_mmap_addr() is always to be
>> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time.
>>
>> This usually happens in some linux distros, for instance Ubuntu, which
>> the config is set to zero in release version. Therefore it could be a bit
>> optimized with '#if <expr>' at compile time.
>>
>> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com>
>> ---
>>  security/selinux/hooks.c | 2 ++
>>  1 file changed, 2 insertions(+)
> First, I agree with Stephen's comments that you should ask your distro
> (you mentioned Debian) to move MIN_ADDR higher.  Beyond that, I have
> one request, see below ...
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index d06e350fedee..a049aab6524b 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr)
>>  {
>>         int rc = 0;
>>
>> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0
>>         if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
>>                 u32 sid = current_sid();
>>                 rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>>                                   MEMPROTECT__MMAP_ZERO, NULL);
>>         }
>> +#endif
>>
>>         return rc;
>>  }
> Pre-processor conditionals inside a function are generally something
> we don't recommend.  In this case I would suggest doing something like
> this:
>
> #if (MMAP_MIN_ADDR > 0)
> static int selinux_mmap_addr(...)
> {
>   /* current func definition */
> }
> #else /* MMAP_MIN_ADDR > 0 */
> static int selinux_mmap_addr(...)
> {
>   return 0;
> }
> #endif /* MMAP_MIN_ADDR > 0 */

Better yet, skip the #else here and #if out the LSM_HOOK_INIT(mmap_addr, ...).
No hook at all is faster than a hook that does nothing.

Re: [PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[Paul Moore]

On Mon, Jul 17, 2023 at 4:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/17/2023 1:13 PM, Paul Moore wrote:
> > On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote:
> >> The major part, the conditional branch in selinux_mmap_addr() is always to be
> >> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time.
> >>
> >> This usually happens in some linux distros, for instance Ubuntu, which
> >> the config is set to zero in release version. Therefore it could be a bit
> >> optimized with '#if <expr>' at compile time.
> >>
> >> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com>
> >> ---
> >>  security/selinux/hooks.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> > First, I agree with Stephen's comments that you should ask your distro
> > (you mentioned Debian) to move MIN_ADDR higher.  Beyond that, I have
> > one request, see below ...
> >
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index d06e350fedee..a049aab6524b 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr)
> >>  {
> >>         int rc = 0;
> >>
> >> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0
> >>         if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
> >>                 u32 sid = current_sid();
> >>                 rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
> >>                                   MEMPROTECT__MMAP_ZERO, NULL);
> >>         }
> >> +#endif
> >>
> >>         return rc;
> >>  }
> > Pre-processor conditionals inside a function are generally something
> > we don't recommend.  In this case I would suggest doing something like
> > this:
> >
> > #if (MMAP_MIN_ADDR > 0)
> > static int selinux_mmap_addr(...)
> > {
> >   /* current func definition */
> > }
> > #else /* MMAP_MIN_ADDR > 0 */
> > static int selinux_mmap_addr(...)
> > {
> >   return 0;
> > }
> > #endif /* MMAP_MIN_ADDR > 0 */
>
> Better yet, skip the #else here and #if out the LSM_HOOK_INIT(mmap_addr, ...).
> No hook at all is faster than a hook that does nothing.

My only concern with that approach is the disconnected nature: one
ifdef around the func definition, one around the LSM_HOOK_INIT() call.
If we thought a zero MMAP_MIN_ADDR value was a good idea, or even
common, I would be more inclined to pay the bad-code-practices-tax
here, but seeing as we don't want to encourage a zero MMAP_MIN_ADDR
value I'd rather lean towards the more maintainable code.

-- 
paul-moore.com

Re: [PATCH] selinux: optimize major part with a kernel config in selinux_mmap_addr()

[Thomas Weißschuh]

On 2023-07-10 17:25:00+0900, Leesoo Ahn wrote:
> The major part, the conditional branch in selinux_mmap_addr() is always to be
> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time.
> 
> This usually happens in some linux distros, for instance Ubuntu, which
> the config is set to zero in release version. Therefore it could be a bit
> optimized with '#if <expr>' at compile time.
> 
> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com>
> ---
>  security/selinux/hooks.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d06e350fedee..a049aab6524b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr)
>  {
>  	int rc = 0;
>  
> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0
>  	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
>  		u32 sid = current_sid();
>  		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>  				  MEMPROTECT__MMAP_ZERO, NULL);
>  	}
> +#endif

Shouldn't the compiler figure out on its own that "0 < 0" is always
false and optimize it all away? My gcc 13.1.1 does so.

Without your change:

$ ./scripts/bloat-o-meter  security/selinux/hooks.o-min-addr-64k security/selinux/hooks.o-min-addr-0
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-65 (-65)
Function                                     old     new   delta
selinux_mmap_addr                             81      16     -65
Total: Before=57673, After=57608, chg -0.11%

The same with your patch and also with the proposal by Paul that
redefines the whole function to "return 0".