Re: [PATCH 00/26] Address Space Isolation (ASI) 2024

Re: [PATCH 00/26] Address Space Isolation (ASI) 2024

[Brendan Jackman]

On Fri, 12 Jul 2024 at 19:00, Brendan Jackman <jackmanb@google.com> wrote:
>
> Overview
> ========
> This RFC demonstrates an implementation of Address Space Isolation
> (ASI), similar to Junaid Shahid’s proposal from 2022 [1].

Hi all,

I'll be discussing this series at the x86 MC at LPC next week. I
didn't get any high-level feedback so now would be a great time to
take a look and see if you have any thoughts about the basic
structure.

There are some bugs in this code but for an RFC it's basically
representative enough of what ASI will eventually look like.

In case it piques your interest here is some performance data I've
just gathered:

https://gist.github.com/bjackman/673415ee46fab01aa8d5f6ab1321b5b5

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Borislav Petkov]

On Fri, Jul 12, 2024 at 05:00:19PM +0000, Brendan Jackman wrote:
> +/*
> + * Can be used for functions which themselves are not strictly noinstr, but
> + * may be called from noinstr code.
> + */
> +#define inline_or_noinstr						\

Hmm, this is confusing. So is it noinstr or is it getting inlined?

I'd expect you either always inline the small functions - as you do for some
aleady - or mark the others noinstr. But not something in between.

Why this?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Brendan Jackman]

Hey Boris,

On Fri, 25 Oct 2024 at 13:41, Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Jul 12, 2024 at 05:00:19PM +0000, Brendan Jackman wrote:
> > +/*
> > + * Can be used for functions which themselves are not strictly noinstr, but
> > + * may be called from noinstr code.
> > + */
> > +#define inline_or_noinstr                                            \
>
> Hmm, this is confusing. So is it noinstr or is it getting inlined?

We don't care if it's getting inlined, which is kinda the point. This
annotation means "you may call this function from noinstr code". My
current understanding is that the normal noinstr annotation means
"this function fundamentally mustn't be instrumented".

So with inline_or_noinstr you get:

1. "Documentation" that the function itself doesn't have any problem
with getting traced etc.
2. Freedom for the compiler to inline or not.

> I'd expect you either always inline the small functions - as you do for some
> aleady - or mark the others noinstr. But not something in between.
>
> Why this?

Overall it's pretty likely I'm wrong about the subtlety of noinstr's
meaning. And the benefits I listed above are pretty minor. I should
have looked into this as it would have been an opportunity to reduce
the patch count of this RFC!

Maybe I'm also forgetting something more important, perhaps Junaid
will weigh in...

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Junaid Shahid]

On 10/25/24 6:21 AM, Brendan Jackman wrote:
> Hey Boris,
> 
> On Fri, 25 Oct 2024 at 13:41, Borislav Petkov <bp@alien8.de> wrote:
>>
>> On Fri, Jul 12, 2024 at 05:00:19PM +0000, Brendan Jackman wrote:
>>> +/*
>>> + * Can be used for functions which themselves are not strictly noinstr, but
>>> + * may be called from noinstr code.
>>> + */
>>> +#define inline_or_noinstr                                            \
>>
>> Hmm, this is confusing. So is it noinstr or is it getting inlined?
> 
> We don't care if it's getting inlined, which is kinda the point. This
> annotation means "you may call this function from noinstr code". My
> current understanding is that the normal noinstr annotation means
> "this function fundamentally mustn't be instrumented".
> 
> So with inline_or_noinstr you get:
> 
> 1. "Documentation" that the function itself doesn't have any problem
> with getting traced etc.
> 2. Freedom for the compiler to inline or not.
> 
>> I'd expect you either always inline the small functions - as you do for some
>> aleady - or mark the others noinstr. But not something in between.
>>
>> Why this?
> 
> Overall it's pretty likely I'm wrong about the subtlety of noinstr's
> meaning. And the benefits I listed above are pretty minor. I should
> have looked into this as it would have been an opportunity to reduce
> the patch count of this RFC!
> 
> Maybe I'm also forgetting something more important, perhaps Junaid
> will weigh in...

