Re: [PATCH] kernel: dma: Make CMA boot parameters __ro_after_init

[kbuild test robot]

CC: kbuil...@lists.01.org
In-Reply-To: <20191012122918.80...@gmail.com>
References: <20191012122918.80...@gmail.com>
TO: Shyam Saini <mayhs1...@gmail.com>
CC: kernel-h...@lists.openwall.com, io...@lists.linux-foundation.org, linux-...@vger.kernel.org, linu...@kvack.org, Shyam Saini <mayhs1...@gmail.com>, Christoph Hellwig <h...@lst.de>, Marek Szyprowski <m.szyp...@samsung.com>, Robin Murphy <robin....@arm.com>, Matthew Wilcox <wi...@infradead.org>, Christopher Lameter <c...@linux.com>, Kees Cook <kees...@chromium.org>, io...@lists.linux-foundation.org, linux-...@vger.kernel.org, linu...@kvack.org, Shyam Saini <mayhs1...@gmail.com>, Christoph Hellwig <h...@lst.de>, Marek Szyprowski <m.szyp...@samsung.com>, Robin Murphy <robin....@arm.com>, Matthew Wilcox <wi...@infradead.org>, Christopher Lameter <c...@linux.com>, Kees Cook <kees...@chromium.org>
CC: io...@lists.linux-foundation.org, linux-...@vger.kernel.org, linu...@kvack.org, Shyam Saini <mayhs1...@gmail.com>, Christoph Hellwig <h...@lst.de>, Marek Szyprowski <m.szyp...@samsung.com>, Robin Murphy <robin....@arm.com>, Matthew Wilcox <wi...@infradead.org>, Christopher Lameter <c...@linux.com>, Kees Cook <kees...@chromium.org>

Hi Shyam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.4-rc2 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Shyam-Saini/kernel-dma-Make-CMA-boot-parameters-__ro_after_init/20191014-011828
config: arm64-defconfig (attached as .config)
compiler: clang version 10.0.0 (git://gitmirror/llvm_project 1f5823b788037b0dc8c846a22ef6a5a8a0714af7)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <l...@intel.com>

All errors (new ones prefixed by >>):

>> kernel/dma/contiguous.c:46:36: error: 'size_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init size_cmdline = -1;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
>> kernel/dma/contiguous.c:47:36: error: 'base_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init base_cmdline;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
>> kernel/dma/contiguous.c:48:36: error: 'limit_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init limit_cmdline;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
3 errors generated.

vim +46 kernel/dma/contiguous.c

34
35 /*
36 * Default global CMA area size can be defined in kernel's .config.
37 * This is useful mainly for distro maintainers to create a kernel
38 * that works correctly for most supported systems.
39 * The size can be set in bytes or as a percentage of the total memory
40 * in the system.
41 *
42 * Users, who want to set the size of global CMA area for their system
43 * should use cma= kernel parameter.
44 */
> 45 static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
> 46 static phys_addr_t __ro_after_init size_cmdline = -1;
> 47 static phys_addr_t __ro_after_init base_cmdline;
> 48 static phys_addr_t __ro_after_init limit_cmdline;
49

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

[Nathan Chancellor]

On Sat, Oct 12, 2019 at 05:59:18PM +0530, Shyam Saini wrote:
> This parameters are not changed after early boot.
> By making them __ro_after_init will reduce any attack surface in the
> kernel.
>
> Link: https://lwn.net/Articles/676145/
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Marek Szyprowski <m.szyp...@samsung.com>
> Cc: Robin Murphy <robin....@arm.com>
> Cc: Matthew Wilcox <wi...@infradead.org>
> Cc: Christopher Lameter <c...@linux.com>
> Cc: Kees Cook <kees...@chromium.org>
> Signed-off-by: Shyam Saini <mayhs1...@gmail.com>
> ---
> kernel/dma/contiguous.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 69cfb4345388..1b689b1303cd 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -42,10 +42,10 @@ struct cma *dma_contiguous_default_area;

> * Users, who want to set the size of global CMA area for their system

> * should use cma= kernel parameter.

> */
> -static const phys_addr_t size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
> -static phys_addr_t size_cmdline = -1;
> -static phys_addr_t base_cmdline;
> -static phys_addr_t limit_cmdline;
> +static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;

The 0day bot reported an issue with this change with clang:

https://groups.google.com/d/msgid/clang-built-linux/201910140334.nhultlt8%25lkp%40intel.com

kernel/dma/contiguous.c:46:36: error: 'size_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init size_cmdline = -1;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
kernel/dma/contiguous.c:47:36: error: 'base_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init base_cmdline;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
kernel/dma/contiguous.c:48:36: error: 'limit_cmdline' causes a section type conflict with 'size_bytes'
static phys_addr_t __ro_after_init limit_cmdline;
^
kernel/dma/contiguous.c:45:42: note: declared here
static const phys_addr_t __ro_after_init size_bytes = (phys_addr_t)CMA_SIZE_MBYTES * SZ_1M;
^
3 errors generated.

The errors seem kind of cryptic at first but something that is const
should automatically be in the read only section, this part of the
commit seems unnecessary. Removing that part of the change fixes the error.

Cheers,
Nathan

[Nathan Chancellor]

Reported on the patch's thread so that it does not get applied:

https://lore.kernel.org/lkml/20191014022543.GA2674@ubuntu-m2-xlarge-x86/

Cheers,
Nathan

[Shyam Saini]

Hi Nathan,

Thanks for your feedback and reporting this error.

> The errors seem kind of cryptic at first but something that is const
> should automatically be in the read only section, this part of the
> commit seems unnecessary. Removing that part of the change fixes the error.

I have overlooked size_bytes variable
It shouldn't be const if it is declared as __ro_after_init.

I will fix and resend it.