| Message ID | 20221010023859.11896-4-shikemeng@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A few cleanup patches for blk-cgroup | expand |
On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote: > Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in > pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL > before use for pd_alloc_fn in blkcg_activate_policy to avoid protential > NULL dereference. > > Signed-off-by: Kemeng Shi <shikemeng@huawei.com> > --- > block/blk-cgroup.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index 463c568d3e86..fc083c35dc42 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q, > if (blkcg_policy_enabled(q, pol)) > return 0; > > + if (pol->pd_alloc_fn == NULL) > + return -EINVAL; This isn't the only place this function is called, so the above won't achieve much. Given that this is rather trivially noticeable and all the current users do implement pd_alloc_fn, I'm not sure we need to update this now. Thanks.
on 10/11/2022 4:29 AM, Tejun Heo wrote: > On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote: >> Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in >> pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL >> before use for pd_alloc_fn in blkcg_activate_policy to avoid protential >> NULL dereference. >> >> Signed-off-by: Kemeng Shi <shikemeng@huawei.com> >> --- >> block/blk-cgroup.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c >> index 463c568d3e86..fc083c35dc42 100644 >> --- a/block/blk-cgroup.c >> +++ b/block/blk-cgroup.c >> @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q, >> if (blkcg_policy_enabled(q, pol)) >> return 0; >> >> + if (pol->pd_alloc_fn == NULL) >> + return -EINVAL; > > This isn't the only place this function is called, so the above won't > achieve much. Given that this is rather trivially noticeable and all the > current users do implement pd_alloc_fn, I'm not sure we need to update this > now. Thanks for review. The rest call of this function will always protect by blkcg_policy_enabled while policy only can be enabled if new added NULL check is passed. So the new added NULL check enough. By the way, the policy enable/disable work is direct call to __set_bit(pol->plid, q->blkcg_pols) in blkcg_policy_enabled and __clear_bit(pol->plid, q->blkcg_pols) in blkcg_deactivate_policy which is not intuitive. Is it a good idea to add function blkcg_policy_enable and blkcg_policy_disable to improve readability? > > Thanks. >
Hi, Tejun 在 2022/10/11 4:29, Tejun Heo 写道: > On Mon, Oct 10, 2022 at 10:38:58AM +0800, Kemeng Shi wrote: >> Function only make sure pd_alloc_fn and pd_free_fn in >> pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL >> before use for pd_alloc_fn in blkcg_activate_policy to avoid protential >> NULL dereference. >> >> Signed-off-by: Kemeng Shi <shikemeng@huawei.com> >> --- >> block/blk-cgroup.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c >> index 463c568d3e86..fc083c35dc42 100644 >> --- a/block/blk-cgroup.c >> +++ b/block/blk-cgroup.c >> @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q, >> if (blkcg_policy_enabled(q, pol)) >> return 0; >> >> + if (pol->pd_alloc_fn == NULL) >> + return -EINVAL; > > This isn't the only place this function is called, so the above won't > achieve much. Given that this is rather trivially noticeable and all the > current users do implement pd_alloc_fn, I'm not sure we need to update this > now. It's right all the current users implement pd_alloc_fn, can we check pd_alloc/free_fn NULL instead in blkcg_policy_register()? Thanks, Kuai > > Thanks. >
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 463c568d3e86..fc083c35dc42 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1404,6 +1404,9 @@ int blkcg_activate_policy(struct request_queue *q, if (blkcg_policy_enabled(q, pol)) return 0; + if (pol->pd_alloc_fn == NULL) + return -EINVAL; + if (queue_is_mq(q)) blk_mq_freeze_queue(q); retry:
Function blkcg_policy_register only make sure pd_alloc_fn and pd_free_fn in pairs, so pd_alloc_fn could be NULL in registered blkcg_policy. Check NULL before use for pd_alloc_fn in blkcg_activate_policy to avoid protential NULL dereference. Signed-off-by: Kemeng Shi <shikemeng@huawei.com> --- block/blk-cgroup.c | 3 +++ 1 file changed, 3 insertions(+)