Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN | From | Gary R Hook <> | Date | Tue, 28 Mar 2017 09:15:45 -0500 |
| |
On 03/28/2017 04:58 AM, Arnd Bergmann wrote:> The newly added AES GCM implementation uses one of the largest stack frames > in the kernel, around 1KB on normal 64-bit kernels, and 1.6KB when > CONFIG_KASAN > is enabled: > > drivers/crypto/ccp/ccp-ops.c: In function 'ccp_run_aes_gcm_cmd': > drivers/crypto/ccp/ccp-ops.c:851:1: error: the frame size of 1632 bytes > is larger than 1536 bytes [-Werror=frame-larger-than=] > > This is problematic for multiple reasons: > > - The crypto functions are often used in deep call chains, e.g. behind > mm, fs and dm layers, making it more likely to run into an actual stack > overflow > > - Using this much stack space is an indicator that the code is not > written to be as efficient as it could be.
I'm not sure I agree that A -> B, but I will certainly look into this.
> - While this goes unnoticed at the moment in mainline with the frame size > warning being disabled when KASAN is in use, I would like to enable > the warning again, and the current code is slightly above my arbitrary > pick for a limit of 1536 bytes (I already did patches for every other > driver exceeding this).
I've got my stack frame size (also) set to 1536, and would have paid more attention had a warning occurred due to my code.
> A more drastic refactoring of the driver might be needed to reduce the > stack usage more substantially, but this patch is fairly simple and > at least addresses the third one of the problems I mentioned, reducing the > stack size by about 150 bytes and bringing it below the warning limit > I picked.
Again, I'll devote some time to this.
> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h > index 3a45c2af2fbd..c5ea0796a891 100644 > --- a/drivers/crypto/ccp/ccp-dev.h > +++ b/drivers/crypto/ccp/ccp-dev.h > @@ -432,24 +432,24 @@ struct ccp_dma_info { > unsigned int offset; > unsigned int length; > enum dma_data_direction dir; > -}; > +} __packed __aligned(4);
My gcc 4.8 doesn't understand __aligned(). Shouldn't we use #pragma(4) here?
> struct ccp_dm_workarea { > struct device *dev; > struct dma_pool *dma_pool; > - unsigned int length; > > u8 *address; > struct ccp_dma_info dma; > + unsigned int length; > }; > > struct ccp_sg_workarea { > struct scatterlist *sg; > int nents; > + unsigned int dma_count; > > struct scatterlist *dma_sg; > struct device *dma_dev; > - unsigned int dma_count; > enum dma_data_direction dma_dir; > > unsigned int sg_used;
I'm okay with rearranging, but I'm going to submit an alternative patch.
| ![\](/images/icornerr.gif) |