Re: Use after free with BFQ and cgroups

Re: Use after free with BFQ and cgroups

[Michal Koutný]

Hello.

On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
[...]
+Cc cgroups ML
https://lore.kernel.org/linux-block/20211125172809.GC19572-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org/


I understand there are more objects than blkcgs but I assume it can
eventually boil down to blkcg references, so I suggest another
alternative. (But I may easily miss the relations between BFQ objects,
so consider this only high-level opinion.)

> After some poking, looking into crashdumps, and applying some debug patches
> the following seems to be happening: We have a process P in blkcg G. Now
> G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
> still holds reference to G from its bfq_queue. Then P submits IO, G gets
> inserted into service tree despite being already offline.

(If G is offline, P can only be zombie, just saying. (I guess it can
still be Q's IO on behalf of G.))

IIUC, the reference to G is only held by P. If the G reference is copied
into another structure (the service tree) it should get another
reference. My naïve proposal would be css_get(). (1)

> IO completes, P exits, bfq_queue pointing to G gets destroyed, the
> last reference to G is dropped, G gets freed although is it still
> inserted in the service tree.  Eventually someone trips over the freed
> memory.

Isn't it the bfq_queue.bfq_entity that's inserted in the service tree
(not blkcg G)?
You write bfq_queue is destroyed, shouldn't that remove it from the
service tree? (2)

> Now I was looking into how to best fix this. There are several
> possibilities and I'm not sure which one to pick so that's why I'm writing
> to you. bfq_pd_offline() is walking all entities in service trees and
> trying to get rid of references to bfq_group (by reparenting entities).
> Is this guaranteed to see all entities that point to G? From the scenario
> I'm observing it seems this can miss entities pointing to G - e.g. if they
> are in idle tree, we will just remove them from the idle tree but we won't
> change entity->parent so they still point to G. This can be seen as one
> culprit of the bug.

There can be two types of references to blkcg (transitively via
bfq_group):
a) "plain" (just a pointer stored somewhere),
b) "pinned" (marked by css_get() of the respective blkcg).

The bfq_pd_offline() callback should erase all plain references (e.g. by
reparenting) or poke the holders of pinned references to release (unpin)
them eventually (so that blkcg goes away).

I reckon it's not possible to traverse all references in the
bfq_pd_offline().

> Or alternatively, should we e.g. add __bfq_deactivate_entity() to
> bfq_put_queue() when that function is dropping last queue in a bfq_group?

I guess this is what I wondered about in (2). (But I'm not sure this
really is proof against subsequent re-insertions into the tree.)

> Or should we just reparent bfq queues that have already dead parent on
> activation?

If (1) used css_tryget_online(), the parent (or ancestor if it happened
to be offlined too) could be the fallback.

> What's your opinion?

The question here is how long would stay the offlined blkcgs around if
they were directly pinned upon the IO submission. If it's unbound, then
reparenting makes more sense.


Michal

Re: Use after free with BFQ and cgroups

[Jan Kara]

On Fri 26-11-21 15:47:24, Michal Koutný wrote:
> Hello.
> 
> On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> [...]
> +Cc cgroups ML
> https://lore.kernel.org/linux-block/20211125172809.GC19572-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org/
> 
> 
> I understand there are more objects than blkcgs but I assume it can
> eventually boil down to blkcg references, so I suggest another
> alternative. (But I may easily miss the relations between BFQ objects,
> so consider this only high-level opinion.)
> 
> > After some poking, looking into crashdumps, and applying some debug patches
> > the following seems to be happening: We have a process P in blkcg G. Now
> > G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
> > still holds reference to G from its bfq_queue. Then P submits IO, G gets
> > inserted into service tree despite being already offline.
> 
> (If G is offline, P can only be zombie, just saying. (I guess it can
> still be Q's IO on behalf of G.))
> 
> IIUC, the reference to G is only held by P. If the G reference is copied
> into another structure (the service tree) it should get another
> reference. My naïve proposal would be css_get(). (1)

So I was looking into this puzzle. The answer is following:

The process P (podman, pid 2571) is currently attached to the root cgroup
but it has io_context with BFQ queue that points to the already-offline G
as a parent. The bio is thus associated with the root cgroup (via
bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
should be queued and then uses its parent to determine blkg which it should
be charged and thus gets to the dying cgroup.

Apparently P got recently moved from G to the root cgroup and there was
reference left in the BFQ queue structure to G.

> > IO completes, P exits, bfq_queue pointing to G gets destroyed, the
> > last reference to G is dropped, G gets freed although is it still
> > inserted in the service tree.  Eventually someone trips over the freed
> > memory.
> 
> Isn't it the bfq_queue.bfq_entity that's inserted in the service tree
> (not blkcg G)?

Yes, it is. But the entity is part of bfq_group structure which is the pd
for the blkcg.

> You write bfq_queue is destroyed, shouldn't that remove it from the
> service tree? (2)

Yes, BFQ queue is removed from the service trees on destruction. But its
parent - bfq_group - is not removed from its service tree. And that's where
we hit the problem.

> > Now I was looking into how to best fix this. There are several
> > possibilities and I'm not sure which one to pick so that's why I'm writing
> > to you. bfq_pd_offline() is walking all entities in service trees and
> > trying to get rid of references to bfq_group (by reparenting entities).
> > Is this guaranteed to see all entities that point to G? From the scenario
> > I'm observing it seems this can miss entities pointing to G - e.g. if they
> > are in idle tree, we will just remove them from the idle tree but we won't
> > change entity->parent so they still point to G. This can be seen as one
> > culprit of the bug.
> 
> There can be two types of references to blkcg (transitively via
> bfq_group):
> a) "plain" (just a pointer stored somewhere),
> b) "pinned" (marked by css_get() of the respective blkcg).
> 
> The bfq_pd_offline() callback should erase all plain references (e.g. by
> reparenting) or poke the holders of pinned references to release (unpin)
> them eventually (so that blkcg goes away).
> 
> I reckon it's not possible to traverse all references in the
> bfq_pd_offline().

So bfq_pd_offline() does erase all plain references AFAICT. But later it
can create new plain references (service tree) from the existing "pinned"
ones and once pinned references go away, those created plain references
cause trouble. And the more I'm looking into this the more I'm convinced
bfq_pd_offline() should be more careful and remove also the pinned
references from bfq queues. It actually does it for most queues but it can
currently miss some... I'll look into that.

Thanks for your very good questions and hints!

								Honza
-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[yukuai (C)]

在 2021/11/30 1:11, Jan Kara 写道:
> On Fri 26-11-21 15:47:24, Michal Koutný wrote:
>> Hello.
>>
>> On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack@suse.cz> wrote:
>> [...]
>> +Cc cgroups ML
>> https://lore.kernel.org/linux-block/20211125172809.GC19572@quack2.suse.cz/
>>
>>
>> I understand there are more objects than blkcgs but I assume it can
>> eventually boil down to blkcg references, so I suggest another
>> alternative. (But I may easily miss the relations between BFQ objects,
>> so consider this only high-level opinion.)
>>
>>> After some poking, looking into crashdumps, and applying some debug patches
>>> the following seems to be happening: We have a process P in blkcg G. Now
>>> G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
>>> still holds reference to G from its bfq_queue. Then P submits IO, G gets
>>> inserted into service tree despite being already offline.
>>
>> (If G is offline, P can only be zombie, just saying. (I guess it can
>> still be Q's IO on behalf of G.))
>>
>> IIUC, the reference to G is only held by P. If the G reference is copied
>> into another structure (the service tree) it should get another
>> reference. My naïve proposal would be css_get(). (1)
> 
> So I was looking into this puzzle. The answer is following:
> 
> The process P (podman, pid 2571) is currently attached to the root cgroup
> but it has io_context with BFQ queue that points to the already-offline G
> as a parent. The bio is thus associated with the root cgroup (via
> bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
> should be queued and then uses its parent to determine blkg which it should
> be charged and thus gets to the dying cgroup.

Hi, Jan

After some code review, we found that the root cause of the problem
semms to be different.

If the process is moved from group G to root group, and a new io is
issued from the process, then bfq should detect this and changing
bfq_queue's parent to root bfq_group:

bfq_insert_request
  bfq_init_rq
   bfq_bic_update_cgroup
    serial_nr = __bio_blkcg(bio)->css.serial_nr; -> from root group
    bic->blkcg_serial_nr == serial_nr -> this do not pass，because 
bic->blkcg_serial_nr is still from group G
    __bfq_bic_change_cgroup -> bfq_queue parent will be changed to root 
group

And we think the following path is possible to trigger the problem:

1) process P1 and P2 is currently in cgroup C1, corresponding to
bfq_queue q1, q2 and bfq_group g1. And q1 and q2 are merged:
q1->next_bfqq = q2.

2) move P1 from C1 to root_cgroup, q1->next_bfqq is still q2
and flag BFQQF_split_coop is not set yet.

3) P2 exit, q2 won't exit because it's still referenced through
queue merge.

4) delete C1, g1 is offlined

