Re: [PATCH v4 18/26] arm64: vdso32: Replace TASK_SIZE_32 check in vgettimeofday

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Vincenzo,

On Thu, Mar 19, 2020 at 12:38:42PM +0000, Vincenzo Frascino wrote:
> On 3/18/20 6:36 PM, Catalin Marinas wrote:
> > On Wed, Mar 18, 2020 at 04:14:26PM +0000, Vincenzo Frascino wrote:
> >> On 3/17/20 5:48 PM, Catalin Marinas wrote:
> >>> So clock_gettime() on arm32 always falls back to the syscall?
> >>
> >> This seems not what you asked, and I think I answered accordingly. Anyway, in
> >> the case of arm32 the error code path is handled via syscall fallback.
> >>
> >> Look at the code below as an example (I am using getres because I know this
> >> email will be already too long, and I do not want to add pointless code, but the
> >> concept is the same for gettime and the others):
> >>
> >> static __maybe_unused
> >> int __cvdso_clock_getres(clockid_t clock, struct __kernel_timespec *res)
> >> {
> >> 	int ret = __cvdso_clock_getres_common(clock, res);
> >>
> >> 	if (unlikely(ret))
> >> 		return clock_getres_fallback(clock, res);
> >> 	return 0;
> >> }
> >>
> >> When the return code of the "vdso" internal function returns an error the system
> >> call is triggered.
> > 
> > But when __cvdso_clock_getres_common() does *not* return an error, it
> > means that it handled the clock_getres() call without a fallback to the
> > syscall. I assume this is possible on arm32. When the clock_getres() is
> > handled directly (not as a syscall), why doesn't arm32 need the same
> > (res >= TASK_SIZE) check?
> 
> Ok, I see what you mean.

I'm not sure.

> It does not need to differ when __cvdso_clock_getres_common() does *not* return
> an error, we can move the checks in the fallback and leave the vdso code the
> same. The reason why I put the checks at the beginning of vdso code is because
> since I know such a condition it is going to fail I prefer to bailout
> immediately when it is detected instead of going through a bus error and a
> syscall before I can bailout.

I don't dispute your choice of choosing to bail out early, that's fine
by me. What I'm asking above, and you haven't answered, is why we don't
need exactly the same check on arm32. I.e.:

diff --git a/arch/arm/vdso/vgettimeofday.c b/arch/arm/vdso/vgettimeofday.c
index 1976c6f325a4..17ee5d211228 100644
--- a/arch/arm/vdso/vgettimeofday.c
+++ b/arch/arm/vdso/vgettimeofday.c
@@ -28,6 +28,9 @@ int __vdso_gettimeofday(struct __kernel_old_timeval *tv,
 int __vdso_clock_getres(clockid_t clock_id,
 			struct old_timespec32 *res)
 {
+	if ((u32)res >= TASK_SIZE)
+		return -EFAULT;
+
 	return __cvdso_clock_getres_time32(clock_id, res);
 }
 

(where arch/arm means arm32 ;)).

If the arm32 vdsotest passes, I'd like to know why.

> It is mainly a design choice based on what I explained above but I am open to
> suggestions if you have a better way to proceed.

I suggest just drop the TASK_SIZE_32 test altogether in this series to
get it merged for 5.7-rc1. We'll fix the ABI issues in -rc2/-rc3 once
you confirm that the test fully passes on arm32 when it doesn't fall
back to the syscall handling and we understood why.

> > Furthermore, my assumption is that __cvdso_clock_getres_common() should
> > handle this case already and we don't need it in the arch vdso code.
> > 
> 
> This is not the point I was trying to make, what I was trying to analyze here
> was the check compared to why the test verifies it, not the correctness of the
> check itself.

You should implement it based on what the man page defines, not some
specific test. Tests are rarely exhaustive (unless you do formal
modelling).

-- 
Catalin