Re: Live patching on ARM64

Re: Live patching on ARM64

[Josh Poimboeuf]

Hi Madhavan,

I'd also recommend subscribing to the live-patching mailing list (Cc'ed).

On Thu, Jan 14, 2021 at 04:07:55PM -0600, Madhavan T. Venkataraman wrote:
> Hi all,
> 
> My name is Madhavan Venkataraman.
> 
> Microsoft is very interested in Live Patching support for ARM64.
> On behalf of Microsoft, I would like to contribute.
> 
> I would like to get in touch with the people who are currently working
> in this area, find out what exactly they are working on and see if they
> could use an extra pair of eyes/hands with what they are working on.
> 
> It looks like the most recent work in this area has been from the
> following folks:
> 
> Mark Brown and Mark Rutland:
> 	Kernel changes to providing reliable stack traces.
> 
> Julien Thierry:
> 	Providing ARM64 support in objtool.
> 
> Torsten Duwe:
> 	Ftrace with regs.
> 
> I apologize if I have missed anyone else who is working on Live Patching
> for ARM64. Do let me know.
> 
> Is there any work I can help with? Any areas that need investigation, any code
> that needs to be written, any work that needs to be reviewed, any testing that
> needs to done? You folks are probably super busy and would not mind an extra
> hand.
> 
> I have subscribed to linux-arm-kernel. But if any discussions should happen on
> another mailing list, could you please CC me? Appreciate it.
> 
> Thanks! I look forward to working with you.
> 
> Madhavan
> 
> P.S.:
> 
> Julien,
> 
> Could you send me a link to your latest RFC submission? Thanks!

-- 
Josh

Re: Live patching on ARM64

[Mark Rutland]

On Thu, Jan 14, 2021 at 04:07:55PM -0600, Madhavan T. Venkataraman wrote:
> Hi all,
> 
> My name is Madhavan Venkataraman.

Hi Madhavan,

> Microsoft is very interested in Live Patching support for ARM64.
> On behalf of Microsoft, I would like to contribute.
> 
> I would like to get in touch with the people who are currently working
> in this area, find out what exactly they are working on and see if they
> could use an extra pair of eyes/hands with what they are working on.
> 
> It looks like the most recent work in this area has been from the
> following folks:
> 
> Mark Brown and Mark Rutland:
> 	Kernel changes to providing reliable stack traces.
> 
> Julien Thierry:
> 	Providing ARM64 support in objtool.
> 
> Torsten Duwe:
> 	Ftrace with regs.

IIRC that's about right. I'm also trying to make arm64 patch-safe (more
on that below), and there's a long tail of work there for anyone
interested.

> I apologize if I have missed anyone else who is working on Live Patching
> for ARM64. Do let me know.
> 
> Is there any work I can help with? Any areas that need investigation, any code
> that needs to be written, any work that needs to be reviewed, any testing that
> needs to done? You folks are probably super busy and would not mind an extra
> hand.

One general thing that I believe we'll need to do is to rework code to
be patch-safe (which implies being noinstr-safe too). For example, we'll
need to rework the instruction patching code such that this cannot end
up patching itself (or anything that has instrumented it) in an unsafe
way.

Once we have objtool it should be possible to identify those cases
automatically. Currently I'm aware that we'll need to do something in at
least the following places:

* The entry code -- I'm currently chipping away at this.

* The insn framework (which is used by some patching code), since the
  bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
  
  We can probably shift the bulk of the aarch64_insn_gen_*() and
  aarch64_get_*() helpers into a header as __always_inline functions,
  which would allow them to be used in noinstr code. As those are
  typically invoked with a number of constant arguments that the
  compiler can fold, this /might/ work out as an optimization if the
  compiler can elide the error paths.

