Re: [PATCH 08/10] parisc: fix livelock in uaccess

Helge Deller
2023-02-28T20:23:32Z

Hi Al,

On 2/28/23 21:00, Helge Deller wrote:
> On 2/28/23 20:32, Helge Deller wrote:
>> Hi Al,
>>
>> On 2/28/23 20:14, Al Viro wrote:
>>> On Tue, Feb 28, 2023 at 07:26:48PM +0100, Helge Deller wrote:
>>>
>>>> I can test both parisc32 and parisc64.
>>>>
>>>>>     Just to confirm: your "can be killed with ^C" had been on the
>>>>> mainline parisc kernel (with userfaultfd enable, of course, or it wouldn't
>>>>> hang up at all), right?
>>>>
>>>> It was a recent mainline kernel with your patch.
>>>
>>> Er...  Reproducer *is* supposed to block; the bug is that on the kernel
>>> without this patch it would (AFAICS) be impossible to kill - not even
>>> with kill -9.  With bug fixed the behaviour should be "blocks and is
>>> killable", i.e. what you've reported.
>>>
>>> What does it do on unpatched kernel?  *IF* the big is there, it would
>>> block and be unkillable by any means.  Could you verify that?
>>
>> I just tried 32- and 64-bit kernels.
>> With and without your patch on top of kernel 6.2.
>> The result is always the same:
>> The process hangs, but does not consume CPU and can be killed with ^C or kill command.
>>
>> strace output is:
>>
>> userfaultfd(0)                          = 3
>> ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
>> ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
>> fstatfs64(0, 88, {f_type=DEVPTS_SUPER_MAGIC, f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={val=[0, 0]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_NOSUID|ST_NOEXEC|ST_RELATIME}) = 0
>
> Ok, one step further...
>
> We get fooled by glibc, which replaces this line in the testcase:
>         ret = fstatfs(0, (struct statfs *)mem);
> and instead executes the 64-bit variant fstatfs64() inside a wrapper in glibc on another address.
>
> I replaced that line in the testcase to use the 32-bit syscall with the right (userfault-specified) address by
>         ret = syscall(__NR_fstatfs, 0, mem);
> and now I see:
> userfaultfd(0)                          = 3
> ioctl(3, UFFDIO_API, {api=0xaa, features=0 => features=UFFD_FEATURE_EVENT_FORK|UFFD_FEATURE_EVENT_REMAP|UFFD_FEATURE_EVENT_REMOVE|UFFD_FEATURE_MISSING_HUGETLBFS|UFFD_FEATURE_MISSING_SHMEM|UFFD_FEATURE_EVENT_UNMAP|UFFD_FEATURE_SIGBUS|UFFD_FEATURE_THREAD_ID|0x800, ioctls=1<<_UFFDIO_REGISTER|1<<_UFFDIO_UNREGISTER|1<<_UFFDIO_API}) = 0
> ioctl(3, UFFDIO_REGISTER, {range={start=0xf7afa000, len=0x1000}, mode=UFFDIO_REGISTER_MODE_MISSING, ioctls=1<<_UFFDIO_WAKE|1<<_UFFDIO_COPY|1<<_UFFDIO_ZEROPAGE}) = 0
> fstatfs(0,
> <here it hangs now and is unkillable>
>
> Needs further testing.

Now I can confirm (with the adjusted reproducer), that your patch
allows to kill the process with SIGKILL, while without your patch
it's not possibe to kill the process at all.
I've tested with a 32- and 64-bit parisc kernel.

You may add
Tested-by: Helge Deller <deller@gmx.de> # parisc
to the patch.

If you want me to take the patch (with the warning regarding missing msg variable fixed)
through the parisc tree, please let me know.

Thank you!
Helge