Page MenuHomeFreeBSD

VirtFS/9p filesystem passthrough support (virtio-9p)
Needs RevisionPublic

Authored by jceel on Apr 9 2017, 9:45 PM.

Details

Reviewers
emaste
rgrimes
jhb
Group Reviewers
bhyve
Summary

Finally I think this is good enough to start upstreaming process.

The patch contains virtio-9p bhyve piece and associated 9p server library, lib9p. lib9p is standalone and can be also used to build a TCP-based 9p server.

New bhyve(8) syntax:

-s <slot>,virtio-9p,sharename=/path/to/the/share # creates virtio-9p device called "sharename" in the guest pointing to /path/to/the/share directory.

As for now, virtio-9p doesn't work in capability mode (bhyve has to be built with WITHOUT_CAPSICUM defined).

Thanks to Chris Torek for writing large parts of the lib9p library and to Sean Eric Fagan for his contributions.

Test Plan

Add virtio-9p device to the bhyve(8) command line, as speciftied above. Boot linux guest. Type the following:

mount -t 9p -o cache=mmap -o msize=512000 testshare /mnt/9p

enjoy fast host filesystem access and overall awesomeness.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

jceel created this revision.Apr 9 2017, 9:45 PM
jceel edited the summary of this revision. (Show Details)Apr 9 2017, 9:48 PM
jceel edited the summary of this revision. (Show Details)
jceel edited the test plan for this revision. (Show Details)Apr 9 2017, 9:52 PM
jceel retitled this revision from 9p filesystem passthrough support (virtio-9p) to VirtFS/9p filesystem passthrough support (virtio-9p).Apr 9 2017, 10:13 PM
grehan edited edge metadata.Apr 9 2017, 10:17 PM

Any thoughts on the interaction with capsicum ? Seems like you just need blanket access underneath a particular directory.

jceel added a comment.Apr 9 2017, 10:21 PM

Any thoughts on the interaction with capsicum ? Seems like you just need blanket access underneath a particular directory.

Yeah, absolutely. I'm not too proficient in capsicum, but that sounds easy enough :-)

You'll have to use openat(), with the fd set up prior to cap_enter(). Here's a small example from theraven@freebsd.org, who I'm sure will be happy to provide further advice :) http://www.informit.com/articles/article.aspx?p=1924012&seqNum=2

araujo added a subscriber: araujo.Apr 10 2017, 4:52 PM

Is there another upstream repo where you'll maintain lib9p? I'm slightly surprised to see bespoke code landing in contrib/

emaste requested changes to this revision.Apr 10 2017, 5:51 PM

I've looked at the changes exclusive of the new files in contrib/lib9p, and am marking this as needs revision for the Capsicum issue discussed inline.

If/when you upload a new diff with changes for this review, can you please add full/more context to make it easier? See https://wiki.freebsd.org/Phabricator.

lib/lib9p/Makefile
11–21

sort these?

usr.sbin/bhyve/Makefile
68

This can't be committed as is (unconditionally disabling Capsicum in bhyve). For initial testing purposes you could add some sort of off-by-default conditional block that disables Capsicum and enables 9p, although it's preferable if we can address Capsicum in 3p before commit.

This revision now requires changes to proceed.Apr 10 2017, 5:51 PM

Is there another upstream repo where you'll maintain lib9p? I'm slightly surprised to see bespoke code landing in contrib/

Yes, the upstream is maintained at https://github.com/freenas/lib9p. I would like to keep it in a separate repo, as xhyve folks would also like to use it (as a git submodule).

emaste added a comment.Dec 5 2017, 8:26 PM

Yes, the upstream is maintained at https://github.com/freenas/lib9p. I would like to keep it in a separate repo, as xhyve folks would also like to use it (as a git submodule).

Ok, thanks.

Any update on a reworked patch that avoids universally forcing capsicum off for bhyve?

grehan added a subscriber: tychon.Dec 6 2017, 1:19 AM

Adding tychon@ as a subscriber: he's been doing some 9P work in recent times.

seanc added a subscriber: seanc.Jan 17 2018, 6:46 PM
emaste added a subscriber: jhb.Nov 20 2018, 5:32 PM
krion added a subscriber: krion.Dec 23 2018, 7:51 PM
jceel updated this revision to Diff 52326.Dec 27 2018, 1:33 AM

This update adds the Capsicum/Casper support to lib9p. It definitely needs more polishing, though.

dch added a subscriber: dch.Jan 25 2019, 11:20 AM
rgrimes removed a reviewer: grehan.Feb 20 2019, 5:55 PM
jhb requested changes to this revision.Jul 30 2019, 8:09 PM

This does not appear to be capsicumized. The only thing I see is that 'rootcap' is initialized but not used. Ed's earlier question about what the upstream for lib9p is has also not been answered.

usr.sbin/bhyve/Makefile
7

I don't think you need the .PATH? You aren't adding source files from this directory, only using headers, correct?

usr.sbin/bhyve/pci_virtio_9p.c
31

Perhaps s/virtio/VirtIO/? And s/passtrough/passthrough/.

49

This should be with the other sys/* headers. In general we also try to sort headers within their respective groups (though sys/param.h should be first for the kernel headers as it is now)

256

I think you don't need to strdup rootpath. The existing bytes in 'opts' will be left alone since the earlier strsep() will have skipped over it already.

260

This means you can't put 'ro' first. I think it would be more flexible to think about handling syntax errors, etc. Also, if read-only mounts aren't supported, should we instead fail with an error until such time as it is supported?

while (...)  {
    if (strcmp(opt, "ro") == 0) {
        printf("virtio-9p: read-only mounts not supported\n");
        return (-1);
    } else if (strchr('=', opt) != NULL) {
        if (sharename != NULL) {
            printf("virtio-9p: more than one share name given\n");
            return (-1);
        }
        sharename = strep(&opt, "=");
        rootpath = opt;
    } else
        printf("virtio-9p: unknown option '%s'\n", opt);
}
266

Maybe defer allocating the softc to avoid leaking the memory here (though we exit immediately, but recently folks have been adding free()'s to appease coverity)

268

This set of rights aren't used?

278

This doesn't check for overflow. For example, what if the sharename is longer than VT9P_MAXTAGSZ? I would also probably just strdup() the tag name instead of padding the calloc and using strncpy, or else use memcpy or some such if the tag is not supposed to be NULL-terminated. In addition, it might be nice to defer allocating the softc until here to avoid having to free() it in earlier error conditions to avoid leaking memory:

if (strlen(sharename) > VT9P_MAXTAGSZ) {
    printf("virtio-9p: share name '%s' too long\n", sharename);
    return (-1);
}
sc = calloc(1, sizeof(*sc));
sc->vsc_config = malloc(sizeof(struct pci_vt9p_config) + strlen(sharename));
sc->vsc_config->tag_len = strlen(sharename);
memcpy(sc->vsc_config->tag, sharenane, sc-vsc_config->tag_len);
This revision now requires changes to proceed.Jul 30 2019, 8:09 PM
markj added a subscriber: markj.Aug 1 2019, 10:35 PM
markj added inline comments.
contrib/lib9p/backend/fs.c
1414

Shouldn't this use mkfifoat()?

jhb added inline comments.Aug 2 2019, 3:50 PM
usr.sbin/bhyve/pci_virtio_9p.c
268

After talking with @markj some, I better understand what capsicumizing this meant (and your comments described), which is changing the library to use fooat(). Restricting rights on the rootfd using 'rootcap' is probably pointless since you need to allow pretty much all rights anyway, so I think you can just remove this unused variable and fix any remaining places that aren't yet using fooat() (I think Mark found one).