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@xxxxxxxxxx> >> --- >> 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. > -- Best wishes Kemeng Shi
