| Message ID | 20230606180724.2455066-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-cgroup: Reinit blkg_iostat_set after clearing in blkcg_reset_stats() | expand |
On Tue, Jun 06, 2023 at 02:07:24PM -0400, Waiman Long wrote: > When blkg_alloc() is called to allocate a blkcg_gq structure > with the associated blkg_iostat_set's, there are 2 fields within > blkg_iostat_set that requires proper initialization - blkg & sync. > The former field was introduced by commit 3b8cc6298724 ("blk-cgroup: > Optimize blkcg_rstat_flush()") while the later one was introduced by > commit f73316482977 ("blk-cgroup: reimplement basic IO stats using > cgroup rstat"). > > Unfortunately those fields in the blkg_iostat_set's are not properly > re-initialized when they are cleared in v1's blkcg_reset_stats(). This > can lead to a kernel panic due to NULL pointer access of the blkg > pointer. The missing initialization of sync is less problematic and > can be a problem in a debug kernel due to missing lockdep initialization. > > Fix these problems by re-initializing them after memory clearing. > > Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") > Fixes: f73316482977 ("blk-cgroup: reimplement basic IO stats using cgroup rstat") > Signed-off-by: Waiman Long <longman@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Tue, Jun 06, 2023 at 02:07:24PM -0400, Waiman Long wrote: > When blkg_alloc() is called to allocate a blkcg_gq structure > with the associated blkg_iostat_set's, there are 2 fields within > blkg_iostat_set that requires proper initialization - blkg & sync. > The former field was introduced by commit 3b8cc6298724 ("blk-cgroup: > Optimize blkcg_rstat_flush()") while the later one was introduced by > commit f73316482977 ("blk-cgroup: reimplement basic IO stats using > cgroup rstat"). > > Unfortunately those fields in the blkg_iostat_set's are not properly > re-initialized when they are cleared in v1's blkcg_reset_stats(). This > can lead to a kernel panic due to NULL pointer access of the blkg > pointer. The missing initialization of sync is less problematic and > can be a problem in a debug kernel due to missing lockdep initialization. > > Fix these problems by re-initializing them after memory clearing. > > Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") > Fixes: f73316482977 ("blk-cgroup: reimplement basic IO stats using cgroup rstat") > Signed-off-by: Waiman Long <longman@redhat.com> > --- > block/blk-cgroup.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index 90c2efc3767f..48084321e3e0 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -636,8 +636,13 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css, > struct blkg_iostat_set *bis = > per_cpu_ptr(blkg->iostat_cpu, cpu); > memset(bis, 0, sizeof(*bis)); > + > + /* Re-initialize the cleared blkg_iostat_set */ > + u64_stats_init(&bis->sync); > + bis->blkg = blkg; > } > memset(&blkg->iostat, 0, sizeof(blkg->iostat)); > + u64_stats_init(&blkg->iostat.sync); > > for (i = 0; i < BLKCG_MAX_POLS; i++) { > struct blkcg_policy *pol = blkcg_policy[i]; Looks fine, Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On Tue, 06 Jun 2023 14:07:24 -0400, Waiman Long wrote: > When blkg_alloc() is called to allocate a blkcg_gq structure > with the associated blkg_iostat_set's, there are 2 fields within > blkg_iostat_set that requires proper initialization - blkg & sync. > The former field was introduced by commit 3b8cc6298724 ("blk-cgroup: > Optimize blkcg_rstat_flush()") while the later one was introduced by > commit f73316482977 ("blk-cgroup: reimplement basic IO stats using > cgroup rstat"). > > [...] Applied, thanks! [1/1] blk-cgroup: Reinit blkg_iostat_set after clearing in blkcg_reset_stats() commit: 3d2af77e31ade05ff7ccc3658c3635ec1bea0979 Best regards,
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 90c2efc3767f..48084321e3e0 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -636,8 +636,13 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css, struct blkg_iostat_set *bis = per_cpu_ptr(blkg->iostat_cpu, cpu); memset(bis, 0, sizeof(*bis)); + + /* Re-initialize the cleared blkg_iostat_set */ + u64_stats_init(&bis->sync); + bis->blkg = blkg; } memset(&blkg->iostat, 0, sizeof(blkg->iostat)); + u64_stats_init(&blkg->iostat.sync); for (i = 0; i < BLKCG_MAX_POLS; i++) { struct blkcg_policy *pol = blkcg_policy[i];
When blkg_alloc() is called to allocate a blkcg_gq structure with the associated blkg_iostat_set's, there are 2 fields within blkg_iostat_set that requires proper initialization - blkg & sync. The former field was introduced by commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") while the later one was introduced by commit f73316482977 ("blk-cgroup: reimplement basic IO stats using cgroup rstat"). Unfortunately those fields in the blkg_iostat_set's are not properly re-initialized when they are cleared in v1's blkcg_reset_stats(). This can lead to a kernel panic due to NULL pointer access of the blkg pointer. The missing initialization of sync is less problematic and can be a problem in a debug kernel due to missing lockdep initialization. Fix these problems by re-initializing them after memory clearing. Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") Fixes: f73316482977 ("blk-cgroup: reimplement basic IO stats using cgroup rstat") Signed-off-by: Waiman Long <longman@redhat.com> --- block/blk-cgroup.c | 5 +++++ 1 file changed, 5 insertions(+)