5) issue a new io in q1, q1's parent entity will change to root,
however the io will end up in q1->next_bfqq = q2, and thus the
offlined g1 is inserted to service tree through q2.

6) P1 exit, q2 exit, and finially g1 is freed, while g1 is still
in service tree of it's parent.

We confirmed this by our reproducer through a simple patch:
stop merging bfq_queues if their parents are different.

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 1ce1a99a7160..14c1d1c3811e 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct 
bfq_queue *new_bfqq)
         while ((__bfqq = new_bfqq->new_bfqq)) {
                 if (__bfqq == bfqq)
                         return NULL;
+               if (__bfqq->entity.parent != bfqq->entity.parent) {
+                       if (bfq_bfqq_coop(__bfqq))
+                               bfq_mark_bfqq_split_coop(__bfqq);
+                       return NULL;
+               }
                 new_bfqq = __bfqq;
         }

@@ -2825,8 +2830,16 @@ 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 (bfqq->new_bfqq) {
+               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
+
+               if(bfqq->entity.parent == new_bfqq->entity.parent)
+                       return new_bfqq;
+
+               if(bfq_bfqq_coop(new_bfqq))
+                       bfq_mark_bfqq_split_coop(new_bfqq);
+               return NULL;
+       }

Do you think this analysis is correct?

Thanks,
Kuai
> 
> Apparently P got recently moved from G to the root cgroup and there was
> reference left in the BFQ queue structure to G.
> 
>>> IO completes, P exits, bfq_queue pointing to G gets destroyed, the
>>> last reference to G is dropped, G gets freed although is it still
>>> inserted in the service tree.  Eventually someone trips over the freed
>>> memory.
>>
>> Isn't it the bfq_queue.bfq_entity that's inserted in the service tree
>> (not blkcg G)?
> 
> Yes, it is. But the entity is part of bfq_group structure which is the pd
> for the blkcg.
> 
>> You write bfq_queue is destroyed, shouldn't that remove it from the
>> service tree? (2)
> 
> Yes, BFQ queue is removed from the service trees on destruction. But its
> parent - bfq_group - is not removed from its service tree. And that's where
> we hit the problem.
> 
>>> Now I was looking into how to best fix this. There are several
>>> possibilities and I'm not sure which one to pick so that's why I'm writing
>>> to you. bfq_pd_offline() is walking all entities in service trees and
>>> trying to get rid of references to bfq_group (by reparenting entities).
>>> Is this guaranteed to see all entities that point to G? From the scenario
>>> I'm observing it seems this can miss entities pointing to G - e.g. if they
>>> are in idle tree, we will just remove them from the idle tree but we won't
>>> change entity->parent so they still point to G. This can be seen as one
>>> culprit of the bug.
>>
>> There can be two types of references to blkcg (transitively via
>> bfq_group):
>> a) "plain" (just a pointer stored somewhere),
>> b) "pinned" (marked by css_get() of the respective blkcg).
>>
>> The bfq_pd_offline() callback should erase all plain references (e.g. by
>> reparenting) or poke the holders of pinned references to release (unpin)
>> them eventually (so that blkcg goes away).
>>
>> I reckon it's not possible to traverse all references in the
>> bfq_pd_offline().
> 
> So bfq_pd_offline() does erase all plain references AFAICT. But later it
> can create new plain references (service tree) from the existing "pinned"
> ones and once pinned references go away, those created plain references
> cause trouble. And the more I'm looking into this the more I'm convinced
> bfq_pd_offline() should be more careful and remove also the pinned
> references from bfq queues. It actually does it for most queues but it can
> currently miss some... I'll look into that.
> 
> Thanks for your very good questions and hints!
> 
> 								Honza
> 

