| Message ID | 1443188504-20296-1-git-send-email-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 09/25/2015 07:41 AM, Alberto Garcia wrote: > Disabling I/O limits from a BDS also drains all pending throttled > requests, so it should be done at the beginning of bdrv_close() with > the rest of the bdrv_drain() calls before the BlockDriver is closed. Can this be abused? If I have a guest running in a cloud where the cloud provider has put severe throttling limits on me, but lets me hotplug to my heart's content, couldn't I just repeatedly plug/unplug the disk to get around the throttling (every time I unplug, all writes flush at full speed, then I immediately replug to start batching up a new set of writes). In other words, shouldn't the draining still be throttled, to prevent my abuse?
On Fri 25 Sep 2015 04:22:26 PM CEST, Eric Blake wrote: >> Disabling I/O limits from a BDS also drains all pending throttled >> requests, so it should be done at the beginning of bdrv_close() with >> the rest of the bdrv_drain() calls before the BlockDriver is closed. > > Can this be abused? If I have a guest running in a cloud where the > cloud provider has put severe throttling limits on me, but lets me > hotplug to my heart's content, couldn't I just repeatedly plug/unplug > the disk to get around the throttling (every time I unplug, all writes > flush at full speed, then I immediately replug to start batching up a > new set of writes). In other words, shouldn't the draining still be > throttled, to prevent my abuse? I didn't think about this case, and I don't know how practical this is, but note that bdrv_drain() (which is already at the beginning of bdrv_close()) flushes the I/O queue explicitly bypassing the limits, so other cases where a user can trigger a bdrv_drain() would also be vulnerable to this. Berto
On Fri, 09/25 16:31, Alberto Garcia wrote: > On Fri 25 Sep 2015 04:22:26 PM CEST, Eric Blake wrote: > > >> Disabling I/O limits from a BDS also drains all pending throttled > >> requests, so it should be done at the beginning of bdrv_close() with > >> the rest of the bdrv_drain() calls before the BlockDriver is closed. > > > > Can this be abused? If I have a guest running in a cloud where the > > cloud provider has put severe throttling limits on me, but lets me > > hotplug to my heart's content, couldn't I just repeatedly plug/unplug > > the disk to get around the throttling (every time I unplug, all writes > > flush at full speed, then I immediately replug to start batching up a > > new set of writes). In other words, shouldn't the draining still be > > throttled, to prevent my abuse? > > I didn't think about this case, and I don't know how practical this is, > but note that bdrv_drain() (which is already at the beginning of > bdrv_close()) flushes the I/O queue explicitly bypassing the limits, so > other cases where a user can trigger a bdrv_drain() would also be > vulnerable to this. > Yes, the issue is pre-existing. This patch only reordered things inside bdrv_close() so it's no worse. But indeed there is this vulnerability, maybe we should throttle the queue in all cases? Fam
On Mon 28 Sep 2015 02:18:33 AM CEST, Fam Zheng <famz@redhat.com> wrote: >> > Can this be abused? If I have a guest running in a cloud where the >> > cloud provider has put severe throttling limits on me, but lets me >> > hotplug to my heart's content, couldn't I just repeatedly >> > plug/unplug the disk to get around the throttling (every time I >> > unplug, all writes flush at full speed, then I immediately replug >> > to start batching up a new set of writes). In other words, >> > shouldn't the draining still be throttled, to prevent my abuse? >> >> I didn't think about this case, and I don't know how practical this >> is, but note that bdrv_drain() (which is already at the beginning of >> bdrv_close()) flushes the I/O queue explicitly bypassing the limits, >> so other cases where a user can trigger a bdrv_drain() would also be >> vulnerable to this. > > Yes, the issue is pre-existing. This patch only reordered things > inside bdrv_close() so it's no worse. > > But indeed there is this vulnerability, maybe we should throttle the > queue in all cases? I would like to see a test case with numbers that show how much you can actually bypass the I/O limits. Berto
Am 25.09.2015 um 15:41 hat Alberto Garcia geschrieben: > Disabling I/O limits from a BDS also drains all pending throttled > requests, so it should be done at the beginning of bdrv_close() with > the rest of the bdrv_drain() calls before the BlockDriver is closed. > > Signed-off-by: Alberto Garcia <berto@igalia.com> Thanks, applied to the block branch. Kevin
On Mon, Sep 28, 2015 at 12:04 PM, Alberto Garcia <berto@igalia.com> wrote: > On Mon 28 Sep 2015 02:18:33 AM CEST, Fam Zheng <famz@redhat.com> wrote: > >>> > Can this be abused? If I have a guest running in a cloud where the >>> > cloud provider has put severe throttling limits on me, but lets me >>> > hotplug to my heart's content, couldn't I just repeatedly >>> > plug/unplug the disk to get around the throttling (every time I >>> > unplug, all writes flush at full speed, then I immediately replug >>> > to start batching up a new set of writes). In other words, >>> > shouldn't the draining still be throttled, to prevent my abuse? >>> >>> I didn't think about this case, and I don't know how practical this >>> is, but note that bdrv_drain() (which is already at the beginning of >>> bdrv_close()) flushes the I/O queue explicitly bypassing the limits, >>> so other cases where a user can trigger a bdrv_drain() would also be >>> vulnerable to this. >> >> Yes, the issue is pre-existing. This patch only reordered things >> inside bdrv_close() so it's no worse. >> >> But indeed there is this vulnerability, maybe we should throttle the >> queue in all cases? > > I would like to see a test case with numbers that show how much you can > actually bypass the I/O limits. > > Berto > For a wild real-world case, consider a written log/db xlog. As an example, attached picture shows an actual IOPS measurement for the test sample which has been automatically throttled to 70 wIOPS. The application behind is an exim4 sending messages at a rate about 20/s. Databases also could break the QEMU IOPS write limits but on more specific conditions and I think it could be problematic to reproduce. Breaking through limit could be possible on an advertised/set qd > 1.
diff --git a/block.c b/block.c index 6268e37..1f90b47 100644 --- a/block.c +++ b/block.c @@ -1907,6 +1907,12 @@ void bdrv_close(BlockDriverState *bs) if (bs->job) { block_job_cancel_sync(bs->job); } + + /* Disable I/O limits and drain all pending throttled requests */ + if (bs->io_limits_enabled) { + bdrv_io_limits_disable(bs); + } + bdrv_drain(bs); /* complete I/O */ bdrv_flush(bs); bdrv_drain(bs); /* in case flush left pending I/O */ @@ -1958,11 +1964,6 @@ void bdrv_close(BlockDriverState *bs) blk_dev_change_media_cb(bs->blk, false); } - /*throttling disk I/O limits*/ - if (bs->io_limits_enabled) { - bdrv_io_limits_disable(bs); - } - QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) { g_free(ban); }
Disabling I/O limits from a BDS also drains all pending throttled requests, so it should be done at the beginning of bdrv_close() with the rest of the bdrv_drain() calls before the BlockDriver is closed. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)