| Message ID | 20221211222058.2946830-3-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | blk-cgroup: Fix potential UAF & miscellaneous cleanup | expand |
Hello. On Sun, Dec 11, 2022 at 05:20:57PM -0500, Waiman Long <longman@redhat.com> wrote: > Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), > blkg's stats is only flushed if they are online. I'm not sure I follow -- css_release_work_fn/cgroup_rstat_flush may be called on an offlined blkcg (offlined!=released). There's no invariant ensuring offlined blkcg won't be flushed. (There is only current situation when there is no reader of io data that'd need them flushed [1].) > In addition, the stat flushing of blkgs in blkcg_rstat_flush() > includes propagating the rstat data to its parent. However, if a blkg > has been destroyed (offline), the validity of its parent may be > questionable. Parents won't be freed (neither offlined) before children (see css_killed_work_fn). It should be regularly OK to pass data into a parent of an offlined blkcg. > For safety, revert back to the old behavior by ignoring offline > blkg's. I don't know if this is a good reasoning. If you argue that offlined children needn't be taken into parent's account, then I think it's more efficient to exclude the offlined blkcgs from update. (With the caveat I have in [1].) Regards, Michal [1] https://lore.kernel.org/r/YqEfNgUc8jxlAq8D@blackbook/
On 12/12/22 07:59, Michal Koutný wrote: > Hello. > > On Sun, Dec 11, 2022 at 05:20:57PM -0500, Waiman Long <longman@redhat.com> wrote: >> Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), >> blkg's stats is only flushed if they are online. > I'm not sure I follow -- css_release_work_fn/cgroup_rstat_flush may be > called on an offlined blkcg (offlined!=released). There's no invariant > ensuring offlined blkcg won't be flushed. (There is only current > situation when there is no reader of io data that'd need them flushed > [1].) The original cgroup_rstat_flush() iterates the list of blkg's in the blkg_list. A blkg will be removed from the list when it is offlined. This patch just reverts its behavior to that previous behavior. > >> In addition, the stat flushing of blkgs in blkcg_rstat_flush() >> includes propagating the rstat data to its parent. However, if a blkg >> has been destroyed (offline), the validity of its parent may be >> questionable. > Parents won't be freed (neither offlined) before children (see > css_killed_work_fn). It should be regularly OK to pass data into a > parent of an offlined blkcg. I guess it is likely to be safe to flush an offline blkg. I am just being conservative in case there is any corner case where it may be a problem which I haven't foreseen. > >> For safety, revert back to the old behavior by ignoring offline >> blkg's. > I don't know if this is a good reasoning. If you argue that offlined > children needn't be taken into parent's account, then I think it's more > efficient to exclude the offlined blkcgs from update. (With the caveat I > have in [1].) It is possible that a blkg may be updated before it becomes offline, but the flush isn't done in time before that happens. The next patch will catch some of that. Cheers, Longman
On Mon, Dec 12, 2022 at 01:59:53PM +0100, Michal Koutný wrote: > Hello. > > On Sun, Dec 11, 2022 at 05:20:57PM -0500, Waiman Long <longman@redhat.com> wrote: > > Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), > > blkg's stats is only flushed if they are online. > > I'm not sure I follow -- css_release_work_fn/cgroup_rstat_flush may be > called on an offlined blkcg (offlined!=released). There's no invariant > ensuring offlined blkcg won't be flushed. (There is only current > situation when there is no reader of io data that'd need them flushed > [1].) > > > In addition, the stat flushing of blkgs in blkcg_rstat_flush() > > includes propagating the rstat data to its parent. However, if a blkg > > has been destroyed (offline), the validity of its parent may be > > questionable. > > Parents won't be freed (neither offlined) before children (see > css_killed_work_fn). It should be regularly OK to pass data into a > parent of an offlined blkcg. > > > For safety, revert back to the old behavior by ignoring offline > > blkg's. > > I don't know if this is a good reasoning. If you argue that offlined > children needn't be taken into parent's account, then I think it's more > efficient to exclude the offlined blkcgs from update. (With the caveat I > have in [1].) Yeah, I'm not sure about this patch either. Doesn't seem necessary. Thanks.
On 12/12/22 17:16, Tejun Heo wrote: > On Mon, Dec 12, 2022 at 01:59:53PM +0100, Michal Koutný wrote: >> Hello. >> >> On Sun, Dec 11, 2022 at 05:20:57PM -0500, Waiman Long <longman@redhat.com> wrote: >>> Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), >>> blkg's stats is only flushed if they are online. >> I'm not sure I follow -- css_release_work_fn/cgroup_rstat_flush may be >> called on an offlined blkcg (offlined!=released). There's no invariant >> ensuring offlined blkcg won't be flushed. (There is only current >> situation when there is no reader of io data that'd need them flushed >> [1].) >> >>> In addition, the stat flushing of blkgs in blkcg_rstat_flush() >>> includes propagating the rstat data to its parent. However, if a blkg >>> has been destroyed (offline), the validity of its parent may be >>> questionable. >> Parents won't be freed (neither offlined) before children (see >> css_killed_work_fn). It should be regularly OK to pass data into a >> parent of an offlined blkcg. >> >>> For safety, revert back to the old behavior by ignoring offline >>> blkg's. >> I don't know if this is a good reasoning. If you argue that offlined >> children needn't be taken into parent's account, then I think it's more >> efficient to exclude the offlined blkcgs from update. (With the caveat I >> have in [1].) > Yeah, I'm not sure about this patch either. Doesn't seem necessary. I wrote this patch because I am not totally sure it is safe to flush offline blkgs. Since both you and Michal don't see any problem with it. I am fine taking it out. Cheers, Longman
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 21cc88349f21..c466aef0d467 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -885,6 +885,12 @@ static void blkcg_rstat_flush(struct cgroup_subsys_state *css, int cpu) WRITE_ONCE(bisc->lqueued, false); + /* Don't flush its stats if blkg is offline */ + if (unlikely(!blkg->online)) { + percpu_ref_put(&blkg->refcnt); + continue; + } + /* fetch the current per-cpu values */ do { seq = u64_stats_fetch_begin(&bisc->sync);
Before commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), blkg's stats is only flushed if they are online. In addition, the stat flushing of blkgs in blkcg_rstat_flush() includes propagating the rstat data to its parent. However, if a blkg has been destroyed (offline), the validity of its parent may be questionable. For safety, revert back to the old behavior by ignoring offline blkg's. Signed-off-by: Waiman Long <longman@redhat.com> --- block/blk-cgroup.c | 6 ++++++ 1 file changed, 6 insertions(+)