* The alternatives code, since we call instrumentable and patchable
  functions between updating instructions and performing all the
  necessary maintenance. There are a number of cases within
  __apply_alternatives(), e.g.

  - test_bit()
  - cpus_have_cap()
  - pr_info_once()
  - lm_alias()
  - alt_cb, if the callback is not marked as noinstr, or if it calls
    instrumentable code (e.g. from the insn framework).
  - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
    related code can be instrumented.

  This might need some underlying rework elsewhere (e.g. in the
  cpufeature code, or atomics framework).

So on the kernel side, maybe a first step would be to try to headerize
the insn generation code as __always_inline, and see whether that looks
ok? With that out of the way it'd be a bit easier to rework patching
code depending on the insn framework.

I'm not sure about the objtool side, so I'll leave that to Julien and co
to answer.

Thanks,
Mark.

Re: Live patching on ARM64

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3723 bytes --]

On Fri, Jan 15, 2021 at 12:33:47PM +0000, Mark Rutland wrote:
> On Thu, Jan 14, 2021 at 04:07:55PM -0600, Madhavan T. Venkataraman wrote:
> > Hi all,
> > 
> > My name is Madhavan Venkataraman.
> 
> Hi Madhavan,
> 
> > Microsoft is very interested in Live Patching support for ARM64.
> > On behalf of Microsoft, I would like to contribute.
> > 
> > I would like to get in touch with the people who are currently working
> > in this area, find out what exactly they are working on and see if they
> > could use an extra pair of eyes/hands with what they are working on.
> > 
> > It looks like the most recent work in this area has been from the
> > following folks:

Also copying in Bill Wendling who has also expressed an interest in
this.  Not deleting context for his benefit.

> > Mark Brown and Mark Rutland:
> > 	Kernel changes to providing reliable stack traces.
> > 
> > Julien Thierry:
> > 	Providing ARM64 support in objtool.
> > 
> > Torsten Duwe:
> > 	Ftrace with regs.
> 
> IIRC that's about right. I'm also trying to make arm64 patch-safe (more
> on that below), and there's a long tail of work there for anyone
> interested.
> 
> > I apologize if I have missed anyone else who is working on Live Patching
> > for ARM64. Do let me know.
> > 
> > Is there any work I can help with? Any areas that need investigation, any code
> > that needs to be written, any work that needs to be reviewed, any testing that
> > needs to done? You folks are probably super busy and would not mind an extra
> > hand.
> 
> One general thing that I believe we'll need to do is to rework code to
> be patch-safe (which implies being noinstr-safe too). For example, we'll
> need to rework the instruction patching code such that this cannot end
> up patching itself (or anything that has instrumented it) in an unsafe
> way.
> 
> Once we have objtool it should be possible to identify those cases
> automatically. Currently I'm aware that we'll need to do something in at
> least the following places:
> 
> * The entry code -- I'm currently chipping away at this.
> 
> * The insn framework (which is used by some patching code), since the
>   bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>   
>   We can probably shift the bulk of the aarch64_insn_gen_*() and
>   aarch64_get_*() helpers into a header as __always_inline functions,
>   which would allow them to be used in noinstr code. As those are
>   typically invoked with a number of constant arguments that the
>   compiler can fold, this /might/ work out as an optimization if the
>   compiler can elide the error paths.
> 
> * The alternatives code, since we call instrumentable and patchable
>   functions between updating instructions and performing all the
>   necessary maintenance. There are a number of cases within
>   __apply_alternatives(), e.g.
> 
>   - test_bit()
>   - cpus_have_cap()
>   - pr_info_once()
>   - lm_alias()
>   - alt_cb, if the callback is not marked as noinstr, or if it calls
>     instrumentable code (e.g. from the insn framework).
>   - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>     related code can be instrumented.
> 
>   This might need some underlying rework elsewhere (e.g. in the
>   cpufeature code, or atomics framework).
> 
> So on the kernel side, maybe a first step would be to try to headerize
> the insn generation code as __always_inline, and see whether that looks
> ok? With that out of the way it'd be a bit easier to rework patching
> code depending on the insn framework.
> 
> I'm not sure about the objtool side, so I'll leave that to Julien and co
> to answer.
> 
> Thanks,
> Mark.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Live patching on ARM64