Re: Use after free with BFQ and cgroups

[Paolo Valente]

> Il giorno 9 dic 2021, alle ore 03:23, yukuai (C) <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> ha scritto:
> 
> 在 2021/11/30 1:11, Jan Kara 写道:
>> On Fri 26-11-21 15:47:24, Michal Koutný wrote:
>>> Hello.
>>> 
>>> On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>>> [...]
>>> +Cc cgroups ML
>>> https://lore.kernel.org/linux-block/20211125172809.GC19572-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org/
>>> 
>>> 
>>> I understand there are more objects than blkcgs but I assume it can
>>> eventually boil down to blkcg references, so I suggest another
>>> alternative. (But I may easily miss the relations between BFQ objects,
>>> so consider this only high-level opinion.)
>>> 
>>>> After some poking, looking into crashdumps, and applying some debug patches
>>>> the following seems to be happening: We have a process P in blkcg G. Now
>>>> G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
>>>> still holds reference to G from its bfq_queue. Then P submits IO, G gets
>>>> inserted into service tree despite being already offline.
>>> 
>>> (If G is offline, P can only be zombie, just saying. (I guess it can
>>> still be Q's IO on behalf of G.))
>>> 
>>> IIUC, the reference to G is only held by P. If the G reference is copied
>>> into another structure (the service tree) it should get another
>>> reference. My naïve proposal would be css_get(). (1)
>> So I was looking into this puzzle. The answer is following:
>> The process P (podman, pid 2571) is currently attached to the root cgroup
>> but it has io_context with BFQ queue that points to the already-offline G
>> as a parent. The bio is thus associated with the root cgroup (via
>> bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
>> should be queued and then uses its parent to determine blkg which it should
>> be charged and thus gets to the dying cgroup.
> 
> Hi, Jan
> 
> After some code review, we found that the root cause of the problem
> semms to be different.
> 
> If the process is moved from group G to root group, and a new io is
> issued from the process, then bfq should detect this and changing
> bfq_queue's parent to root bfq_group:
> 
> bfq_insert_request
> bfq_init_rq
>  bfq_bic_update_cgroup
>   serial_nr = __bio_blkcg(bio)->css.serial_nr; -> from root group
>   bic->blkcg_serial_nr == serial_nr -> this do not pass，because bic->blkcg_serial_nr is still from group G
>   __bfq_bic_change_cgroup -> bfq_queue parent will be changed to root group
> 
> And we think the following path is possible to trigger the problem:
> 
> 1) process P1 and P2 is currently in cgroup C1, corresponding to
> bfq_queue q1, q2 and bfq_group g1. And q1 and q2 are merged:
> q1->next_bfqq = q2.
> 
> 2) move P1 from C1 to root_cgroup, q1->next_bfqq is still q2
> and flag BFQQF_split_coop is not set yet.
> 
> 3) P2 exit, q2 won't exit because it's still referenced through
> queue merge.
> 
> 4) delete C1, g1 is offlined
> 
> 5) issue a new io in q1, q1's parent entity will change to root,
> however the io will end up in q1->next_bfqq = q2, and thus the
> offlined g1 is inserted to service tree through q2.
> 
> 6) P1 exit, q2 exit, and finially g1 is freed, while g1 is still
> in service tree of it's parent.
> 
> We confirmed this by our reproducer through a simple patch:
> stop merging bfq_queues if their parents are different.
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1ce1a99a7160..14c1d1c3811e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
>        while ((__bfqq = new_bfqq->new_bfqq)) {
>                if (__bfqq == bfqq)
>                        return NULL;
> +               if (__bfqq->entity.parent != bfqq->entity.parent) {
> +                       if (bfq_bfqq_coop(__bfqq))
> +                               bfq_mark_bfqq_split_coop(__bfqq);
> +                       return NULL;
> +               }
>                new_bfqq = __bfqq;
>        }
> 
> @@ -2825,8 +2830,16 @@ 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 (bfqq->new_bfqq) {
> +               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
> +
> +               if(bfqq->entity.parent == new_bfqq->entity.parent)
> +                       return new_bfqq;
> +
> +               if(bfq_bfqq_coop(new_bfqq))
> +                       bfq_mark_bfqq_split_coop(new_bfqq);
> +               return NULL;
> +       }
> 
> Do you think this analysis is correct?
> 

Hi Kuai,
I haven't checked all details, but, for sure, two bfq_queues belonging
to different parent entities (i.e., to different groups) do not have
to be merged.  So, if two bfq_queues happen to be (still) merged while
belonging to different groups, then the scheduler is in an
inconsistent state.  And rules may follow.

Let's see comments from the other people working on this issue.

Thanks for spotting this,
Paolo

