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

Will Deacon
2020-04-14T15:53:07Z

On Tue, Apr 14, 2020 at 04:43:19PM +0100, Mark Rutland wrote:
> 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.

I was wondering whether we could extend gen_vdso_offsets.sh to emit this
information. Why isn't looking at the shared object enough? That said,
it does get grotty so maybe it's not worth doing.

> 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.

Yeah, I'm not really worried about that since I think we only fail if a
subsequent allocation fails, it's just that the dynamic allocation seemed
a bit OTT on first glance.

Will