[Madhavan T. Venkataraman]

On 1/15/21 6:33 AM, Mark Rutland wrote:

>> It looks like the most recent work in this area has been from the
>> following folks:
>>
>> Mark Brown and Mark Rutland:
>> 	Kernel changes to providing reliable stack traces.
>>
>> Julien Thierry:
>> 	Providing ARM64 support in objtool.
>>
>> Torsten Duwe:
>> 	Ftrace with regs.
> 
> IIRC that's about right. I'm also trying to make arm64 patch-safe (more
> on that below), and there's a long tail of work there for anyone
> interested.
> 

OK.

>> I apologize if I have missed anyone else who is working on Live Patching
>> for ARM64. Do let me know.
>>
>> Is there any work I can help with? Any areas that need investigation, any code
>> that needs to be written, any work that needs to be reviewed, any testing that
>> needs to done? You folks are probably super busy and would not mind an extra
>> hand.
> 
> One general thing that I believe we'll need to do is to rework code to
> be patch-safe (which implies being noinstr-safe too). For example, we'll
> need to rework the instruction patching code such that this cannot end
> up patching itself (or anything that has instrumented it) in an unsafe
> way.
> 

OK.

> Once we have objtool it should be possible to identify those cases
> automatically. Currently I'm aware that we'll need to do something in at
> least the following places:
> 
> * The entry code -- I'm currently chipping away at this.
> 

OK.

> * The insn framework (which is used by some patching code), since the
>   bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>   
>   We can probably shift the bulk of the aarch64_insn_gen_*() and
>   aarch64_get_*() helpers into a header as __always_inline functions,
>   which would allow them to be used in noinstr code. As those are
>   typically invoked with a number of constant arguments that the
>   compiler can fold, this /might/ work out as an optimization if the
>   compiler can elide the error paths.
> 
> * The alternatives code, since we call instrumentable and patchable
>   functions between updating instructions and performing all the
>   necessary maintenance. There are a number of cases within
>   __apply_alternatives(), e.g.
> 
>   - test_bit()
>   - cpus_have_cap()
>   - pr_info_once()
>   - lm_alias()
>   - alt_cb, if the callback is not marked as noinstr, or if it calls
>     instrumentable code (e.g. from the insn framework).
>   - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>     related code can be instrumented.
> 
>   This might need some underlying rework elsewhere (e.g. in the
>   cpufeature code, or atomics framework).
> 

OK.

> So on the kernel side, maybe a first step would be to try to headerize
> the insn generation code as __always_inline, and see whether that looks
> ok? With that out of the way it'd be a bit easier to rework patching
> code depending on the insn framework.
> 

OK.

I have an understanding of some of the above already. I will come up to
speed on the others. I will email you any questions I might have.

> I'm not sure about the objtool side, so I'll leave that to Julien and co
> to answer.
> 

Thanks for the information.

Madhavan

Re: Live patching on ARM64

[Julien Thierry]

Hi Madhavan,