> Thanks,
> Kuai
>> Apparently P got recently moved from G to the root cgroup and there was
>> reference left in the BFQ queue structure to G.
>>>> IO completes, P exits, bfq_queue pointing to G gets destroyed, the
>>>> last reference to G is dropped, G gets freed although is it still
>>>> inserted in the service tree.  Eventually someone trips over the freed
>>>> memory.
>>> 
>>> Isn't it the bfq_queue.bfq_entity that's inserted in the service tree
>>> (not blkcg G)?
>> Yes, it is. But the entity is part of bfq_group structure which is the pd
>> for the blkcg.
>>> You write bfq_queue is destroyed, shouldn't that remove it from the
>>> service tree? (2)
>> Yes, BFQ queue is removed from the service trees on destruction. But its
>> parent - bfq_group - is not removed from its service tree. And that's where
>> we hit the problem.
>>>> Now I was looking into how to best fix this. There are several
>>>> possibilities and I'm not sure which one to pick so that's why I'm writing
>>>> to you. bfq_pd_offline() is walking all entities in service trees and
>>>> trying to get rid of references to bfq_group (by reparenting entities).
>>>> Is this guaranteed to see all entities that point to G? From the scenario
>>>> I'm observing it seems this can miss entities pointing to G - e.g. if they
>>>> are in idle tree, we will just remove them from the idle tree but we won't
>>>> change entity->parent so they still point to G. This can be seen as one
>>>> culprit of the bug.
>>> 
>>> There can be two types of references to blkcg (transitively via
>>> bfq_group):
>>> a) "plain" (just a pointer stored somewhere),
>>> b) "pinned" (marked by css_get() of the respective blkcg).
>>> 
>>> The bfq_pd_offline() callback should erase all plain references (e.g. by
>>> reparenting) or poke the holders of pinned references to release (unpin)
>>> them eventually (so that blkcg goes away).
>>> 
>>> I reckon it's not possible to traverse all references in the
>>> bfq_pd_offline().
>> So bfq_pd_offline() does erase all plain references AFAICT. But later it
>> can create new plain references (service tree) from the existing "pinned"
>> ones and once pinned references go away, those created plain references
>> cause trouble. And the more I'm looking into this the more I'm convinced
>> bfq_pd_offline() should be more careful and remove also the pinned
>> references from bfq queues. It actually does it for most queues but it can
>> currently miss some... I'll look into that.
>> Thanks for your very good questions and hints!
>> 								Honza

Re: Use after free with BFQ and cgroups

[Jan Kara]

On Thu 09-12-21 10:23:33, yukuai (C) wrote:
> We confirmed this by our reproducer through a simple patch:
> stop merging bfq_queues if their parents are different.

Can you please share your reproducer? I have prepared some patches which
I'd like to verify before posting... Thanks!

								Honza
-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[yukuai (C)]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

在 2021/12/22 23:21, Jan Kara 写道:
> On Thu 09-12-21 10:23:33, yukuai (C) wrote:
>> We confirmed this by our reproducer through a simple patch:
>> stop merging bfq_queues if their parents are different.
> 
> Can you please share your reproducer? I have prepared some patches which
> I'd like to verify before posting... Thanks!

Hi,

Here is the reproducer, usually the problem will come up within an
hour.

Thanks,
Kuai
> 
> 								Honza
> 

[-- Attachment #2: null_bad.sh --]
[-- Type: text/plain, Size: 3242 bytes --]

#!/bin/bash
NR=1
basedir=/sys/fs/cgroup/blkio/null
CG_PREFIX=/sys/fs/cgroup/blkio/null/nullb

function set_cgroup()
{
	testdir=$1
	dev_id=$2
	let weight=RANDOM%900+100
	let iops=RANDOM%1000+100
	let bps=RANDOM%10485760+10485760
	echo "$weight" > $testdir/blkio.bfq.weight
	echo "$dev_id $iops" > $testdir/blkio.throttle.read_iops_device
	echo "$dev_id $iops" > $testdir/blkio.throttle.write_iops_device
	echo "$dev_id $bps" > $testdir/blkio.throttle.read_bps_device
	echo "$dev_id $bps" > $testdir/blkio.throttle.write_bps_device
}

function set_sys()
{
	local queue_dir=/sys/block/$1/queue

	let rq_affinity=RANDOM%3
	echo $rq_affinity > $queue_dir/rq_affinity

	let add_random=RANDOM%2
	echo $add_random > $queue_dir/add_random

	let rotational=RANDOM%2
	echo $rotational > $queue_dir/rotational

	let nomerges=RANDOM%2
	echo $nomerges > $queue_dir/nomerges

	let s_num=RANDOM%5
	case $s_num in
		0)
		scheduler=none
		;;
		1)
		scheduler=bfq
		;;
		2)
		scheduler=bfq
		;;
		3)
		scheduler=none
		;;
	esac
	echo bfq > $queue_dir/scheduler
}

create_cg()
{
	local i
	local path

	for i in $(seq 0 $NR)
	do
		path=${CG_PREFIX}${i}
		mkdir -p $path
	done
}

switch_cg()
{
	local path=${CG_PREFIX}$1
	local t

	for t in $(cat $path/tasks)
	do
		echo $t > /sys/fs/cgroup/blkio/tasks
	done

	echo "tasks in $path"
	cat $path/tasks
}

rm_cg()
{
	local path=${CG_PREFIX}$1

	rmdir $path
	return $?
}

mkdir $basedir
cgdir1=/sys/fs/cgroup/blkio/null/nullb0
cgdir2=/sys/fs/cgroup/blkio/null/nullb1

ADD_MOD="modprobe null_blk"
while true
do
	let flag=RANDOM%2
	if [ $flag -eq 1 ];then
		$ADD_MOD queue_mode=2 blocking=1 nr_devices=2
	else
		$ADD_MOD queue_mode=2 nr_devices=2
	fi
		
	create_cg

	dev_id=`lsblk | grep nullb0 | awk '{print $2}'`
	set_cgroup $basedir $dev_id 
	set_sys nullb0

	dev_id=`lsblk | grep nullb1 | awk '{print $2}'`
	set_cgroup $basedir $dev_id 
	set_sys nullb1

	let flag=RANDOM%20
	if [ $flag -eq 5 ];then
		echo 1 > /sys/block/nullb0/make-it-fail
		echo 1 > /sys/block/nullb1/make-it-fail
	else
		echo 0 > /sys/block/nullb0/make-it-fail
		echo 0 > /sys/block/nullb1/make-it-fail
	fi

	i=0
	while [ $i -le 3 ]
	do
		cgexec -g "blkio:null/nullb0" fio -filename=/dev/nullb0 -ioengine=libaio -time_based=1 -rw=rw -thread -size=100g -bs=512 -numjobs=4 -iodepth=8 -runtime=5 -group_reporting -name=brd-IOwrite -rwmixread=50 &>/dev/null &
		cgexec -g "blkio:null/nullb0" fio -filename=/dev/nullb0 -ioengine=psync -direct=1 -time_based=1 -rw=rw -thread -size=100g -bs=512 -numjobs=4 -iodepth=8 -runtime=5 -group_reporting -name=brd-IOwrite -rwmixread=50 &>/dev/null &
		cgexec -g "blkio:null/nullb1" fio -filename=/dev/nullb1 -ioengine=libaio -time_based=1 -rw=rw -thread -size=100g -bs=1024k -numjobs=4 -iodepth=8 -runtime=5 -group_reporting -name=brd-IOwrite -rwmixread=50 &>/dev/null &
		cgexec -g "blkio:null/nullb1" fio -filename=/dev/nullb1 -ioengine=psync -direct=1 -time_based=1 -rw=rw -thread -size=100g -bs=1024k -numjobs=4 -iodepth=8 -runtime=5 -group_reporting -name=brd-IOwrite -rwmixread=50 &>/dev/null &
		((i=i+1))
	done

	sleep 3

	until rm_cg 0
	do
		switch_cg 0
		sleep 0.1
	done

	until rm_cg 1
	do
		switch_cg 1
		sleep 0.1
	done

	while true
	do
		rmmod null_blk &>/dev/null && break
		sleep 0.1
	done
