[PATCH BUGFIX V2 0/1] block, bfq: deschedule empty bfq_queues not referred by any process

[PATCH BUGFIX V2 0/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Paolo Valente]

Hi Jens,
change from V1: added check to correctly work only on bfq-queues
scheduled for service, and not on in-service bfq-queues (it makes no
sense, and it creates inconsistencies, to deschedule an in-service
bfq-queue).

Differently from V1, which was still under test when I submitted it,
this version has already been tested, by those who reported V1's
failures.

Thanks,
Paolo

Paolo Valente (1):
  block, bfq: deschedule empty bfq_queues not referred by any process

 block/bfq-iosched.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

--
2.20.1

[PATCH BUGFIX V2 1/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Paolo Valente]

Since commit 3726112ec731 ("block, bfq: re-schedule empty queues if
they deserve I/O plugging"), to prevent the service guarantees of a
bfq_queue from being violated, the bfq_queue may be left busy, i.e.,
scheduled for service, even if empty (see comments in
__bfq_bfqq_expire() for details). But, if no process will send
requests to the bfq_queue any longer, then there is no point in
keeping the bfq_queue scheduled for service.

In addition, keeping the bfq_queue scheduled for service, but with no
process reference any longer, may cause the bfq_queue to be freed when
descheduled from service. But this is assumed to never happen, and
causes a UAF if it happens. This, in turn, caused crashes [1, 2].

This commit fixes this issue by descheduling an empty bfq_queue when
it remains with not process reference.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1767539
[2] https://bugzilla.kernel.org/show_bug.cgi?id=205447

Fixes: 3726112ec731 ("block, bfq: re-schedule empty queues if they deserve I/O plugging")
Reported-by: Chris Evich <cevich@redhat.com>
Reported-by: Patrick Dung <patdung100@gmail.com>
Reported-by: Thorsten Schubert <tschubert@bafh.org>
Tested-by: Thorsten Schubert <tschubert@bafh.org>
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 0319d6339822..0c6214497fcc 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2713,6 +2713,28 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 	}
 }
 
+
+static
+void bfq_release_process_ref(struct bfq_data *bfqd, struct bfq_queue *bfqq)
+{
+	/*
+	 * To prevent bfqq's service guarantees from being violated,
+	 * bfqq may be left busy, i.e., queued for service, even if
+	 * empty (see comments in __bfq_bfqq_expire() for
+	 * details). But, if no process will send requests to bfqq any
+	 * longer, then there is no point in keeping bfqq queued for
+	 * service. In addition, keeping bfqq queued for service, but
+	 * with no process ref any longer, may have caused bfqq to be
+	 * freed when dequeued from service. But this is assumed to
+	 * never happen.
+	 */
+	if (bfq_bfqq_busy(bfqq) && RB_EMPTY_ROOT(&bfqq->sort_list) &&
+	    bfqq != bfqd->in_service_queue)
+		bfq_del_bfqq_busy(bfqd, bfqq, false);
+
+	bfq_put_queue(bfqq);
+}
+
 static void
 bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic,
 		struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
@@ -2783,8 +2805,7 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic,
 	 */
 	new_bfqq->pid = -1;
 	bfqq->bic = NULL;
-	/* release process reference to bfqq */
-	bfq_put_queue(bfqq);
+	bfq_release_process_ref(bfqd, bfqq);
 }
 
 static bool bfq_allow_bio_merge(struct request_queue *q, struct request *rq,
@@ -4899,7 +4920,7 @@ static void bfq_exit_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq)
 
 	bfq_put_cooperator(bfqq);
 
-	bfq_put_queue(bfqq); /* release process reference */
+	bfq_release_process_ref(bfqd, bfqq);
 }
 
 static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync)
