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

Mark Rutland
2020-04-14T16:28:07Z

On Tue, Apr 14, 2020 at 04:52:48PM +0100, Will Deacon wrote:
> 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.

I hadn't considered that, but having taken a look just now I think that
gets quite grotty and more subtle than what we have today, and I think
that using the vdso_start/vdso_end symbols as we do now leaves the least
scope for issues.

Thanks,
Mark.