done

Re: Use after free with BFQ and cgroups

[Jan Kara]

On Thu 23-12-21 09:02:55, yukuai (C) wrote:
> 在 2021/12/22 23:21, Jan Kara 写道:
> > On Thu 09-12-21 10:23:33, yukuai (C) wrote:
> > > We confirmed this by our reproducer through a simple patch:
> > > stop merging bfq_queues if their parents are different.
> > 
> > Can you please share your reproducer? I have prepared some patches which
> > I'd like to verify before posting... Thanks!
> 
> Hi,
> 
> Here is the reproducer, usually the problem will come up within an
> hour.

Thanks! For some reason the reproducer does not trigger the KASAN warning
for me (with or without my patches). But anyway, I'll post my fixes and
we'll see what do people think, also I'd be grateful if you can test them
in your environment. Thanks.

								Honza
-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[Jan Kara]

Hello!

On Thu 09-12-21 10:23:33, yukuai (C) wrote:
> 在 2021/11/30 1:11, Jan Kara 写道:
> > On Fri 26-11-21 15:47:24, Michal Koutný wrote:
> > > Hello.
> > > 
> > > On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack@suse.cz> wrote:
> > > [...]
> > > +Cc cgroups ML
> > > https://lore.kernel.org/linux-block/20211125172809.GC19572@quack2.suse.cz/
> > > 
> > > 
> > > I understand there are more objects than blkcgs but I assume it can
> > > eventually boil down to blkcg references, so I suggest another
> > > alternative. (But I may easily miss the relations between BFQ objects,
> > > so consider this only high-level opinion.)
> > > 
> > > > After some poking, looking into crashdumps, and applying some debug patches
> > > > the following seems to be happening: We have a process P in blkcg G. Now
> > > > G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
> > > > still holds reference to G from its bfq_queue. Then P submits IO, G gets
> > > > inserted into service tree despite being already offline.
> > > 
> > > (If G is offline, P can only be zombie, just saying. (I guess it can
> > > still be Q's IO on behalf of G.))
> > > 
> > > IIUC, the reference to G is only held by P. If the G reference is copied
> > > into another structure (the service tree) it should get another
> > > reference. My naïve proposal would be css_get(). (1)
> > 
> > So I was looking into this puzzle. The answer is following:
> > 
> > The process P (podman, pid 2571) is currently attached to the root cgroup
> > but it has io_context with BFQ queue that points to the already-offline G
> > as a parent. The bio is thus associated with the root cgroup (via
> > bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
> > should be queued and then uses its parent to determine blkg which it should
> > be charged and thus gets to the dying cgroup.
> 
> After some code review, we found that the root cause of the problem
> semms to be different.
> 
> If the process is moved from group G to root group, and a new io is
> issued from the process, then bfq should detect this and changing
> bfq_queue's parent to root bfq_group:
> 
> bfq_insert_request
>  bfq_init_rq
>   bfq_bic_update_cgroup
>    serial_nr = __bio_blkcg(bio)->css.serial_nr; -> from root group
>    bic->blkcg_serial_nr == serial_nr -> this do not pass，because
> bic->blkcg_serial_nr is still from group G

So in the crashdump I have available, I can see that 
_bio_blkcg(bio)->css.serial_nr is 4. Also bic->blkcg_serial_nr is 4. But
bic->bfqq[1] is a bfq_queue that has its entity->parent set to already
offlined bfq_group. Not sure how that is possible...

>    __bfq_bic_change_cgroup -> bfq_queue parent will be changed to root group
> 
> And we think the following path is possible to trigger the problem:
> 
> 1) process P1 and P2 is currently in cgroup C1, corresponding to
> bfq_queue q1, q2 and bfq_group g1. And q1 and q2 are merged:
> q1->next_bfqq = q2.

I agree shared queues are some factor in this - the problematic bfq_queue
pointing to the dead bfq_group has 'coop' flag set, pid == -1, bic ==
NULL. So clearly it has been merged with another bfq_queue.

> 2) move P1 from C1 to root_cgroup, q1->next_bfqq is still q2
> and flag BFQQF_split_coop is not set yet.

There's no next_bfqq in the kernel I'm looking into... Generally the merge
code seems to be working somewhat differently to what you describe (I'm
looking into 5.16-rc3 kernel).

> 3) P2 exit, q2 won't exit because it's still referenced through
> queue merge.
> 
> 4) delete C1, g1 is offlined
> 
> 5) issue a new io in q1, q1's parent entity will change to root,
> however the io will end up in q1->next_bfqq = q2, and thus the
> offlined g1 is inserted to service tree through q2.
> 
> 6) P1 exit, q2 exit, and finially g1 is freed, while g1 is still
> in service tree of it's parent.
> 
> We confirmed this by our reproducer through a simple patch:
> stop merging bfq_queues if their parents are different.
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1ce1a99a7160..14c1d1c3811e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> bfq_queue *new_bfqq)
>         while ((__bfqq = new_bfqq->new_bfqq)) {
>                 if (__bfqq == bfqq)
>                         return NULL;
> +               if (__bfqq->entity.parent != bfqq->entity.parent) {
> +                       if (bfq_bfqq_coop(__bfqq))
> +                               bfq_mark_bfqq_split_coop(__bfqq);
> +                       return NULL;
> +               }
>                 new_bfqq = __bfqq;
>         }
> 
> @@ -2825,8 +2830,16 @@ 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 (bfqq->new_bfqq) {
> +               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
> +
> +               if(bfqq->entity.parent == new_bfqq->entity.parent)
> +                       return new_bfqq;
> +
> +               if(bfq_bfqq_coop(new_bfqq))
> +                       bfq_mark_bfqq_split_coop(new_bfqq);
> +               return NULL;
> +       }
> 
> Do you think this analysis is correct?