@@ -5001,8 +5022,7 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio)
 
 	bfqq = bic_to_bfqq(bic, false);
 	if (bfqq) {
-		/* release process reference on this queue */
-		bfq_put_queue(bfqq);
+		bfq_release_process_ref(bfqd, bfqq);
 		bfqq = bfq_get_queue(bfqd, bio, BLK_RW_ASYNC, bic);
 		bic_set_bfqq(bic, bfqq, false);
 	}
@@ -5963,7 +5983,7 @@ bfq_split_bfqq(struct bfq_io_cq *bic, struct bfq_queue *bfqq)
 
 	bfq_put_cooperator(bfqq);
 
-	bfq_put_queue(bfqq);
+	bfq_release_process_ref(bfqq->bfqd, bfqq);
 	return NULL;
 }
 
-- 
2.20.1

Re: [PATCH BUGFIX V2 0/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Jens Axboe]

On 11/14/19 2:33 AM, Paolo Valente wrote:
> Hi Jens,
> change from V1: added check to correctly work only on bfq-queues
> scheduled for service, and not on in-service bfq-queues (it makes no
> sense, and it creates inconsistencies, to deschedule an in-service
> bfq-queue).
> 
> Differently from V1, which was still under test when I submitted it,
> this version has already been tested, by those who reported V1's
> failures.

I'm a bit miffed that you'd send out a patch for an issue, this late
in the cycle, and then it not being tested at all. That's not very
confidence inspiring. I have applied this one, just letting you know
that that is not acceptable at all.

-- 
Jens Axboe

Re: [PATCH BUGFIX V2 0/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Paolo Valente]

> Il giorno 14 nov 2019, alle ore 15:02, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 11/14/19 2:33 AM, Paolo Valente wrote:
>> Hi Jens,
>> change from V1: added check to correctly work only on bfq-queues
>> scheduled for service, and not on in-service bfq-queues (it makes no
>> sense, and it creates inconsistencies, to deschedule an in-service
>> bfq-queue).
>> 
>> Differently from V1, which was still under test when I submitted it,
>> this version has already been tested, by those who reported V1's
>> failures.
> 
> I'm a bit miffed that you'd send out a patch for an issue, this late
> in the cycle, and then it not being tested at all. That's not very
> confidence inspiring. I have applied this one, just letting you know
> that that is not acceptable at all.
> 

I'm sorry for irritating you.  Yet I don't fully get your point.  I
have sent this fix now, simply because this bug was found ten days
ago, and I've tried to fix it as soon as possible.  I did test my
patch before sending it.  As for public testing, how could Oleksandr
or any other user/dev have had a chance to test this patch if I had
not submitted it here?

Thanks,
Paolo

> -- 
> Jens Axboe
> 

Re: [PATCH BUGFIX V2 0/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Jens Axboe]

On 11/14/19 8:14 AM, Paolo Valente wrote:
> 
> 
>> Il giorno 14 nov 2019, alle ore 15:02, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 11/14/19 2:33 AM, Paolo Valente wrote:
>>> Hi Jens,
>>> change from V1: added check to correctly work only on bfq-queues
>>> scheduled for service, and not on in-service bfq-queues (it makes no
>>> sense, and it creates inconsistencies, to deschedule an in-service
>>> bfq-queue).
>>>
>>> Differently from V1, which was still under test when I submitted it,
>>> this version has already been tested, by those who reported V1's
>>> failures.
>>
>> I'm a bit miffed that you'd send out a patch for an issue, this late
>> in the cycle, and then it not being tested at all. That's not very
>> confidence inspiring. I have applied this one, just letting you know
>> that that is not acceptable at all.
>>
> 
> I'm sorry for irritating you.  Yet I don't fully get your point.  I
> have sent this fix now, simply because this bug was found ten days
> ago, and I've tried to fix it as soon as possible.  I did test my
> patch before sending it.  As for public testing, how could Oleksandr
> or any other user/dev have had a chance to test this patch if I had
> not submitted it here?

If that's the case, then make it clear that you don't expect it to
be merged right now. As it stands, when you sent it out, all I know is
that it's an issue that's crashing current kernels, and we're winding
down this release. Hence there's a sense of urgency there, as we
could be releasing this kernel as soon as this weekend.

