Le 23/10/2020 à 14:28, Christophe Leroy a écrit :
Hi Laurent
Le 07/05/2019 à 18:35, Laurent Dufour a écrit :
Le 01/05/2019 à 12:32, Michael Ellerman a écrit :
Laurent Dufour ldufour@linux.vnet.ibm.com writes:
Le 23/04/2019 à 18:04, Dave Hansen a écrit :
On 4/23/19 4:16 AM, Laurent Dufour wrote:
...
There are 2 assumptions here: 1. 'start' and 'end' are page aligned (this is guaranteed by __do_munmap(). 2. the VDSO is 1 page (this is guaranteed by the union vdso_data_store on powerpc)
Are you sure about #2? The 'vdso64_pages' variable seems rather unnecessary if the VDSO is only 1 page. ;)
Hum, not so sure now ;) I got confused, only the header is one page. The test is working as a best effort, and don't cover the case where only few pages inside the VDSO are unmmapped (start > mm->context.vdso_base). This is not what CRIU is doing and so this was enough for CRIU support.
Michael, do you think there is a need to manage all the possibility here, since the only user is CRIU and unmapping the VDSO is not a so good idea for other processes ?
Couldn't we implement the semantic that if any part of the VDSO is unmapped then vdso_base is set to zero? That should be fairly easy, eg:
if (start < vdso_end && end >= mm->context.vdso_base) mm->context.vdso_base = 0;
We might need to add vdso_end to the mm->context, but that should be OK.
That seems like it would work for CRIU and make sense in general?
Sorry for the late answer, yes this would make more sense.
Here is a patch doing that.
In your patch, the test seems overkill:
+ if ((start <= vdso_base && vdso_end <= end) || /* 1 */ + (vdso_base <= start && start < vdso_end) || /* 3,4 */ + (vdso_base < end && end <= vdso_end)) /* 2,3 */ + mm->context.vdso_base = mm->context.vdso_end = 0;
What about
if (start < vdso_end && vdso_start < end) mm->context.vdso_base = mm->context.vdso_end = 0;
This should cover all cases, or am I missing something ?
And do we really need to store vdso_end in the context ? I think it should be possible to re-calculate it: the size of the VDSO should be (&vdso32_end - &vdso32_start) + PAGE_SIZE for 32 bits VDSO, and (&vdso64_end - &vdso64_start) + PAGE_SIZE for the 64 bits VDSO.
Thanks Christophe for the advise.
That is covering all the cases, and indeed is similar to the Michael's proposal I missed last year.
I'll send a patch fixing this issue following your proposal.
Cheers, Laurent.