Honestly, I'm not sure. At this point I'm not sure why the bfqq pointed to
from bic didn't get reparented to the new cgroup when bio was submitted...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[yukuai (C)]

在 2021/12/14 1:33, Jan Kara 写道:
> Hello!
> 
> On Thu 09-12-21 10:23:33, yukuai (C) wrote:
>> 在 2021/11/30 1:11, Jan Kara 写道:
>>> On Fri 26-11-21 15:47:24, Michal Koutný wrote:
>>>> Hello.
>>>>
>>>> On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>>>> [...]
>>>> +Cc cgroups ML
>>>> https://lore.kernel.org/linux-block/20211125172809.GC19572-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org/
>>>>
>>>>
>>>> I understand there are more objects than blkcgs but I assume it can
>>>> eventually boil down to blkcg references, so I suggest another
>>>> alternative. (But I may easily miss the relations between BFQ objects,
>>>> so consider this only high-level opinion.)
>>>>
>>>>> After some poking, looking into crashdumps, and applying some debug patches
>>>>> the following seems to be happening: We have a process P in blkcg G. Now
>>>>> G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
>>>>> still holds reference to G from its bfq_queue. Then P submits IO, G gets
>>>>> inserted into service tree despite being already offline.
>>>>
>>>> (If G is offline, P can only be zombie, just saying. (I guess it can
>>>> still be Q's IO on behalf of G.))
>>>>
>>>> IIUC, the reference to G is only held by P. If the G reference is copied
>>>> into another structure (the service tree) it should get another
>>>> reference. My naïve proposal would be css_get(). (1)
>>>
>>> So I was looking into this puzzle. The answer is following:
>>>
>>> The process P (podman, pid 2571) is currently attached to the root cgroup
>>> but it has io_context with BFQ queue that points to the already-offline G
>>> as a parent. The bio is thus associated with the root cgroup (via
>>> bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
>>> should be queued and then uses its parent to determine blkg which it should
>>> be charged and thus gets to the dying cgroup.
>>
>> After some code review, we found that the root cause of the problem
>> semms to be different.
>>
>> If the process is moved from group G to root group, and a new io is
>> issued from the process, then bfq should detect this and changing
>> bfq_queue's parent to root bfq_group:
>>
>> bfq_insert_request
>>   bfq_init_rq
>>    bfq_bic_update_cgroup
>>     serial_nr = __bio_blkcg(bio)->css.serial_nr; -> from root group
>>     bic->blkcg_serial_nr == serial_nr -> this do not pass，because
>> bic->blkcg_serial_nr is still from group G
> 
> So in the crashdump I have available, I can see that
> _bio_blkcg(bio)->css.serial_nr is 4. Also bic->blkcg_serial_nr is 4. But
> bic->bfqq[1] is a bfq_queue that has its entity->parent set to already
> offlined bfq_group. Not sure how that is possible...
> 
>>     __bfq_bic_change_cgroup -> bfq_queue parent will be changed to root group
>>
>> And we think the following path is possible to trigger the problem:
>>
>> 1) process P1 and P2 is currently in cgroup C1, corresponding to
>> bfq_queue q1, q2 and bfq_group g1. And q1 and q2 are merged:
>> q1->next_bfqq = q2.
> 
> I agree shared queues are some factor in this - the problematic bfq_queue
> pointing to the dead bfq_group has 'coop' flag set, pid == -1, bic ==
> NULL. So clearly it has been merged with another bfq_queue.
> 
>> 2) move P1 from C1 to root_cgroup, q1->next_bfqq is still q2
>> and flag BFQQF_split_coop is not set yet.
> 
> There's no next_bfqq in the kernel I'm looking into... Generally the merge
> code seems to be working somewhat differently to what you describe (I'm
> looking into 5.16-rc3 kernel).

Hi, Jan

Sorry, It should be new_bfqq, which will be set if the queue is merged
to other queue.
> 
>> 3) P2 exit, q2 won't exit because it's still referenced through
>> queue merge.
>>
>> 4) delete C1, g1 is offlined
>>
>> 5) issue a new io in q1, q1's parent entity will change to root,
>> however the io will end up in q1->next_bfqq = q2, and thus the
>> offlined g1 is inserted to service tree through q2.
>>
>> 6) P1 exit, q2 exit, and finially g1 is freed, while g1 is still
>> in service tree of it's parent.
>>
>> We confirmed this by our reproducer through a simple patch:
>> stop merging bfq_queues if their parents are different.
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 1ce1a99a7160..14c1d1c3811e 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
>> bfq_queue *new_bfqq)
>>          while ((__bfqq = new_bfqq->new_bfqq)) {
>>                  if (__bfqq == bfqq)
>>                          return NULL;
>> +               if (__bfqq->entity.parent != bfqq->entity.parent) {
>> +                       if (bfq_bfqq_coop(__bfqq))
>> +                               bfq_mark_bfqq_split_coop(__bfqq);
>> +                       return NULL;
>> +               }
>>                  new_bfqq = __bfqq;
>>          }
>>
>> @@ -2825,8 +2830,16 @@ 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 (bfqq->new_bfqq) {
>> +               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
>> +
>> +               if(bfqq->entity.parent == new_bfqq->entity.parent)
>> +                       return new_bfqq;
>> +
>> +               if(bfq_bfqq_coop(new_bfqq))
>> +                       bfq_mark_bfqq_split_coop(new_bfqq);
>> +               return NULL;
>> +       }
>>
>> Do you think this analysis is correct?
> 
> Honestly, I'm not sure. At this point I'm not sure why the bfqq pointed to
> from bic didn't get reparented to the new cgroup when bio was submitted...

After queue merging, bic is set to the new bfqq, for example:

