mbedtls: remove static keyword from certain function pointers

[SebastianBoe]

TF-M usually downloads mbedtls and then applies a handful of
patches. These patches were forgotten when Zephyr started to use it's
own mbedtls fork with TF-M.

One of these patches has now been re-applied in this commit. It is
needed for an important code sharing optimization documented in a link
below.

The static keyword is removed from three function pointer
declarations. Aside from allowing the optimization described below
this could theoretically backfire.

There could be other non-static symbols that now trigger aliasing
issues.

The compiler could have trouble optimizing now that it doesn't know of
all symbol usages.

But I believe that in practice this will be OK.

I don't know if there are any plans to upstream this patch to mbedtls
itself.

This partially addresses review feedback given in zephyrproject-rtos/zephyr#34263

https://tf-m-user-guide.trustedfirmware.org/docs/technical_references/design_docs/code_sharing.html

Original patch:
https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/commit/lib/ext/mbedcrypto?id=4a5cc9776ed3b053bac57326931f936fbbc660e9

Signed-off-by: Sebastian Bøe sebastian.boe@nordicsemi.no

[tejlmand]

I don't know if there are any plans to upstream this patch to mbedtls
itself.

Let's hope.

But beside that, should we apply TF-M specific patches in the Zephyr mbedtls fork ?
That fork is also being used outside TF-M scope, in which case TF-M specific patches may not be desired in some cases.

This particular PR looks ok to me, so this is more a general question on how we want to have a single mbedtls fork that is suitable for both Zephyr without TF-M, and TF-M in Zephyr.

One option could be to apply those patches in a tfm-patch branch, like:
https://github.com/zephyrproject-rtos/mbedtls/tree/master
https://github.com/zephyrproject-rtos/mbedtls/tree/tfm-patch

and then in west.yml we could have:

    - name: mbedtls
      revision: 5765cb7f75a9973ae9232d438e361a9d7bbc49e7
      path: modules/crypto/mbedtls
    - name: mbedtls-tfm
      repo-path: mbedtls
      revision: <SHA in the tfm-patch branch>
      path: modules/crypto/mbedtls-tfm

Other ideas @mbolivar-nordic @microbuilder ?

[SebastianBoe]

Currently I am hoping that TF-M never patches mbedtls in a way that is incompatible with our usage.

I have reviewed the three patches in TF-M master.

One is irrelevant as it is about IAR support which we don't support (Last time I checked).

This PR covers the second one.

And the third I am still working on understanding, but at first glance it looks harmless.

[tejlmand]

Currently I am hoping that TF-M never patches mbedtls in a way that is incompatible with our usage.

Let's hope.

[SebastianBoe]

The third patch is harmless. It is patching code that is dead in a Zephyr context.

[d3zd3z]

As a deeper issue, static inline is always incorrect to have in a header file. Well, incorrect in the sense that it has the potential to cause lots of code duplication. But, it seems to get used all of the place. The correct solution is to have extern inline in the header, but then in one instance where it gets included, not have the extern inline and just declare it as a function. Usually this is done by defining a macro that expands to extern inline most of the time, but redefine that for this single instance. (see the gcc docs at the end for more details).

[d3zd3z]

Should this be submitted upstream as well? We've gotten to the point of not having any patches against mbed TLS to carry.

[tejlmand]

Should this be submitted upstream as well? We've gotten to the point of not having any patches against mbed TLS to carry.

I would say that should be the TF-M project upstreaming that.
It's their patch @SebastianBoe is applying here.

[tejlmand]

As a deeper issue, static inline is always incorrect to have in a header file.

yes, but what does that have to do with this PR ?
The patch removes the static keyword in a C-file, not a header.
And there is no inline keyword.
Removing the static keyword means the compiler can no longer optimize the code as effectively as it could before.

[mbolivar-nordic]

Other ideas @mbolivar-nordic @microbuilder ?

seems like you are happy with this solution, so no comments from me

[microbuilder]

@tejlmand I agree this could get complicated with TF-M having one set of expectations that might not be true of other projects using the same Zephyr repo. Long term, a tfm-patch branch might make sense, but that's also a maintenance burden that needs to be documented, and what changes need to be made per branch when we upgrade (which is the problem I'm having right now with TF-M 1.4.0).