Yes, IIRC the idea was that there is no need to prohibit inlining for this class 
of functions.

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Thomas Gleixner]

On Tue, Oct 29 2024 at 10:38, Junaid Shahid wrote:
> On 10/25/24 6:21 AM, Brendan Jackman wrote:
>>> I'd expect you either always inline the small functions - as you do for some
>>> aleady - or mark the others noinstr. But not something in between.
>>>
>>> Why this?
>> 
>> Overall it's pretty likely I'm wrong about the subtlety of noinstr's
>> meaning. And the benefits I listed above are pretty minor. I should
>> have looked into this as it would have been an opportunity to reduce
>> the patch count of this RFC!
>> 
>> Maybe I'm also forgetting something more important, perhaps Junaid
>> will weigh in...
>
> Yes, IIRC the idea was that there is no need to prohibit inlining for this class 
> of functions.

I doubt that it works as you want it to work.

+	inline notrace __attribute((__section__(".noinstr.text")))	\

So this explicitely puts the inline into the .noinstr.text section,
which means when it is used in .text the compiler will generate an out-of
line function in the .noinstr.text section and insert a call into the
usage site. That's independent of the size of the inline.

Thanks,

        tglx

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Junaid Shahid]

On 10/29/24 12:12 PM, Thomas Gleixner wrote:
> 
> I doubt that it works as you want it to work.
> 
> +	inline notrace __attribute((__section__(".noinstr.text")))	\
> 
> So this explicitely puts the inline into the .noinstr.text section,
> which means when it is used in .text the compiler will generate an out-of
> line function in the .noinstr.text section and insert a call into the
> usage site. That's independent of the size of the inline.
> 

Oh, that's interesting. IIRC I had seen regular (.text) inline functions get 
inlined into .noinstr.text callers. I assume the difference is that here the 
section is marked explicitly rather than being implicit?

In any case, I guess we could just mark these functions as plain noinstr. 
(Unless there happens to be some other way to indicate to the compiler to place 
any non-inlined copy of the function in .noinstr.text but still allow inlining 
into .text if it makes sense optimization-wise.)

Thanks,
Junaid

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Brendan Jackman]

On Fri, 1 Nov 2024 at 02:44, Junaid Shahid <junaids@google.com> wrote:
> In any case, I guess we could just mark these functions as plain noinstr.

I wonder if it also would be worth having something like

/*
 * Inline this function so it can be called from noinstr,
 * but it wouldn't actually care itself about being instrumented.
 */
#define inline_for_noinstr __always_inline

Maybe there are already __always_inline functions this would apply to.
Then again, if you care about inlining them so much that you can't
just write "noinstr", then it's probably hot/small enough that
__always_inline would make sense regardless of noinstr.

Probably I'm over-thinking it at this point.

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Thomas Gleixner]

On Thu, Oct 31 2024 at 18:44, Junaid Shahid wrote:
> On 10/29/24 12:12 PM, Thomas Gleixner wrote:
>> 
>> I doubt that it works as you want it to work.
>> 
>> +	inline notrace __attribute((__section__(".noinstr.text")))	\
>> 
>> So this explicitely puts the inline into the .noinstr.text section,
>> which means when it is used in .text the compiler will generate an out-of
>> line function in the .noinstr.text section and insert a call into the
>> usage site. That's independent of the size of the inline.
>> 
>
> Oh, that's interesting. IIRC I had seen regular (.text) inline functions get 
> inlined into .noinstr.text callers. I assume the difference is that here the 
> section is marked explicitly rather than being implicit?

Correct. Inlines without any section attribute are free to be inlined in
any section, but if the compiler decides to uninline them, then it
sticks the uninlined version into the default section ".text".

The other problem there is that an out of line version can be
instrumented if not explicitely forbidden.

That's why we mark them __always_inline, which forces the compiler to
inline it into the usage site unconditionally.

> In any case, I guess we could just mark these functions as plain
> noinstr.

No. Some of them are used in hotpath '.text'. 'noinstr' prevents them to
be actually inlined then as I explained to you before.

