Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 13 Nov 2019 13:57:19 +0100
From:   Oleksandr Natalenko <oleksandr@natalenko.name>
To:     Paolo Valente <paolo.valente@linaro.org>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, ulf.hansson@linaro.org,
        linus.walleij@linaro.org, bfq-iosched@googlegroups.com,
        Chris Evich <cevich@redhat.com>,
        Patrick Dung <patdung100@gmail.com>,
        Thorsten Schubert <tschubert@bafh.org>
Subject: Re: [PATCH BUGFIX] block, bfq: deschedule empty bfq_queues not
 referred by any process
In-Reply-To: <20191112074856.40433-1-paolo.valente@linaro.org>
References: <20191112074856.40433-1-paolo.valente@linaro.org>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <bb393dcaa426786e0963cf0e70f0b062@natalenko.name>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi.

On 12.11.2019 08:48, 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>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
>  block/bfq-iosched.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 0319d6339822..ba68627f7740 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2713,6 +2713,27 @@ 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))
> +		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 +2804,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 +4919,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 +5021,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 +5982,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;
>  }

I'm not sure if I see things right, but this commit along with v5.3.11 
kernel causes almost all boots to hang (for instance, on mounting the 
FS). Once the scheduler is changed to something else than BFQ (I set the 
I/O scheduler early via udev rule), multiple reboots go just fine.

Is this commit also applicable to 5.3 kernels? Or I'm testing a dumb 
thing?

Thanks.

-- 
   Oleksandr Natalenko (post-factum)