On 1/17/21 6:25 PM, Madhavan T. Venkataraman wrote:
> 
> 
> On 1/15/21 6:33 AM, Mark Rutland wrote:
> 
>>> It looks like the most recent work in this area has been from the
>>> following folks:
>>>
>>> Mark Brown and Mark Rutland:
>>> 	Kernel changes to providing reliable stack traces.
>>>
>>> Julien Thierry:
>>> 	Providing ARM64 support in objtool.
>>>
>>> Torsten Duwe:
>>> 	Ftrace with regs.
>>
>> IIRC that's about right. I'm also trying to make arm64 patch-safe (more
>> on that below), and there's a long tail of work there for anyone
>> interested.
>>
> 
> OK.
> 
>>> I apologize if I have missed anyone else who is working on Live Patching
>>> for ARM64. Do let me know.
>>>
>>> Is there any work I can help with? Any areas that need investigation, any code
>>> that needs to be written, any work that needs to be reviewed, any testing that
>>> needs to done? You folks are probably super busy and would not mind an extra
>>> hand.
>>
>> One general thing that I believe we'll need to do is to rework code to
>> be patch-safe (which implies being noinstr-safe too). For example, we'll
>> need to rework the instruction patching code such that this cannot end
>> up patching itself (or anything that has instrumented it) in an unsafe
>> way.
>>
> 
> OK.
> 
>> Once we have objtool it should be possible to identify those cases
>> automatically. Currently I'm aware that we'll need to do something in at
>> least the following places:
>>
>> * The entry code -- I'm currently chipping away at this.
>>
> 
> OK.
> 
>> * The insn framework (which is used by some patching code), since the
>>    bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>>    
>>    We can probably shift the bulk of the aarch64_insn_gen_*() and
>>    aarch64_get_*() helpers into a header as __always_inline functions,
>>    which would allow them to be used in noinstr code. As those are
>>    typically invoked with a number of constant arguments that the
>>    compiler can fold, this /might/ work out as an optimization if the
>>    compiler can elide the error paths.
>>
>> * The alternatives code, since we call instrumentable and patchable
>>    functions between updating instructions and performing all the
>>    necessary maintenance. There are a number of cases within
>>    __apply_alternatives(), e.g.
>>
>>    - test_bit()
>>    - cpus_have_cap()
>>    - pr_info_once()
>>    - lm_alias()
>>    - alt_cb, if the callback is not marked as noinstr, or if it calls
>>      instrumentable code (e.g. from the insn framework).
>>    - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>>      related code can be instrumented.
>>
>>    This might need some underlying rework elsewhere (e.g. in the
>>    cpufeature code, or atomics framework).
>>
> 
> OK.
> 
>> So on the kernel side, maybe a first step would be to try to headerize
>> the insn generation code as __always_inline, and see whether that looks
>> ok? With that out of the way it'd be a bit easier to rework patching
>> code depending on the insn framework.
>>
> 
> OK.
> 
> I have an understanding of some of the above already. I will come up to
> speed on the others. I will email you any questions I might have.
> 
>> I'm not sure about the objtool side, so I'll leave that to Julien and co
>> to answer.
>>

Sorry for the late reply. The last RFC for arm64 support in objtool is a 
bit old because it was preferable to split things into smaller series.

I touched it much lately, so I'm picking it back up and will try to get 
a git branch into shape on a recent mainline (a few things need fixing 
since the last time I rebased it).

I'll update you once I have something at least usable/presentable.

Cheers,

-- 
Julien Thierry

Re: Live patching on ARM64

[Madhavan T. Venkataraman]

> Sorry for the late reply. The last RFC for arm64 support in objtool is a bit old because it was preferable to split things into smaller series.
> 
> I touched it much lately, so I'm picking it back up and will try to get a git branch into shape on a recent mainline (a few things need fixing since the last time I rebased it).
> 
> I'll update you once I have something at least usable/presentable.
> 
> Cheers,
> 

Great. Thanks!

Madhavan

Re: Live patching on ARM64

[Julien Thierry]

Hi,

On 1/19/21 4:19 PM, Madhavan T. Venkataraman wrote:
> 
>> Sorry for the late reply. The last RFC for arm64 support in objtool is a bit old because it was preferable to split things into smaller series.
>>
>> I touched it much lately, so I'm picking it back up and will try to get a git branch into shape on a recent mainline (a few things need fixing since the last time I rebased it).
>>
>> I'll update you once I have something at least usable/presentable.
>>
>> Cheers,
>>
> 

I just sent some series the arm64 objtool support:
- https://lkml.org/lkml/2021/1/20/791
- 
https://lore.kernel.org/linux-arm-kernel/20210120173800.1660730-1-jthierry@redhat.com/T/#t

There are still some things missing, so if you want to investigate a 
more complete state I have a branch:
$ git clone https://github.com/julien-thierry/linux.git -b 
objtoolxarm64-latest

Let me know if there are any questions related to it.

Cheers,

-- 
Julien Thierry