This change seems fine to me, though, and doesn't force our hand yet on the above issue.

[d3zd3z]

Removing the static keyword means the compiler can no longer optimize the code as effectively as it could before.

Probably not best to discuss this here, since, as you mention it really has nothing to do with this PR. But, no, removing static does not affect the code generation. As mentioned in the gcc manual, the inline should be declared 'extern inline' for all but one C file that includes it, and then in a single instance, it should be declared with neither keyword in one place, to provide a non-inline version for the times that gcc decided it would be better to not inline the function. With static inline the inlined version is identical, but if the compiler decides to not inline, it will generate a non-inline version, separately for each C file where this happens. For larger inlines, this can result in quite a bit of extra code.

[SebastianBoe]

I believe what you are referring to applies only to functions, not function pointers.

[microbuilder]

@d3zd3z Is this ready to be merged in?

[tejlmand]

Other ideas @mbolivar-nordic @microbuilder ?

seems like you are happy with this solution, so no comments from me

Not happy but acceptable in this particular case.
I still have my concerns of applying TF-M specific patches to our mbedTLS fork from a general point of view.

Thus I simply want to know your opinions on the matter and also if you had other ideas / comments.

[microbuilder]

Other ideas @mbolivar-nordic @microbuilder ?

seems like you are happy with this solution, so no comments from me

Not happy but acceptable in this particular case. I still have my concerns of applying TF-M specific patches to our mbedTLS fork from a general point of view.

Thus I simply want to know your opinions on the matter and also if you had other ideas / comments.

I agree I don't love the idea, but I won't block it either if no one has strong feelings NOT to do this.

[d3zd3z]

I think the correct place to fix this is in TF-M. I don't think this would go over well with upstream, since there are functions to set these pointers (they are supposed to be internal). Any idea why TF-M doesn't just use those, or does it need to use the pointers themselves? Can it not just call the Mbed TLS allocate/free functions.

But, I'll go ahead an merge this for now, so things work.

[d3zd3z]

Actually, where is this fix intended to go? zephyr's 'main' branch is now using the 'zephyr' branch ok mbedtls. This 'master' branch would only apply to back ports for 2.7. Do we need this fix to go into 2.7?

[SebastianBoe]

Do we need this fix to go into 2.7?

We do not need this for 2.7 so we can wait until after release.

[SebastianBoe]

Any idea why TF-M doesn't just use those, or does it need to use the pointers themselves? Can it not just call the Mbed TLS allocate/free functions.

This is due to a limitation in the code sharing mechanism. File scope variables AKA static global variables that are used by shared functions must be made into non-static global variables. TF-M is calling the (shared) TLS allocate/free functions. This limitation is mentioned here:

https://tf-m-user-guide.trustedfirmware.org/docs/technical_references/design_docs/code_sharing.html#useability-considerations

[d3zd3z]

This is due to a limitation in the code sharing mechanism. File scope variables AKA static global variables that are used by shared functions must be made into non-static global variables.

I guess my question is more along the lines of: why is TF-M accessing these at all. They are intended to be private, and internal to Mbed TLS. It should be calling mbedtls_calloc() if it wants to use the allocate, or mbedtls_free() if it wants to use free. It shouldn't be accessing the function pointers.

[SebastianBoe]

It isn't accessing them directly in firmware, there is a python script that needs to know where in RAM it is placed. It is calling mbedtls_calloc.

See the linked code sharing docs for why the pointer needs to be non-static.

[d3zd3z]

So, as I understand, in order to share these symbols, we have to know their address. I'm not certain that these should actually be shared, and they seem like values that should be unique per "client" that uses the library. Is this something the code sharing mechanism supports? It seems reasonable that one application might want to use a different allocator than other code using it.

Regardless, it seems we will have to keep carrying this patch. It seems rather unlikely we would be able to get this patch upstream, given that there is both a setter and an accessor for these values.

[SebastianBoe]

This was merged to the master branch but the master branch is deprecated so it should have been merged to the zephyr branch.