Re: [mac80211] BUG_ON with current -git (4.8.0-11417-g24532f7)

On Fri, 2016-10-14 at 10:05 +0100, Ard Biesheuvel wrote:

Indeed. And the decrypt path does the same for auth_tag[].

Hadn't gotten that far, due to the BUG_ON() in CONFIG_DEBUG_SG in the
encrypt path :)

But that still means there are two separate problems here, one which
affects the WPA code, and one that only affects the generic CCM
chaining mode (but not the accelerated arm64 implementation)

Yes. The generic CCM chaining still doesn't typically have a request on
the stack though. In fact, ESP (net/ipv4/esp4.c) for example will do
temporary allocations with kmalloc for every frame, it seems.

Unsurprisingly, I would strongly prefer those to be fixed properly
rather than backing out my patch, but I'm happy to help out whichever
solution we reach consensus on.

Yeah, obviously, it would be good to use the accelerated versions after
all.

I will check whether this removes the issue when not using
crypto/ccm.ko

Ok. I think we can probably live with having those 48 bytes in per-CPU
buffers, but I suppose we don't really want to have ~500.

johannes

On 14 October 2016 at 10:10, Johannes Berg [off-list ref] wrote:

On Fri, 2016-10-14 at 10:05 +0100, Ard Biesheuvel wrote:

quoted

Indeed. And the decrypt path does the same for auth_tag[].

Hadn't gotten that far, due to the BUG_ON() in CONFIG_DEBUG_SG in the
encrypt path :)

quoted

But that still means there are two separate problems here, one which
affects the WPA code, and one that only affects the generic CCM
chaining mode (but not the accelerated arm64 implementation)

Yes. The generic CCM chaining still doesn't typically have a request on
the stack though. In fact, ESP (net/ipv4/esp4.c) for example will do
temporary allocations with kmalloc for every frame, it seems.

It is annotated with a TODO, though :-)

38320c70d282b (Herbert Xu               2008-01-28 19:35:05 -0800  41)
 * TODO: Use spare space in skb for this where possible.

quoted

Unsurprisingly, I would strongly prefer those to be fixed properly
rather than backing out my patch, but I'm happy to help out whichever
solution we reach consensus on.

Yeah, obviously, it would be good to use the accelerated versions after
all.

quoted

I will check whether this removes the issue when not using
crypto/ccm.ko

Ok. I think we can probably live with having those 48 bytes in per-CPU
buffers, but I suppose we don't really want to have ~500.

Agreed.