__bfq_insert_request
  new_bfqq = bfq_setup_cooperator
  bfq_merge_bfqqs -> before this is done, bic is set to old bfqq
   bic_set_bfqq(bic, new_bfqq, 1); -> bic is set to new bfqq
  rq->elv.priv[1] = new_bfqq;

I think current problem is that if bfq_queue is merged, task migration
won't break such cooperation，thus issue io from one cgroup may endup to
a bfq_queue that is from another cgroup, which might be offlined.

Thanks,
Kuai
> 
> 								Honza
> 

Re: Use after free with BFQ and cgroups

[Jan Kara]

On Tue 14-12-21 09:24:58, yukuai (C) wrote:
> 在 2021/12/14 1:33, Jan Kara 写道:
> > Hello!
> > 
> > On Thu 09-12-21 10:23:33, yukuai (C) wrote:
> > > 在 2021/11/30 1:11, Jan Kara 写道:
> > > > On Fri 26-11-21 15:47:24, Michal Koutný wrote:
> > > > > Hello.
> > > > > 
> > > > > On Thu, Nov 25, 2021 at 06:28:09PM +0100, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > > > > [...]
> > > > > +Cc cgroups ML
> > > > > https://lore.kernel.org/linux-block/20211125172809.GC19572-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org/
> > > > > 
> > > > > 
> > > > > I understand there are more objects than blkcgs but I assume it can
> > > > > eventually boil down to blkcg references, so I suggest another
> > > > > alternative. (But I may easily miss the relations between BFQ objects,
> > > > > so consider this only high-level opinion.)
> > > > > 
> > > > > > After some poking, looking into crashdumps, and applying some debug patches
> > > > > > the following seems to be happening: We have a process P in blkcg G. Now
> > > > > > G is taken offline so bfq_group is cleaned up in bfq_pd_offline() but P
> > > > > > still holds reference to G from its bfq_queue. Then P submits IO, G gets
> > > > > > inserted into service tree despite being already offline.
> > > > > 
> > > > > (If G is offline, P can only be zombie, just saying. (I guess it can
> > > > > still be Q's IO on behalf of G.))
> > > > > 
> > > > > IIUC, the reference to G is only held by P. If the G reference is copied
> > > > > into another structure (the service tree) it should get another
> > > > > reference. My naïve proposal would be css_get(). (1)
> > > > 
> > > > So I was looking into this puzzle. The answer is following:
> > > > 
> > > > The process P (podman, pid 2571) is currently attached to the root cgroup
> > > > but it has io_context with BFQ queue that points to the already-offline G
> > > > as a parent. The bio is thus associated with the root cgroup (via
> > > > bio->bi_blkg) but BFQ uses io_context to lookup the BFQ queue where IO
> > > > should be queued and then uses its parent to determine blkg which it should
> > > > be charged and thus gets to the dying cgroup.
> > > 
> > > After some code review, we found that the root cause of the problem
> > > semms to be different.
> > > 
> > > If the process is moved from group G to root group, and a new io is
> > > issued from the process, then bfq should detect this and changing
> > > bfq_queue's parent to root bfq_group:
> > > 
> > > bfq_insert_request
> > >   bfq_init_rq
> > >    bfq_bic_update_cgroup
> > >     serial_nr = __bio_blkcg(bio)->css.serial_nr; -> from root group
> > >     bic->blkcg_serial_nr == serial_nr -> this do not pass，because
> > > bic->blkcg_serial_nr is still from group G
> > 
> > So in the crashdump I have available, I can see that
> > _bio_blkcg(bio)->css.serial_nr is 4. Also bic->blkcg_serial_nr is 4. But
> > bic->bfqq[1] is a bfq_queue that has its entity->parent set to already
> > offlined bfq_group. Not sure how that is possible...
> > 
> > >     __bfq_bic_change_cgroup -> bfq_queue parent will be changed to root group
> > > 
> > > And we think the following path is possible to trigger the problem:
> > > 
> > > 1) process P1 and P2 is currently in cgroup C1, corresponding to
> > > bfq_queue q1, q2 and bfq_group g1. And q1 and q2 are merged:
> > > q1->next_bfqq = q2.
> > 
> > I agree shared queues are some factor in this - the problematic bfq_queue
> > pointing to the dead bfq_group has 'coop' flag set, pid == -1, bic ==
> > NULL. So clearly it has been merged with another bfq_queue.
> > 
> > > 2) move P1 from C1 to root_cgroup, q1->next_bfqq is still q2
> > > and flag BFQQF_split_coop is not set yet.
> > 
> > There's no next_bfqq in the kernel I'm looking into... Generally the merge
> > code seems to be working somewhat differently to what you describe (I'm
> > looking into 5.16-rc3 kernel).
> 
> Hi, Jan
> 
> Sorry, It should be new_bfqq, which will be set if the queue is merged
> to other queue.
> > 
> > > 3) P2 exit, q2 won't exit because it's still referenced through
> > > queue merge.
> > > 
> > > 4) delete C1, g1 is offlined
> > > 
> > > 5) issue a new io in q1, q1's parent entity will change to root,
> > > however the io will end up in q1->next_bfqq = q2, and thus the
> > > offlined g1 is inserted to service tree through q2.
> > > 
> > > 6) P1 exit, q2 exit, and finially g1 is freed, while g1 is still
> > > in service tree of it's parent.
> > > 
> > > We confirmed this by our reproducer through a simple patch:
> > > stop merging bfq_queues if their parents are different.
> > > 
> > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > > index 1ce1a99a7160..14c1d1c3811e 100644
> > > --- a/block/bfq-iosched.c
> > > +++ b/block/bfq-iosched.c
> > > @@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> > > bfq_queue *new_bfqq)
> > >          while ((__bfqq = new_bfqq->new_bfqq)) {
> > >                  if (__bfqq == bfqq)
> > >                          return NULL;
> > > +               if (__bfqq->entity.parent != bfqq->entity.parent) {
> > > +                       if (bfq_bfqq_coop(__bfqq))
> > > +                               bfq_mark_bfqq_split_coop(__bfqq);
> > > +                       return NULL;
> > > +               }
> > >                  new_bfqq = __bfqq;
> > >          }
> > > 
> > > @@ -2825,8 +2830,16 @@ 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 (bfqq->new_bfqq) {
> > > +               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
> > > +
> > > +               if(bfqq->entity.parent == new_bfqq->entity.parent)
> > > +                       return new_bfqq;
> > > +
> > > +               if(bfq_bfqq_coop(new_bfqq))
> > > +                       bfq_mark_bfqq_split_coop(new_bfqq);
> > > +               return NULL;
> > > +       }
> > > 
> > > Do you think this analysis is correct?
> > 
> > Honestly, I'm not sure. At this point I'm not sure why the bfqq pointed to
> > from bic didn't get reparented to the new cgroup when bio was submitted...
> 
> After queue merging, bic is set to the new bfqq, for example:
> 
> __bfq_insert_request
>  new_bfqq = bfq_setup_cooperator
>  bfq_merge_bfqqs -> before this is done, bic is set to old bfqq
>   bic_set_bfqq(bic, new_bfqq, 1); -> bic is set to new bfqq
>  rq->elv.priv[1] = new_bfqq;
> 
> I think current problem is that if bfq_queue is merged, task migration
> won't break such cooperation，thus issue io from one cgroup may endup to
> a bfq_queue that is from another cgroup, which might be offlined.

