Re: [PATCH v3 2/7] introduce UFFD-WP low-level interface helpers

[Andrey Gruzdev]

On 19.11.2020 21:39, Peter Xu wrote:
> On Thu, Nov 19, 2020 at 03:59:35PM +0300, Andrey Gruzdev via wrote:
> > +/**
> > + * uffd_register_memory: register memory range with UFFD
> > + *
> > + * Returns 0 in case of success, negative value on error
> > + *
> > + * @uffd: UFFD file descriptor
> > + * @start: starting virtual address of memory range
> > + * @length: length of memory range
> > + * @track_missing: generate events on missing-page faults
> > + * @track_wp: generate events on write-protected-page faults
> > + */
> > +static int uffd_register_memory(int uffd, hwaddr start, hwaddr length,
> > +        bool track_missing, bool track_wp)
> > +{
> > +    struct uffdio_register uffd_register;
> > +
> > +    uffd_register.range.start = start;
> > +    uffd_register.range.len = length;
> > +    uffd_register.mode = (track_missing ? UFFDIO_REGISTER_MODE_MISSING : 0) |
> > +                         (track_wp ? UFFDIO_REGISTER_MODE_WP : 0);
> > +
> > +    if (ioctl(uffd, UFFDIO_REGISTER, &uffd_register)) {
> > +        error_report("uffd_register_memory() failed: "
> > +                "start=%0"PRIx64" len=%"PRIu64" mode=%llu errno=%i",
> > +                start, length, uffd_register.mode, errno);
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
>
> These functions look good; we should even be able to refactor the existing
> ones, e.g., ram_block_enable_notify(), but we can also do that later.  As a
> start, we can move these helpers into some common files under util/.
>
> [...]

Yep, agree.

> > +/**
> > + * ram_write_tracking_start: start UFFD-WP memory tracking
> > + *
> > + * Returns 0 for success or negative value in case of error
> > + *
> > + */
> > +int ram_write_tracking_start(void)
> > +{
>
> Need to be slightly careful on unwind on this function, because if it fails
> somehow we don't want to crash the existing running good vm... more below.
>
> > +    int uffd;
> > +    RAMState *rs = ram_state;
> > +    RAMBlock *bs;
> > +
> > +    /* Open UFFD file descriptor */
> > +    uffd = uffd_create_fd();
> > +    if (uffd < 0) {
> > +        return uffd;
> > +    }
> > +    rs->uffdio_fd = uffd;
> > +
> > +    RAMBLOCK_FOREACH_NOT_IGNORED(bs) {
> > +        /* Nothing to do with read-only and MMIO-writable regions */
> > +        if (bs->mr->readonly || bs->mr->rom_device) {
> > +            continue;
> > +        }
> > +
> > +        /* Register block memory with UFFD to track writes */
> > +        if (uffd_register_memory(rs->uffdio_fd, (hwaddr) bs->host,
> > +                bs->max_length, false, true)) {
> > +            goto fail;
> > +        }
> > +        /* Apply UFFD write protection to the block memory range */
> > +        if (uffd_protect_memory(rs->uffdio_fd, (hwaddr) bs->host,
> > +                bs->max_length, true)) {
>
> Here logically we need to undo the previous register first, however userfaultfd
> will auto-clean these when close(fd), so it's ok.  However still better to
> unwind the protection of pages, I think.  So...

It should auto-clean, but as an additional safety measure - yes.

> > +            goto fail;
> > +        }
> > +        bs->flags |= RAM_UF_WRITEPROTECT;
> > +
> > +        info_report("UFFD-WP write-tracking enabled: "
> > +                "block_id=%s page_size=%zu start=%p length=%lu "
> > +                "romd_mode=%i ram=%i readonly=%i nonvolatile=%i rom_device=%i",
> > +                bs->idstr, bs->page_size, bs->host, bs->max_length,
> > +                bs->mr->romd_mode, bs->mr->ram, bs->mr->readonly,
> > +                bs->mr->nonvolatile, bs->mr->rom_device);
> > +    }
> > +
> > +    return 0;
> > +
> > +fail:
> > +    uffd_close_fd(uffd);
>
> ... maybe do the unprotect here together, that as long as any of the above step
> failed, we need to remember to unprotect all the protected pages (or just
> unprotect everything).  And also the RAM_UF_WRITEPROTECT flags being set.

Not resetting RAM_UF_WRITEPROTECT is certainly a bug here.

But it seems that simply calling close() on UFFD file descriptor does 
all the rest cleanup for us - unprotect registered memory regions, 
remove all extra state from kernel etc. I never had a problem with 
simple close(uffd) to cleanup.. But maybe really more safe to do 
unprotect/unregister explicitly.

> > +    rs->uffdio_fd = -1;
> > +    return -1;
> > +}
> > +
> > +/**
> > + * ram_write_tracking_stop: stop UFFD-WP memory tracking and remove protection
>
> Didn't remove protections, yet?
>
> We should remove those.  For a succeeded snapshot we can avoid that (if we want
> such optimization), or imho we'd better unprotect all just in case the user
> interrupted the snapshot.

Seems that closing UFFD descriptor does unprotect for us implicitly..
Am I wrong?

> > + */
> > +void ram_write_tracking_stop(void)
> > +{
> > +    RAMState *rs = ram_state;
> > +    RAMBlock *bs;
> > +    assert(rs->uffdio_fd >= 0);
> > +
> > +    RAMBLOCK_FOREACH_NOT_IGNORED(bs) {
> > +        if ((bs->flags & RAM_UF_WRITEPROTECT) == 0) {
> > +            continue;
> > +        }
> > +        info_report("UFFD-WP write-tracking disabled: "
> > +                "block_id=%s page_size=%zu start=%p length=%lu "
> > +                "romd_mode=%i ram=%i readonly=%i nonvolatile=%i rom_device=%i",
> > +                bs->idstr, bs->page_size, bs->host, bs->max_length,
> > +                bs->mr->romd_mode, bs->mr->ram, bs->mr->readonly,
> > +                bs->mr->nonvolatile, bs->mr->rom_device);
> > +        /* Cleanup flags */
> > +        bs->flags &= ~RAM_UF_WRITEPROTECT;
> > +    }
> > +
> > +    /*
> > +     * Close UFFD file descriptor to remove protection,
> > +     * release registered memory regions and flush wait queues
> > +     */
> > +    uffd_close_fd(rs->uffdio_fd);
> > +    rs->uffdio_fd = -1;
> > +}


--
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                virtuzzo.com