Re: Live patching on ARM64

[Madhavan T. Venkataraman]

Hi all,

Mark Rutland had sent me some ideas on what work is pending for ARM64 live patching.
I sent some questions to Mark Rutland. I forgot to include everyone in the email.
Sorry about that. I have reproduced my questions and his responses below. Please
chime in with any comments:

Thanks!




On Mon, Jan 25, 2021 at 11:58:47AM -0600, Madhavan T. Venkataraman wrote:
> Some questions below:

I've answered thos below.

If possible, I'd prefer to handle future queries on a public list (so
that others can chime in, and so that it gets archived), so if you could
direct further questions to a thread on LAKML, that would be much
appreciated.

> On 1/15/21 6:33 AM, Mark Rutland wrote:
> [...]
>>
>> One general thing that I believe we'll need to do is to rework code to
>> be patch-safe (which implies being noinstr-safe too). For example, we'll
>> need to rework the instruction patching code such that this cannot end
>> up patching itself (or anything that has instrumented it) in an unsafe
>> way.
>>
>
> OK. I understand that. Are there are other scenarios that make patching
> unsafe?

I suspect so; these are simply the cases I'm immediately aware of. I
suspect there are other cases that we will need to consider that don't
immediately spring to mind.

> I expect the kernel already handles scenarios such as two CPUs patching
> the same location at the same time or a thread executing at a location that is
> currently being patched.

IIRC that is supposed to be catered for by ftrace (and so I assume for
livepatching too); I'm not certain about kprobes. In addition to
synchronization in the core ftrace code, arm64's ftrace_modify_code()
has a sanity-check with a non-atomic RMW sequence. We might be able to
make that more robust wiuth a faultable cmpxchg, and some changes around
ftrace_update_ftrace_func() and ftrace_make_nop()  to get rid of the
unvalidated cases.

> Any other scenarios to be considered?

I'm not immediately aware of others, but suspect more cases will become
apparent as work progresses on the bits we already know about.

>> Once we have objtool it should be possible to identify those cases
>> automatically. Currently I'm aware that we'll need to do something in at
>> least the following places:
>>
>
> OK. AFAIK, objtool checks for the following:
>
>         - returning from noinstr function with instrumentation enabled
>
>         - calling instrumentable functions from noinstr code without:
>
>                 instrumentation_begin();
>                 instrumentation_end();
>
> Is that what you mean?

That's what I was thinking of, yes -- this should highlight some places
that will need attention.

> Does objtool check other things as well that is relevant to (un)safe
> patching?

I'm not entirely familiar with objtool, so I'm not exactly sure what it
can do; I expect Josh and Julien can give more detail here.

>> * The insn framework (which is used by some patching code), since the
>>   bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>>   
>>   We can probably shift the bulk of the aarch64_insn_gen_*() and
>>   aarch64_get_*() helpers into a header as __always_inline functions,
>>   which would allow them to be used in noinstr code. As those are
>>   typically invoked with a number of constant arguments that the
>>   compiler can fold, this /might/ work out as an optimization if the
>>   compiler can elide the error paths.
>
> OK. I will take a look at the insn code.

IIRC Julien's objtool series had some patches had some patches moving
the insn code about, so it'd be worth checking whether that's a help or
a hindrance. If it's possible to split out a set of preparatory patches
that make that ready both for objtool and the kernel, that would make it
easier to review that and queue it early.

>> * The alternatives code, since we call instrumentable and patchable
>>   functions between updating instructions and performing all the
>>   necessary maintenance. There are a number of cases within
>>   __apply_alternatives(), e.g.
>>
>>   - test_bit()
>>   - cpus_have_cap()
>>   - pr_info_once()
>>   - lm_alias()
>>   - alt_cb, if the callback is not marked as noinstr, or if it calls
>>     instrumentable code (e.g. from the insn framework).
>>   - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>>     related code can be instrumented.
>>
>>   This might need some underlying rework elsewhere (e.g. in the
>>   cpufeature code, or atomics framework).
>>
>> So on the kernel side, maybe a first step would be to try to headerize
>> the insn generation code as __always_inline, and see whether that looks
>> ok? With that out of the way it'd be a bit easier to rework patching
>> code depending on the insn framework.
>
> OK. I will study this.

