linux-next: manual merge of the sound-asoc tree with the crypto tree

linux-next: manual merge of the sound-asoc tree with the crypto tree

[Stephen Rothwell]

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

Hi all,

Today's linux-next merge of the sound-asoc tree got a conflict in:

  sound/soc/codecs/cros_ec_codec.c

between commit:

  85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()")

from the crypto tree and commit:

  a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically")

from the sound-asoc tree.

I fixed it up (I just used the former) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging.  You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

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

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Mark Brown]

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

On Tue, May 12, 2020 at 02:49:49PM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the sound-asoc tree got a conflict in:
> 
>   sound/soc/codecs/cros_ec_codec.c
> 
> between commit:
> 
>   85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()")
> 

Oh, this is the first I've heard of that patch :(

> from the crypto tree and commit:

>   a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically")

> from the sound-asoc tree.

> I fixed it up (I just used the former) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging.  You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.

That doesn't seem ideal - Eric, Herbert can we get a branch with the
crypto patches in them to pull into the ASoC tree or something?

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

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Eric Biggers]

On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote:
> On Tue, May 12, 2020 at 02:49:49PM +1000, Stephen Rothwell wrote:
> > Hi all,
> > 
> > Today's linux-next merge of the sound-asoc tree got a conflict in:
> > 
> >   sound/soc/codecs/cros_ec_codec.c
> > 
> > between commit:
> > 
> >   85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()")
> > 
> 
> Oh, this is the first I've heard of that patch :(
> 
> > from the crypto tree and commit:
> 
> >   a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically")
> 
> > from the sound-asoc tree.

I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER".
I guess wasn't enough and I should have added alsa-devel@alsa-project.org too?

> 
> > I fixed it up (I just used the former) and can carry the fix as
> > necessary. This is now fixed as far as linux-next is concerned, but any
> > non trivial conflicts should be mentioned to your upstream maintainer
> > when your tree is submitted for merging.  You may also want to consider
> > cooperating with the maintainer of the conflicting tree to minimise any
> > particularly complex conflicts.
> 
> That doesn't seem ideal - Eric, Herbert can we get a branch with the
> crypto patches in them to pull into the ASoC tree or something?

We should just drop one of the patches.

If you just want to eliminate the compiler warning about stack usage in
wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves
the stack allocation into another function in another file.

Alternatively, if you actually need to reduce the total stack usage (across all
functions), then the kmalloc() patch in sound-asoc would be better.

Which do you prefer?

- Eric

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Mark Brown]

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

On Tue, May 12, 2020 at 09:36:32AM -0700, Eric Biggers wrote:
> On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote:

> > > from the crypto tree and commit:

> > >   a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically")

> > > from the sound-asoc tree.

> I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER".
> I guess wasn't enough and I should have added alsa-devel@alsa-project.org too?

Yeah, you generally need to make sure the subsystem maintainers are
included as well as individual driver maintainers.

> > That doesn't seem ideal - Eric, Herbert can we get a branch with the
> > crypto patches in them to pull into the ASoC tree or something?

> We should just drop one of the patches.

> If you just want to eliminate the compiler warning about stack usage in
> wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves
> the stack allocation into another function in another file.

> Alternatively, if you actually need to reduce the total stack usage (across all
> functions), then the kmalloc() patch in sound-asoc would be better.

Well, reducing the stack usage overall seems nicer overall - heads off
future problems if the struct grows or something, and if we still end up
allocating just as much on the stack then we'll have trouble at runtime
anyway - does that make sense?

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

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Eric Biggers]

On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote:
> On Tue, May 12, 2020 at 09:36:32AM -0700, Eric Biggers wrote:
> > On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote:
> 
> > > > from the crypto tree and commit:
> 
> > > >   a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically")
> 
> > > > from the sound-asoc tree.
> 
> > I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER".
> > I guess wasn't enough and I should have added alsa-devel@alsa-project.org too?
> 
> Yeah, you generally need to make sure the subsystem maintainers are
> included as well as individual driver maintainers.
> 
> > > That doesn't seem ideal - Eric, Herbert can we get a branch with the
> > > crypto patches in them to pull into the ASoC tree or something?
> 
> > We should just drop one of the patches.
> 
> > If you just want to eliminate the compiler warning about stack usage in
> > wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves
> > the stack allocation into another function in another file.
> 
> > Alternatively, if you actually need to reduce the total stack usage (across all
> > functions), then the kmalloc() patch in sound-asoc would be better.
> 
> Well, reducing the stack usage overall seems nicer overall - heads off
> future problems if the struct grows or something, and if we still end up
> allocating just as much on the stack then we'll have trouble at runtime
> anyway - does that make sense?

