Re: [PATCH 0/5] arm64: vdso: cleanups

Mark Rutland
2020-04-14T15:43:30Z

On Tue, Apr 14, 2020 at 04:20:58PM +0100, Will Deacon wrote:
> On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > code was somewhat baroque, making it harder than necessary to understand and
> > extend. These patches attempt to improve this by making the code more
> > consistent and avoiding unnecessary duplication.
> >
> > The first patch in the series fixes a bug in a boot time error path. This bug
> > was made obvious during the refactoring but I've moved it to the start so that
> > it can be backported more easily.
> >
> > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > branch [1].
>
> Cheers, this looks really good to me. The only thing I'm slightly confused
> by is that we still have something like this in __vdso_init():
>
> /* Allocate the vDSO pagelist, plus a page for the data. */
> vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
> sizeof(struct page *),
> GFP_KERNEL);
>
> But I don't see why this needs to be dynamic, and don't we leak the
> allocation on failure? Not a big deal, but seems silly if we could just
> have a couple of static page * arrays.

Unfortunately it has to be dynamic as the number of vdso code pages
isn't known until the vdso is linked into the kernel proper. The only
way to allocate that at build time would be as part of the linker
script, and I think that'd be far more confusing.

We calculate the number of pages from the bounds of the code:

| vdso_info[abi].vdso_pages = (
| vdso_info[abi].vdso_code_end -
| vdso_info[abi].vdso_code_start) >>
| PAGE_SHIFT;
|
| /* Allocate the vDSO pagelist, plus a page for the data. */
| vdso_pagelist = kcalloc(vdso_info[abi].vdso_pages + 1,
| sizeof(struct page *),
| GFP_KERNEL);

... that said, conflating the data page list and code page list is
harming legibility, and I reckon that's worth cleaning up. The data page
list can be statically allocate given it's a single page.

There is a leak if we fail to allocate the compat vdso pages, but I
don't see a nice way of cleaning that up. It looks like
do_one_initcall() will WARN() in that case as we'll return a non-zero
error code.

Thanks,
Mark.