Great, thanks!

Mark.

Re: Live patching on ARM64

[Singh, Balbir]

On 15/1/21 11:33 pm, Mark Rutland wrote:
> On Thu, Jan 14, 2021 at 04:07:55PM -0600, Madhavan T. Venkataraman wrote:
>> Hi all,
>>
>> My name is Madhavan Venkataraman.
> 
> Hi Madhavan,
> 
>> Microsoft is very interested in Live Patching support for ARM64.
>> On behalf of Microsoft, I would like to contribute.
>>
>> I would like to get in touch with the people who are currently working
>> in this area, find out what exactly they are working on and see if they
>> could use an extra pair of eyes/hands with what they are working on.
>>
>> It looks like the most recent work in this area has been from the
>> following folks:
>>
>> Mark Brown and Mark Rutland:
>> 	Kernel changes to providing reliable stack traces.
>>
>> Julien Thierry:
>> 	Providing ARM64 support in objtool.
>>
>> Torsten Duwe:
>> 	Ftrace with regs.
> 
> IIRC that's about right. I'm also trying to make arm64 patch-safe (more
> on that below), and there's a long tail of work there for anyone
> interested.
> 
>> I apologize if I have missed anyone else who is working on Live Patching
>> for ARM64. Do let me know.

I am quite interested as well, I did some of the work for ppc64le

>>
>> Is there any work I can help with? Any areas that need investigation, any code
>> that needs to be written, any work that needs to be reviewed, any testing that
>> needs to done? You folks are probably super busy and would not mind an extra
>> hand.
> 
> One general thing that I believe we'll need to do is to rework code to
> be patch-safe (which implies being noinstr-safe too). For example, we'll
> need to rework the instruction patching code such that this cannot end
> up patching itself (or anything that has instrumented it) in an unsafe
> way.

Do we know how this differs across architectures? Usually kprobe and ftrace
unsafe functions are annotated as such, is there more to it?

> 
> Once we have objtool it should be possible to identify those cases
> automatically. Currently I'm aware that we'll need to do something in at
> least the following places:
> 
> * The entry code -- I'm currently chipping away at this.

Could you please explain, whats bits of the entry code? I suspect we never
patch anything in assembly

> 
> * The insn framework (which is used by some patching code), since the
>   bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>   

noinstr is largely kcsan and kasan related, right?

>   We can probably shift the bulk of the aarch64_insn_gen_*() and
>   aarch64_get_*() helpers into a header as __always_inline functions,
>   which would allow them to be used in noinstr code. As those are
>   typically invoked with a number of constant arguments that the
>   compiler can fold, this /might/ work out as an optimization if the
>   compiler can elide the error paths.
> 
> * The alternatives code, since we call instrumentable and patchable
>   functions between updating instructions and performing all the
>   necessary maintenance. There are a number of cases within
>   __apply_alternatives(), e.g.
> 
>   - test_bit()
>   - cpus_have_cap()
>   - pr_info_once()
>   - lm_alias()
>   - alt_cb, if the callback is not marked as noinstr, or if it calls
>     instrumentable code (e.g. from the insn framework).
>   - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>     related code can be instrumented.
> 
>   This might need some underlying rework elsewhere (e.g. in the
>   cpufeature code, or atomics framework).
> 
> So on the kernel side, maybe a first step would be to try to headerize
> the insn generation code as __always_inline, and see whether that looks
> ok? With that out of the way it'd be a bit easier to rework patching
> code depending on the insn framework.
> 
> I'm not sure about the objtool side, so I'll leave that to Julien and co
> to answer.

Thanks, it would be good to see what the expectations from objtool are,
I thought only x86 needed it due to variable size instructions and -fomit-
frame-pointers

Balbir Singh.