If you have a potential fix, but it isn't tested yet, then make that
clear by submitting it as an RFC. You'd say something like:

"This is a potential fix for X/Y/Z, let's wait for the original reporters
to verify this before including it."

And make that clear with RFC in the subject line. Your patch had none
of that, in fact it said:

[PATCH BUGFIX]

and the commit message had no references to this needing any further
testing.

-- 
Jens Axboe

Re: [PATCH BUGFIX V2 1/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Holger Hoffstätte]

On 11/14/19 10:33 AM, Paolo Valente wrote:
> Since commit 3726112ec731 ("block, bfq: re-schedule empty queues if
> they deserve I/O plugging"), to prevent the service guarantees of a
> bfq_queue from being violated, the bfq_queue may be left busy, i.e.,
> scheduled for service, even if empty (see comments in
> __bfq_bfqq_expire() for details). But, if no process will send
> requests to the bfq_queue any longer, then there is no point in
> keeping the bfq_queue scheduled for service.
> 
> In addition, keeping the bfq_queue scheduled for service, but with no
> process reference any longer, may cause the bfq_queue to be freed when
> descheduled from service. But this is assumed to never happen, and
> causes a UAF if it happens. This, in turn, caused crashes [1, 2].
> 
> This commit fixes this issue by descheduling an empty bfq_queue when
> it remains with not process reference.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1767539
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=205447
> 
> Fixes: 3726112ec731 ("block, bfq: re-schedule empty queues if they deserve I/O plugging")
> Reported-by: Chris Evich <cevich@redhat.com>
> Reported-by: Patrick Dung <patdung100@gmail.com>
> Reported-by: Thorsten Schubert <tschubert@bafh.org>
> Tested-by: Thorsten Schubert <tschubert@bafh.org>
> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>

Jens,

can you please also tag this for stable-5.3 before the next push?
The original problem was found on 5.3 after all, and hoping for the
stable-bot to pick it up automagically is a bit unreliable.

Thanks,
Holger

Re: [PATCH BUGFIX V2 1/1] block, bfq: deschedule empty bfq_queues not referred by any process

[Jens Axboe]

On 11/15/19 11:32 AM, Holger Hoffstätte wrote:
> On 11/14/19 10:33 AM, Paolo Valente wrote:
>> Since commit 3726112ec731 ("block, bfq: re-schedule empty queues if
>> they deserve I/O plugging"), to prevent the service guarantees of a
>> bfq_queue from being violated, the bfq_queue may be left busy, i.e.,
>> scheduled for service, even if empty (see comments in
>> __bfq_bfqq_expire() for details). But, if no process will send
>> requests to the bfq_queue any longer, then there is no point in
>> keeping the bfq_queue scheduled for service.
>>
>> In addition, keeping the bfq_queue scheduled for service, but with no
>> process reference any longer, may cause the bfq_queue to be freed when
>> descheduled from service. But this is assumed to never happen, and
>> causes a UAF if it happens. This, in turn, caused crashes [1, 2].
>>
>> This commit fixes this issue by descheduling an empty bfq_queue when
>> it remains with not process reference.
>>
>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1767539
>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=205447
>>
>> Fixes: 3726112ec731 ("block, bfq: re-schedule empty queues if they deserve I/O plugging")
>> Reported-by: Chris Evich <cevich@redhat.com>
>> Reported-by: Patrick Dung <patdung100@gmail.com>
>> Reported-by: Thorsten Schubert <tschubert@bafh.org>
>> Tested-by: Thorsten Schubert <tschubert@bafh.org>
>> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> 
> Jens,
> 
> can you please also tag this for stable-5.3 before the next push?
> The original problem was found on 5.3 after all, and hoping for the
> stable-bot to pick it up automagically is a bit unreliable.

Too late for that, but we can point stable@ at it once it's merged.

-- 
Jens Axboe