Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sat, Jul 05, 2025 at 08:10:03AM +0100, David Laight wrote:
> On Fri, 4 Jul 2025 17:04:02 +0300
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > On Fri, Jul 04, 2025 at 11:40:10AM +0100, David Laight wrote:
> > > On Fri, 4 Jul 2025 05:56:50 +0300
> > > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > >   
> > > > On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:  
> > > ...  
> > > > > Well, that was some truly misguided advice from my side so all the shame
> > > > > here is on me :-) There's no global memzero() and neither explicit
> > > > > version makes much sense here. Sorry about that.
> > > > > 
> > > > > I gave it now (actual) thought, and here's what I'd propose:
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > index 96746d5b03e3..e769f6143a7c 100644
> > > > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> > > > >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > > > >  
> > > > >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > > > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > > > -		       sizeof(struct ffa_send_direct_data2));
> > > > > -
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > > > > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > > > > +			.data = { func_id, a0, a1, a2 },
> > > > > +		};  
> > > 
> > > clang has a habit of compiling that as an un-named on-stack structure that
> > > is initialised and then memcpy() used to copy it into place.
> > > Often not was intended and blows the stack when the structure is large.
> > > 
> > > So probably not a pattern that should be encouraged.  
> > 
> > This is interesting observation so I had to do some compilation tests to
> > verify the claim just to see how it plays out (and for the sake of
> > learning while doing it).
> > 
> > Note that I use GCC for the examples but I have high doubts that clang
> > would do worse. Please share the insight if that is a wrong assumption.
> 
> It is a clang issue and may only affect builds with some of the 'memory
> sanitiser' run-time checks.
> Search through the mail archives for issues with overlarge stack frames.

If clang really did that, it would cost at worst 40 bytes of stack (aka
the size of struct ffa_send_direct_data. And if there is an issue, it
absolutely will get fixed.

That does not weight over making a code change that makes the most sense
for Linux in the long-term. And to add, I did show both the code and
the figures to support my claim.
 

> 
> 	David

BR, Jarkko