> (Unless there happens to be some other way to indicate to the compiler to place 
> any non-inlined copy of the function in .noinstr.text but still allow inlining 
> into .text if it makes sense optimization-wise.)

Ideally the compilers would provide

        __attribute__(force_caller_section)

which makes them place an out of line inline into the section of the
function from which it is called. But we can't have useful things or
they are so badly documented that I can't find them ...

What actually works by some definition of "works" is:

       static __always_inline void __foo(void) { }

       static inline void foo(void)
       {
                __(foo);
       }

       static inline noinstr void foo_noinstr(void)
       {
                __(foo);
       }

The problem is that both GCC and clang optimize foo[_noinstr]() away and
then follow the __always_inline directive of __foo() even if I make
__foo() insanely large and have a gazillion of different functions
marked noinline invoking foo() or foo_noinstr(), unless I add -fno-inline
to the command line.

Which means it's not much different from just having '__always_inline
foo()' without the wrappers....

Compilers clearly lack a --do-what-I-mean command line option.

Now if I'm truly nasty then both compilers do what I mean even without a
magic command line option:

       static __always_inline void __foo(void) { }

       static __maybe_unused void foo(void)
       {
                __(foo);
       }

       static __maybe_unused noinstr void foo_noinstr(void)
       {
                __(foo);
       }

If there is a single invocation of either foo() or foo_noinstr() and
they are small enough then the compiler inlines them, unless -fno-inline
is on the command line. If there are multiple invocations and/or foo
gets big enough then both compilers out of line them. The out of line
wrappers with __foo() inlined in them end always up in the correct
section.

I actually really like the programming model as it is very clear about
the intention of usage and it allows static checkers to validate.

Thoughts?

Thanks,

        tglx

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Junaid Shahid]

On 11/1/24 1:27 PM, Thomas Gleixner wrote:
> On Thu, Oct 31 2024 at 18:44, Junaid Shahid wrote:
>> On 10/29/24 12:12 PM, Thomas Gleixner wrote:
>>>
>>> I doubt that it works as you want it to work.
>>>
>>> +	inline notrace __attribute((__section__(".noinstr.text")))	\
>>>
>>> So this explicitely puts the inline into the .noinstr.text section,
>>> which means when it is used in .text the compiler will generate an out-of
>>> line function in the .noinstr.text section and insert a call into the
>>> usage site. That's independent of the size of the inline.
>>>
>>
>> Oh, that's interesting. IIRC I had seen regular (.text) inline functions get
>> inlined into .noinstr.text callers. I assume the difference is that here the
>> section is marked explicitly rather than being implicit?
> 
> Correct. Inlines without any section attribute are free to be inlined in
> any section, but if the compiler decides to uninline them, then it
> sticks the uninlined version into the default section ".text".
> 
> The other problem there is that an out of line version can be
> instrumented if not explicitely forbidden.
> 
> That's why we mark them __always_inline, which forces the compiler to
> inline it into the usage site unconditionally.
> 
>> In any case, I guess we could just mark these functions as plain
>> noinstr.
> 
> No. Some of them are used in hotpath '.text'. 'noinstr' prevents them to
> be actually inlined then as I explained to you before.
> 
>> (Unless there happens to be some other way to indicate to the compiler to place
>> any non-inlined copy of the function in .noinstr.text but still allow inlining
>> into .text if it makes sense optimization-wise.)
> 
> Ideally the compilers would provide
> 
>          __attribute__(force_caller_section)
> 
> which makes them place an out of line inline into the section of the
> function from which it is called. But we can't have useful things or
> they are so badly documented that I can't find them ...
> 
> What actually works by some definition of "works" is:
> 
>         static __always_inline void __foo(void) { }
> 
>         static inline void foo(void)
>         {
>                  __(foo);
>         }
> 
>         static inline noinstr void foo_noinstr(void)
>         {
>                  __(foo);
>         }
> 
> The problem is that both GCC and clang optimize foo[_noinstr]() away and
> then follow the __always_inline directive of __foo() even if I make
> __foo() insanely large and have a gazillion of different functions
> marked noinline invoking foo() or foo_noinstr(), unless I add -fno-inline
> to the command line.
> 
> Which means it's not much different from just having '__always_inline
> foo()' without the wrappers....
> 
> Compilers clearly lack a --do-what-I-mean command line option.
> 
> Now if I'm truly nasty then both compilers do what I mean even without a
> magic command line option:
> 
>         static __always_inline void __foo(void) { }
> 
>         static __maybe_unused void foo(void)
>         {
>                  __(foo);
>         }
> 
>         static __maybe_unused noinstr void foo_noinstr(void)
>         {
>                  __(foo);
>         }
> 
> If there is a single invocation of either foo() or foo_noinstr() and
> they are small enough then the compiler inlines them, unless -fno-inline
> is on the command line. If there are multiple invocations and/or foo
> gets big enough then both compilers out of line them. The out of line
> wrappers with __foo() inlined in them end always up in the correct
> section.
> 
> I actually really like the programming model as it is very clear about
> the intention of usage and it allows static checkers to validate.
> 
> Thoughts?
> 

