| Message ID | 20211125181510.15004-1-paolo.valente@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "Revert "block, bfq: honor already-setup queue merges"" | expand |
> Il giorno 25 nov 2021, alle ore 19:15, Paolo Valente <paolo.valente@linaro.org> ha scritto: > > A crash [1] happened to be triggered in conjunction with commit > 2d52c58b9c9b ("block, bfq: honor already-setup queue merges"). The > latter was then reverted by commit ebc69e897e17 ("Revert "block, bfq: > honor already-setup queue merges""). Yet, the reverted commit was not > the one introducing the bug. In fact, it actually triggered a UAF > introduced by a different commit, and now fixed by commit d29bd41428cf > ("block, bfq: reset last_bfqq_created on group change"). > > So, there is no point in keeping commit 2d52c58b9c9b ("block, bfq: > honor already-setup queue merges") out. This commit restores it. > Hi, this patch does not seem to have been applied yet (or at least commented if there are still problems). Thanks, Paolo > [1] https://bugzilla.kernel.org/show_bug.cgi?id=214503 > > Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com> > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bfq-iosched.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index fec18118dc30..7cde7a11c42b 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2662,6 +2662,15 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq) > * are likely to increase the throughput. > */ > bfqq->new_bfqq = new_bfqq; > + /* > + * The above assignment schedules the following redirections: > + * each time some I/O for bfqq arrives, the process that > + * generated that I/O is disassociated from bfqq and > + * associated with new_bfqq. Here we increases new_bfqq->ref > + * in advance, adding the number of processes that are > + * expected to be associated with new_bfqq as they happen to > + * issue I/O. > + */ > new_bfqq->ref += process_refs; > return new_bfqq; > } > @@ -2724,6 +2733,10 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, > { > struct bfq_queue *in_service_bfqq, *new_bfqq; > > + /* if a merge has already been setup, then proceed with that first */ > + if (bfqq->new_bfqq) > + return bfqq->new_bfqq; > + > /* > * Check delayed stable merge for rotational or non-queueing > * devs. For this branch to be executed, bfqq must not be > @@ -2825,9 +2838,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, > if (bfq_too_late_for_merging(bfqq)) > return NULL; > > - if (bfqq->new_bfqq) > - return bfqq->new_bfqq; > - > if (!io_struct || unlikely(bfqq == &bfqd->oom_bfqq)) > return NULL; > > -- > 2.20.1 >
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index fec18118dc30..7cde7a11c42b 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2662,6 +2662,15 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq) * are likely to increase the throughput. */ bfqq->new_bfqq = new_bfqq; + /* + * The above assignment schedules the following redirections: + * each time some I/O for bfqq arrives, the process that + * generated that I/O is disassociated from bfqq and + * associated with new_bfqq. Here we increases new_bfqq->ref + * in advance, adding the number of processes that are + * expected to be associated with new_bfqq as they happen to + * issue I/O. + */ new_bfqq->ref += process_refs; return new_bfqq; } @@ -2724,6 +2733,10 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, { struct bfq_queue *in_service_bfqq, *new_bfqq; + /* if a merge has already been setup, then proceed with that first */ + if (bfqq->new_bfqq) + return bfqq->new_bfqq; + /* * Check delayed stable merge for rotational or non-queueing * devs. For this branch to be executed, bfqq must not be @@ -2825,9 +2838,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, if (bfq_too_late_for_merging(bfqq)) return NULL; - if (bfqq->new_bfqq) - return bfqq->new_bfqq; - if (!io_struct || unlikely(bfqq == &bfqd->oom_bfqq)) return NULL;
A crash [1] happened to be triggered in conjunction with commit 2d52c58b9c9b ("block, bfq: honor already-setup queue merges"). The latter was then reverted by commit ebc69e897e17 ("Revert "block, bfq: honor already-setup queue merges""). Yet, the reverted commit was not the one introducing the bug. In fact, it actually triggered a UAF introduced by a different commit, and now fixed by commit d29bd41428cf ("block, bfq: reset last_bfqq_created on group change"). So, there is no point in keeping commit 2d52c58b9c9b ("block, bfq: honor already-setup queue merges") out. This commit restores it. [1] https://bugzilla.kernel.org/show_bug.cgi?id=214503 Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bfq-iosched.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)