linux-kernel - Re: [PATCH] bfq: fix blkio cgroup leakage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Jul 2020 15:19:36 +0300
From:   Dmitry Monakhov <dmonakhov@...il.com>
To:     Paolo Valente <paolo.valente@...aro.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, linux-block@...r.kernel.org,
        axboe@...nel.dk
Subject: Re: [PATCH] bfq: fix blkio cgroup leakage

Paolo Valente <paolo.valente@...aro.org> writes:

>> Il giorno 9 lug 2020, alle ore 10:19, Dmitry Monakhov <dmonakhov@...il.com> ha scritto:
>> 
>> Paolo Valente <paolo.valente@...aro.org> writes:
>> 
>>>> Il giorno 8 lug 2020, alle ore 19:48, Dmitry Monakhov <dmonakhov@...il.com> ha scritto:
>>>> 
>>>> Paolo Valente <paolo.valente@...aro.org> writes:
>>>> 
>>>>> Hi,
>>>>> sorry for the delay.  The commit you propose to drop fix the issues
>>>>> reported in [1].
>>>>> 
>>>>> Such a commit does introduce the leak that you report (thank you for
>>>>> spotting it).  Yet, according to the threads mentioned in [1],
>>>>> dropping that commit would take us back to those issues.
>>>>> 
>>>>> Maybe the solution is to fix the unbalance that you spotted?
>>>> I'm not quite shure that do I understand which bug was addressed for commit db37a34c563b.
>>>> AFAIU both bugs mentioned in original patchset was fixed by:
>>>> 478de3380 ("block, bfq: deschedule empty bfq_queues not referred by any proces")
>>>> f718b0932 ( block, bfq: do not plug I/O for bfq_queues with no proc refs)"
>>>> 
>>>> So I review commit db37a34c563b as independent one.
>>>> It introduces extra reference for bfq_groups via bfqg_and_blkg_get(),
>>>> but do we actually need it here?
>>>> 
>>>> #IF CONFIG_BFQ_GROUP_IOSCHED is enabled:
>>>> bfqd->root_group is holded by bfqd from bfq_init_queue()
>>>> other bfq_queue objects are owned by corresponding blkcg from bfq_pd_alloc()
>>>> So bfq_queue can not disappear under us.
>>>> 
>>> 
>>> You are right, but incomplete.  No extra ref is needed for an entity
>>> that represents a bfq_queue.  And this consideration mistook me before
>>> I realized that that commit was needed.  The problem is that an entity
>>> may also represent a group of entities.  In that case no reference is
>>> taken through any bfq_queue.  The commit you want to remove takes this
>>> missing reference.
>> Sorry, It looks like I've mistyped sentance above, I ment to say bfq_group.
>> So here is my statement corrected:
>> #IF CONFIG_BFQ_GROUP_IOSCHED is enabled:
>> bfqd->root_group is holded by bfqd from bfq_init_queue()
>> other *bfq_group* objects are owned by corresponding blkcg, reference get from bfq_pd_alloc()
>> So *bfq_group* can not disappear under us.
>> 
>> So no extra reference is required for entity represents bfq_group. Commit is not required.
>
> No, the entity may remain alive and on some tree after bfq_pd_offline has been invoked.
Ok you right, we should drop the group reference inside __bfq_bfqd_reset_in_service() 
as we do for queue's entities. Please see updated patch version.

View attachment "0001-block-bfq-fix-blkio-cgroup-leakage-v2.patch" of type "text/x-diff" (4158 bytes)