If you're concerned about total stack usage, then my recommendation is that
Herbert drops my patch "ASoC: cros_ec_codec: use crypto_shash_tfm_digest()"
from cryptodev, and you keep the patch
"ASoC: cros_ec_codec: allocate shash_desc dynamically" in sound-asoc.

For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a
better solution, since lots of users are using this macro.  A version of
crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is
too large would be possible, but that wouldn't fully solve the problem since
some users do incremental hashing.

- Eric

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Arnd Bergmann]

On Tue, May 12, 2020 at 10:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote:
>
> For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a
> better solution, since lots of users are using this macro.  A version of
> crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is
> too large would be possible, but that wouldn't fully solve the problem since
> some users do incremental hashing.

It's hard to know how many of the users of SHASH_DESC_ON_STACK() are
likely to cause problems, as multiple factors are involved:

- this one triggered the warning because it was on the stack of a function
  that got inlined into another that has other large variables. Whether it
  got inlined makes little difference to the stack usage, but does make a
  difference to warning about it.

- generally the structure is larger than we like it, especially on architectures
  with 128 byte CRYPTO_MINALIGN like ARM. This actually got worse
  because of b68a7ec1e9a3 ("crypto: hash - Remove VLA usage"), as
  the stack usage is now always the maximum of all hashes where it used
  to be specific to the hash that was actually used and could be smaller

- the specific instance in calculate_sha256() feels a bit silly, as this
  function allocates a tfm and a descriptor, runs the digest and then
  frees both again. I don't know how common this pattern is, but
  it seems a higher-level abstraction might be helpful anyway.

      Arnd

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Herbert Xu]

On Tue, May 12, 2020 at 01:08:05PM -0700, Eric Biggers wrote:
>
> If you're concerned about total stack usage, then my recommendation is that
> Herbert drops my patch "ASoC: cros_ec_codec: use crypto_shash_tfm_digest()"
> from cryptodev, and you keep the patch
> "ASoC: cros_ec_codec: allocate shash_desc dynamically" in sound-asoc.

OK I'll drop this patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: linux-next: manual merge of the sound-asoc tree with the crypto tree

[Ard Biesheuvel]

On Tue, 12 May 2020 at 22:31, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, May 12, 2020 at 10:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote:
> >
> > For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a
> > better solution, since lots of users are using this macro.  A version of
> > crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is
> > too large would be possible, but that wouldn't fully solve the problem since
> > some users do incremental hashing.
>
> It's hard to know how many of the users of SHASH_DESC_ON_STACK() are
> likely to cause problems, as multiple factors are involved:
>
> - this one triggered the warning because it was on the stack of a function
>   that got inlined into another that has other large variables. Whether it
>   got inlined makes little difference to the stack usage, but does make a
>   difference to warning about it.
>
> - generally the structure is larger than we like it, especially on architectures
>   with 128 byte CRYPTO_MINALIGN like ARM. This actually got worse
>   because of b68a7ec1e9a3 ("crypto: hash - Remove VLA usage"), as
>   the stack usage is now always the maximum of all hashes where it used
>   to be specific to the hash that was actually used and could be smaller
>
> - the specific instance in calculate_sha256() feels a bit silly, as this
>   function allocates a tfm and a descriptor, runs the digest and then
>   frees both again. I don't know how common this pattern is, but
>   it seems a higher-level abstraction might be helpful anyway.
>

We are trying to move to crypto library interfaces for non-performance
critical uses of hashes where the algorithm is known at compile time,
and this is a good example of that pattern.

IOW, this code should just call the sha256_init/update/final routines directly.

I'll send out a patch.