Re: zram corruption due to uninitialized do_swap_page fault

[Ivan Babrou]

On Tue, Mar 15, 2022 at 3:09 PM Minchan Kim <minchan@xxxxxxxxxx> wrote:
> I think the problem with CLONE_VM is following race
>
> CPU A                        CPU B
>
> do_swap_page                do_swap_page
> SWP_SYNCHRONOUS_IO path     SWP_SYNCHRONOUS_IO path
> swap_readpage original data
>   swap_slot_free_notify
>     delete zram entry
>                             swap_readpage zero data
>                             pte_lock
>                             map the *zero data* to userspace
>                             pte_unlock
> pte_lock
> if (!pte_same)
>   goto out_nomap;
> pte_unlock
> return and next refault will
> read zero data
>
> So, CPU A and B see zero data. With patchset below, it changes
>
>
> CPU A                        CPU B
>
> do_swap_page                do_swap_page
> SWP_SYNCHRONOUS_IO path     SWP_SYNCHRONOUS_IO path
>                             swap_readpage original data
>                             pte_lock
>                             map the original data
>                             swap_free
>                               swap_range_free
>                                 bd_disk->fops->swap_slot_free_notify
> swap_readpage read zero data
>                             pte_unlock
> pte_lock
> if (!pte_same)
>   goto out_nomap;
> pte_unlock
> return and next refault will
> read correct data again
>
> Here, CPU A could read zero data from zram but that's not a bug
> (IOW, warning injected doesn't mean bug).
>
> The concern of the patch would increase memory size since it could
> increase wasted memory with compressed form in zram and uncompressed
> form in address space.  However, most of cases of zram uses no
> readahead and then, do_swap_page is followed by swap_free so it will
> free the compressed from in zram quickly.
>
> Ivan, with this patch, you can see the warning you added in the zram
> but it shouldn't trigger the userspace corruption as mentioned above
> if I understand correctly.
>
> Could you test whether the patch prevent userspace broken?

I'm making an internal build and will push it to some location to see
how it behaves, but it might take a few days to get any sort of
confidence in the results (unless it breaks immediately).

I've also pushed my patch that disables SWP_SYNCHRONOUS_IO to a few
locations yesterday to see how it fares.