Messages in this thread | | | Date | Thu, 18 May 2017 12:10:42 -0700 (PDT) | From | Stefano Stabellini <> | Subject | Re: [PATCH 08/18] xen/pvcalls: implement connect command |
| |
On Tue, 16 May 2017, Boris Ostrovsky wrote: > >>> + ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page); > >>> + if (ret < 0) { > >>> + sock_release(map->sock); > >>> + kfree(map); > >>> + goto out; > >>> + } > >>> + map->ring = page; > >>> + map->ring_order = map->ring->ring_order; > >>> + /* first read the order, then map the data ring */ > >>> + virt_rmb(); > >> > >> Not sure I understand what the barrier is for here. I don't think compiler > >> will reorder ring_order access with the call. > > It's to avoid using the live version of ring_order to map the data ring > > pages (the other end could be changing that value at any time). We want > > to be sure that the compiler doesn't optimize out map->ring_order and > > use map->ring->ring_order instead. > > Wouldn't WRITE_ONCE(map->ring_order, map->ring->ring_order) be the right > primitive then?
It doesn't have to be atomic, because right after the assignment we check if map->ring_order is an appropriate value (see below).
> And also: if the other side changes ring size, what are we mapping then? > It's obsolete by now.
If the grants are wrong, the mapping hypercalls will fail, the same way they do with any of the other PV frontends/backends today. That is not the problem we are trying to address with the barrier.
The issue is here is that by runtime changes to map->ring->ring_order, the frontend could issue a denial of service by getting the backend into a busyloop. You can imagine that:
for (i = 0; i < map->ring->ring_order; i++) {
might not work as the backend expects if map->ring->ring_order can change at any time.
One could say that the code is already written this way:
for (i = 0; i < map->ring_order; i++) {
So what's the problem? We have seen instances in the past of the compiler "optimizing" things in a way that actually the assembly did:
for (i = 0; i < map->ring->ring_order; i++) {
This is why I put a barrier there, to avoid such compiler "optimizations". Does it make sense?
> >>> + if (map->ring_order > MAX_RING_ORDER) { > >>> + ret = -EFAULT; > >>> + goto out; > >>> + } > >> If the barrier is indeed needed this check belongs before it. > > I don't think so, see above. > > > > > >> > >>> + ret = xenbus_map_ring_valloc(dev, map->ring->ref, > >>> + (1 << map->ring_order), &page); > >>> + if (ret < 0) { > >>> + sock_release(map->sock); > >>> + xenbus_unmap_ring_vfree(dev, map->ring); > >>> + kfree(map); > >>> + goto out; > >>> + } > >>> + map->bytes = page; > >>> >
| |