Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* linux-next: manual merge of the sound-asoc tree with the crypto tree
@ 2020-05-12  4:49 Stephen Rothwell
  2020-05-12 16:22 ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Rothwell @ 2020-05-12  4:49 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood, Herbert Xu, Linux Crypto List
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Eric Biggers,
	Arnd Bergmann


[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12  4:49 linux-next: manual merge of the sound-asoc tree with the crypto tree Stephen Rothwell
@ 2020-05-12 16:22 ` Mark Brown
  2020-05-12 16:36   ` Eric Biggers
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2020-05-12 16:22 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Liam Girdwood, Herbert Xu, Linux Crypto List,
	Linux Next Mailing List, Linux Kernel Mailing List, Eric Biggers,
	Arnd Bergmann


[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 16:22 ` Mark Brown
@ 2020-05-12 16:36   ` Eric Biggers
  2020-05-12 17:08     ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-05-12 16:36 UTC (permalink / raw)
  To: Mark Brown
  Cc: Stephen Rothwell, Liam Girdwood, Herbert Xu, Linux Crypto List,
	Linux Next Mailing List, Linux Kernel Mailing List,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 16:36   ` Eric Biggers
@ 2020-05-12 17:08     ` Mark Brown
  2020-05-12 20:08       ` Eric Biggers
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2020-05-12 17:08 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Stephen Rothwell, Liam Girdwood, Herbert Xu, Linux Crypto List,
	Linux Next Mailing List, Linux Kernel Mailing List,
	Arnd Bergmann


[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 17:08     ` Mark Brown
@ 2020-05-12 20:08       ` Eric Biggers
  2020-05-12 20:31         ` Arnd Bergmann
  2020-05-13  2:52         ` Herbert Xu
  0 siblings, 2 replies; 8+ messages in thread
From: Eric Biggers @ 2020-05-12 20:08 UTC (permalink / raw)
  To: Mark Brown, Herbert Xu
  Cc: Stephen Rothwell, Liam Girdwood, Linux Crypto List,
	Linux Next Mailing List, Linux Kernel Mailing List,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 20:08       ` Eric Biggers
@ 2020-05-12 20:31         ` Arnd Bergmann
  2020-05-14 13:21           ` Ard Biesheuvel
  2020-05-13  2:52         ` Herbert Xu
  1 sibling, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2020-05-12 20:31 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Mark Brown, Herbert Xu, Stephen Rothwell, Liam Girdwood,
	Linux Crypto List, Linux Next Mailing List,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 20:08       ` Eric Biggers
  2020-05-12 20:31         ` Arnd Bergmann
@ 2020-05-13  2:52         ` Herbert Xu
  1 sibling, 0 replies; 8+ messages in thread
From: Herbert Xu @ 2020-05-13  2:52 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Mark Brown, Stephen Rothwell, Liam Girdwood, Linux Crypto List,
	Linux Next Mailing List, Linux Kernel Mailing List,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree
  2020-05-12 20:31         ` Arnd Bergmann
@ 2020-05-14 13:21           ` Ard Biesheuvel
  0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2020-05-14 13:21 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Eric Biggers, Mark Brown, Herbert Xu, Stephen Rothwell,
	Liam Girdwood, Linux Crypto List, Linux Next Mailing List,
	Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12  4:49 linux-next: manual merge of the sound-asoc tree with the crypto tree Stephen Rothwell
2020-05-12 16:22 ` Mark Brown
2020-05-12 16:36   ` Eric Biggers
2020-05-12 17:08     ` Mark Brown
2020-05-12 20:08       ` Eric Biggers
2020-05-12 20:31         ` Arnd Bergmann
2020-05-14 13:21           ` Ard Biesheuvel
2020-05-13  2:52         ` Herbert Xu

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git