Thank you for the details. Yes, I think the last scheme that you described with 
separate wrappers for regular and noinst usage makes sense. IIRC the existing 
static validation wouldn't catch it if someone mistakenly called the .text 
version of the function from noinstr code and it just happened to get inlined. 
Perhaps we should add the -fno-inline compiler option with 
CONFIG_NOINSTR_VALIDATION?

Thanks,
Junaid

Re: [PATCH 01/26] mm: asi: Make some utility functions noinstr compatible

[Brendan Jackman]

On Fri, 1 Nov 2024 at 21:27, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, Oct 31 2024 at 18:44, Junaid Shahid wrote:
> What actually works by some definition of "works" is:
>
>        static __always_inline void __foo(void) { }
>
>        static inline void foo(void)
>        {
>                 __(foo);
>        }
>
>        static inline noinstr void foo_noinstr(void)
>        {
>                 __(foo);
>        }
>
> The problem is that both GCC and clang optimize foo[_noinstr]() away and
> then follow the __always_inline directive of __foo() even if I make
> __foo() insanely large and have a gazillion of different functions
> marked noinline invoking foo() or foo_noinstr(), unless I add -fno-inline
> to the command line.

In this experiment did you modify the definition of noinstr to remove
noinline? Otherwise I always get out-of-line calls (as you'd expect
from the noinline).

> Which means it's not much different from just having '__always_inline
> foo()' without the wrappers....
>
> Compilers clearly lack a --do-what-I-mean command line option.
>
> Now if I'm truly nasty then both compilers do what I mean even without a
> magic command line option:
>
>        static __always_inline void __foo(void) { }
>
>        static __maybe_unused void foo(void)
>        {
>                 __(foo);
>        }
>
>        static __maybe_unused noinstr void foo_noinstr(void)
>        {
>                 __(foo);
>        }

I don't see any difference with __maybe_unused: if the noinline is
there it's never inlined, otherwise with the wrapper technique it's
always inlined regardless of __maybe_unused:

static __always_inline void __big(void)
{
        asm volatile(
                "nop; nop; nop; nop; nop; nop; nop; nop; nop;"
                // and so on
                "nop; nop; nop; nop; nop; nop; nop; nop; nop;"
        );
}

static inline __section(".noinstr.text") void big_noinstr(void)
{
        __big();
}

When I call big_noinstr() from a noinstr function I see:

Dump of assembler code for function asi_exit:
   0xffffffff811e0080 <+0>:     endbr64
   0xffffffff811e0084 <+4>:     nop
   0xffffffff811e0085 <+5>:     nop
...and so on

I'm using GCC 14.2.0.

(I thought maybe this was because I used asm volatile nops to embiggen
the function but I see the same thing with a big stream of volatile C
statements).

I think we might have no choice but to always use
__always_inline/noinline for code that's called from both sections -
seems there's no way to tell the compiler "I don't care if you inline
it, but it we can't cross a section boundary". Am I missing anything?