* [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall @ 2020-04-24 21:04 Helge Deller 2020-04-25 8:39 ` Laurent Vivier 0 siblings, 1 reply; 5+ messages in thread From: Helge Deller @ 2020-04-24 21:04 UTC (permalink / raw) To: Riku Voipio, Laurent Vivier, qemu-devel The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl flags. If the user gave any other invalid flags, the host syscall will return correct error codes, so simply drop the extra check here. Signed-off-by: Helge Deller <deller@gmx.de> diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 05f03919ff..ebf0d38321 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, int flags) sigset_t host_mask; abi_long ret; - if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) { - return -TARGET_EINVAL; - } if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) { return -TARGET_EFAULT; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall 2020-04-24 21:04 [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall Helge Deller @ 2020-04-25 8:39 ` Laurent Vivier 2020-04-25 9:24 ` Helge Deller 0 siblings, 1 reply; 5+ messages in thread From: Laurent Vivier @ 2020-04-25 8:39 UTC (permalink / raw) To: Helge Deller, Riku Voipio, qemu-devel Le 24/04/2020 à 23:04, Helge Deller a écrit : > The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl > flags. If the user gave any other invalid flags, the host syscall will > return correct error codes, so simply drop the extra check here. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 05f03919ff..ebf0d38321 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, int flags) > sigset_t host_mask; > abi_long ret; > > - if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) { > - return -TARGET_EINVAL; > - } > if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) { > return -TARGET_EFAULT; > } > Perhaps we want to trigger the TARGET_EINVAL before the TARGET_EFAULT if we have both cases? But I've checked the kernel, and the kernel does a copy_from_user() before checking the flags, but it returns EINVAL rather than EFAULT. We can remove the flags checking but we should also change TARGET_EFAULT by TARGET_EINVAL. Thanks, Laurent ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall 2020-04-25 8:39 ` Laurent Vivier @ 2020-04-25 9:24 ` Helge Deller 2020-04-25 10:03 ` Laurent Vivier 0 siblings, 1 reply; 5+ messages in thread From: Helge Deller @ 2020-04-25 9:24 UTC (permalink / raw) To: Laurent Vivier, Riku Voipio, qemu-devel On 25.04.20 10:39, Laurent Vivier wrote: > Le 24/04/2020 à 23:04, Helge Deller a écrit : >> The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl >> flags. If the user gave any other invalid flags, the host syscall will >> return correct error codes, so simply drop the extra check here. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 05f03919ff..ebf0d38321 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, int flags) >> sigset_t host_mask; >> abi_long ret; >> >> - if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) { >> - return -TARGET_EINVAL; >> - } >> if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) { >> return -TARGET_EFAULT; >> } >> > > Perhaps we want to trigger the TARGET_EINVAL before the TARGET_EFAULT if > we have both cases? > > But I've checked the kernel, and the kernel does a copy_from_user() > before checking the flags, but it returns EINVAL rather than EFAULT. That's not the full picture, since the kernel is not consistent here! In the compat-case (32bit userspace on 64bit kernel) it returns correctly EINVAL and EFAULT: if (sigsetsize != sizeof(compat_sigset_t)) return -EINVAL; if (get_compat_sigset(&mask, user_mask)) return -EFAULT; while in the non-compat case it returns EINVAL only: if (sizemask != sizeof(sigset_t) || copy_from_user(&mask, user_mask, sizeof(mask))) return -EINVAL; I think the kernel should be fixed here... > We can remove the flags checking but we should also change TARGET_EFAULT > by TARGET_EINVAL. According to the different behaviour of the kernel mentioned above you won't get it correct either way. Helge ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall 2020-04-25 9:24 ` Helge Deller @ 2020-04-25 10:03 ` Laurent Vivier 2020-04-25 21:48 ` Helge Deller 0 siblings, 1 reply; 5+ messages in thread From: Laurent Vivier @ 2020-04-25 10:03 UTC (permalink / raw) To: Helge Deller, Riku Voipio, qemu-devel Le 25/04/2020 à 11:24, Helge Deller a écrit : > On 25.04.20 10:39, Laurent Vivier wrote: >> Le 24/04/2020 à 23:04, Helge Deller a écrit : >>> The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl >>> flags. If the user gave any other invalid flags, the host syscall will >>> return correct error codes, so simply drop the extra check here. >>> >>> Signed-off-by: Helge Deller <deller@gmx.de> >>> >>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >>> index 05f03919ff..ebf0d38321 100644 >>> --- a/linux-user/syscall.c >>> +++ b/linux-user/syscall.c >>> @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, int flags) >>> sigset_t host_mask; >>> abi_long ret; >>> >>> - if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) { >>> - return -TARGET_EINVAL; >>> - } >>> if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) { >>> return -TARGET_EFAULT; >>> } >>> >> >> Perhaps we want to trigger the TARGET_EINVAL before the TARGET_EFAULT if >> we have both cases? >> >> But I've checked the kernel, and the kernel does a copy_from_user() >> before checking the flags, but it returns EINVAL rather than EFAULT. > > That's not the full picture, since the kernel is not consistent here! > In the compat-case (32bit userspace on 64bit kernel) it returns correctly > EINVAL and EFAULT: > if (sigsetsize != sizeof(compat_sigset_t)) > return -EINVAL; > if (get_compat_sigset(&mask, user_mask)) > return -EFAULT; > while in the non-compat case it returns EINVAL only: > if (sizemask != sizeof(sigset_t) || > copy_from_user(&mask, user_mask, sizeof(mask))) > return -EINVAL; > > I think the kernel should be fixed here... > >> We can remove the flags checking but we should also change TARGET_EFAULT >> by TARGET_EINVAL. > > According to the different behaviour of the kernel mentioned above > you won't get it correct either way. If we refer to manpage, EFAULT is not one of possible errors. Thanks, Laurent ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall 2020-04-25 10:03 ` Laurent Vivier @ 2020-04-25 21:48 ` Helge Deller 0 siblings, 0 replies; 5+ messages in thread From: Helge Deller @ 2020-04-25 21:48 UTC (permalink / raw) To: Laurent Vivier, Riku Voipio, qemu-devel On 25.04.20 12:03, Laurent Vivier wrote: > Le 25/04/2020 à 11:24, Helge Deller a écrit : >> On 25.04.20 10:39, Laurent Vivier wrote: >>> Le 24/04/2020 à 23:04, Helge Deller a écrit : >>>> The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl >>>> flags. If the user gave any other invalid flags, the host syscall will >>>> return correct error codes, so simply drop the extra check here. >>>> >>>> Signed-off-by: Helge Deller <deller@gmx.de> >>>> >>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >>>> index 05f03919ff..ebf0d38321 100644 >>>> --- a/linux-user/syscall.c >>>> +++ b/linux-user/syscall.c >>>> @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, int flags) >>>> sigset_t host_mask; >>>> abi_long ret; >>>> >>>> - if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) { >>>> - return -TARGET_EINVAL; >>>> - } >>>> if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) { >>>> return -TARGET_EFAULT; >>>> } >>>> >>> >>> Perhaps we want to trigger the TARGET_EINVAL before the TARGET_EFAULT if >>> we have both cases? >>> >>> But I've checked the kernel, and the kernel does a copy_from_user() >>> before checking the flags, but it returns EINVAL rather than EFAULT. >> >> That's not the full picture, since the kernel is not consistent here! >> In the compat-case (32bit userspace on 64bit kernel) it returns correctly >> EINVAL and EFAULT: >> if (sigsetsize != sizeof(compat_sigset_t)) >> return -EINVAL; >> if (get_compat_sigset(&mask, user_mask)) >> return -EFAULT; >> while in the non-compat case it returns EINVAL only: >> if (sizemask != sizeof(sigset_t) || >> copy_from_user(&mask, user_mask, sizeof(mask))) >> return -EINVAL; >> >> I think the kernel should be fixed here... >> >>> We can remove the flags checking but we should also change TARGET_EFAULT >>> by TARGET_EINVAL. >> >> According to the different behaviour of the kernel mentioned above >> you won't get it correct either way. > > If we refer to manpage, EFAULT is not one of possible errors. The manpage doesn't mention any error code for a bad mask address, and IMHO the manpage should reflect the real coding, not vice versa. I've just sent a RFC patch to the kernel mailing list. It's not a critical problem, but it would be nice to be at least consistent across the implementations. Either both should return always EINVAL, or both should return EFAULT. If it gets accepted I'll send a patch to correct the manpage accordingly afterwards, and then we can adjust qemu. Helge ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-25 21:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-24 21:04 [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall Helge Deller 2020-04-25 8:39 ` Laurent Vivier 2020-04-25 9:24 ` Helge Deller 2020-04-25 10:03 ` Laurent Vivier 2020-04-25 21:48 ` Helge Deller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.