Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall

[Helge Deller]

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 <address@hidden>
>>>>
>>>> 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