[PATCH v3 1/2] rtld: Enable MTE for stack when specified in .dynamic

[Claudiu Zissulescu]

> 
> This patch should have at least one test checking that the added
> functionality works as intended for each supported mode of stack
> tagging. Ideally, we should check both statically-linked and
> dynamically-linked cases.

A test is provided in the second patch. I can add it to this one.

> 
> Some notes below. In a nutshell, I think this experimental functionality
> should be hidden behind a configure flag or at least a tunable.
> 
> Note that there already is a configure flag "--enable-memory-tagging"
> which adds -DUSE_MTAG macro and unlock heap tagging in malloc which is
> why it's probably not a good idea to use the same guard for stack
> tagging.
> 
> Whatever way to disable stack tagging is chosen, it should somehow
> co-exist with the "--enable-memory-tagging" configure flag and the
> corresponding tunable "glibc.mem.tagging". Both of these two things
> may change soon to make memory tagging in malloc more usable. For this
> reason I would encourage a conversation about how these two things
> (tagging of heap and stack) can be controlled independently.

I am open for suggestions. A tunable may be a solution, let's see others 
input on this topic too.

>> +#define ARCH_MTE_MODE_SYNC (1 << 1)
>> +#define ARCH_MTE_MODE_ASYNC (1 << 2)
> 
> These two macros seem to be unused. If they are meant for the possible
> values of DT_AARCH64_MEMTAG_MODE, then they would be incorrect because
> possible values are 0 for sync and 1 for async mode.

Noted.

>> +
>> +void
>> +_dl_mte_stack_check (struct link_map *l,
>> +		     const char *program __attribute__((unused)))
>> +{
> 
> Shouldn't there be a check for
> 
>    GLRO(dl_hwcap2) & HWCAP2_MTE
> 
> somewhere (not necessarily here)?

AFAIK, the checking for the MTE capability is done before calling this 
function. I'll double check this.

> 
>> +  ElfW (Dyn) *d;
>> +  bool mte_enabled = false;
>> +  bool mte_mode_selected = false;
>> +
>> +  for (d = l->l_ld; d->d_tag != DT_NULL; ++d)
>> +    {
>> +      if (d->d_tag == DT_AARCH64_MEMTAG_STACK)
>> +	{
>> +	  GLRO (dl_aarch64_cpu_features).mte_state
>> +	    |= AARCH64_CPU_FEATURE_MTE_STATE_STACK;
> 
> This and...
> 
>> +	  mte_enabled = true;
>> +	}
>> +      else if (d->d_tag == DT_AARCH64_MEMTAG_MODE)
>> +	{
>> +	  GLRO (dl_aarch64_cpu_features).mte_state
>> +	    &= ~AARCH64_CPU_FEATURE_MTE_STATE_MODE_MASK;
> 
> this conflicts with use of GLRO (dl_aarch64_cpu_features).mte_state for
> heap memory tagging in malloc.
> > I think we should think about combination of these two features: how
> will they co-exist and interact.

The user application can set it's own MTE behavior, sync or 
asynchronous. I can understand this may impact other setting. Shall we 
remove this feature from user control, or shall we have it as tunable? 
Any other suggestions?

>> +
>> +  return (err != 0
>> +	  && (GLRO (dl_aarch64_cpu_features).mte_state
>> +	      & AARCH64_CPU_FEATURE_MTE_STATE_MODE_MASK) != 0);
> 
> Return value is currently unused. What's the model for a failure
> returned from the prctl syscall?
> 
Any suggestion is welcome here.

Best wishes,
Claudiu