OK, I now understand what you mean. Indeed the full code is like:

bfq_insert_request()
  ...
  bfq_init_rq()
    bfq_check_ioprio_change(bic, bio);
    bfq_bic_update_cgroup(bic, bio);
      - this correctly moves bfqq of the process to appropriate blkcg
    bfqq = bfq_get_bfqq_handle_split()
    ... now associate request with bfqq ...
  __bfq_insert_request()
    new_bfqq = bfq_setup_cooperator()
    ... move request to new_bfqq ... there is no guarantee about the
    parent of new_bfqq, it may be a different cgroup if bic->stable_merge_bfqq
    predates process move, it may point to a dead cgroup or whatever.

Now I think there are several things wrong about the current code:

1) A process can submit IO for multiple cgroups (if e.g. fsync() is doing
writeback of pages created in the page cache by a process from different
cgroup). Thus bfqq can at the same time contain IO from multiple cgroups
and reparenting of bfqq in bfq_bic_update_cgroup() is going to have
interesting side-effects on the scheduling decisions. This is going to be
difficult to fully solve - probably bfqq in bic should have as a parent
cgroup pointed to by css of the process and if we detect currently
submitted bio belongs to a different cgroup, we need to lookup & create new
bfqq with appropriate parent and queue the request there. But this is not
critical issue at this point so we can leave it for later, I just wanted to
mention this to set frame for the problems below.

2) Bfqq merging (bfq_setup_cooperator() and friends) happens in
__bfq_insert_request() rather late during request queueing. As a result we
have to redo several things when we decide to merge queues (as request is
already queued in the previous queue) and we miss to update cgroup parent.
IMHO we should reorganize the code flow so that all the queue merging &
splitting happens first, we get final bfqq out of that and we associate
request with that bfqq (including cgroup handling). This will also fix the
use-after-free issue.

3) If the process is moved to a different cgroup (or just submits bio for a
different cgroup) The bic->stable_merge_bfq may point to bfqq from a
different cgroup and thus merging them is undesirable. Solution for 1) will
also deal with this problem but until then, I think that if
bfq_bic_update_cgroup() notices a cgroup mismatch, it also clears
bic->stable_merge_bfq to avoid this problem. As a side-effect this will
also fix the use-after-free issue even without solving 2).

4) I find it fragile that we may have around bfqqs with dead cgroups as a
parent and if we mistakenly activate these bfqqs (and thus parent cgroups)
and insert them in the service tree, use-after-free problem is here. So I
think that my patch to actively reparent all bfqqs from a dying cgroup
makes sense (alternatively we could make sure service tree holds a ref for
the cgroups it contains but that seems like a more expensive and intrusive
fix) and I'd keep it as a safety measure for the future.

So my current plan is:

Implement 3) as a quick and easily backportable fix for the use-after-free
issue. Then work on 2) and submit it with 4) as cleanups & future-proofing.
Once this is done, we can think about 1) but probably we should try to
see how bad that problem is in practice and whether the solution is worth
it.

What do people think?

								Honza

-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[Tejun Heo]

On Fri, Nov 26, 2021 at 03:47:24PM +0100, Michal Koutný wrote:
> The question here is how long would stay the offlined blkcgs around if
> they were directly pinned upon the IO submission. If it's unbound, then
> reparenting makes more sense.

It should be fine to pin whatever's necessary while related IOs are in
flight and percpu_ref used for css refcnting isn't gonna make any noticeable
difference in terms of overhead.

Thanks.

-- 
tejun

Re: Use after free with BFQ and cgroups

[Jan Kara]

On Mon 29-11-21 07:12:42, Tejun Heo wrote:
> On Fri, Nov 26, 2021 at 03:47:24PM +0100, Michal Koutný wrote:
> > The question here is how long would stay the offlined blkcgs around if
> > they were directly pinned upon the IO submission. If it's unbound, then
> > reparenting makes more sense.
> 
> It should be fine to pin whatever's necessary while related IOs are in
> flight and percpu_ref used for css refcnting isn't gonna make any noticeable
> difference in terms of overhead.

Yes, holding cgroup ref from IO would be fine. But that is not really our
problem.

The problem is bfq_queue associated with a task effectively holds a
reference to the potentially dead cgroup and the reference can stay there
until the task (that itself got reparented to the root cgroup) exits. So I
think we need to reparent these bfq_queue structures as well to avoid
holding cgroup in zombie state excessively long.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Use after free with BFQ and cgroups

[Tejun Heo]

Hello,

On Tue, Nov 30, 2021 at 12:50:10PM +0100, Jan Kara wrote:
> The problem is bfq_queue associated with a task effectively holds a
> reference to the potentially dead cgroup and the reference can stay there
> until the task (that itself got reparented to the root cgroup) exits. So I
> think we need to reparent these bfq_queue structures as well to avoid
> holding cgroup in zombie state excessively long.

Ah, I see. Yeah, that's not great. Agree that it'd be better to reparent
(probably just punt to the root cgroup).

Thanks.

-- 
tejun