NACK: [SRU][J][PATCH 2/5] cgroup: Homogenize cgroup_get_from_id() return value

[Koichiro Den]

On Wed, Feb 12, 2025 at 03:46:52PM GMT, Philip Cox wrote:
> From: Michal Koutný <mkoutny at suse.com>
> 
> Cgroup id is user provided datum hence extend its return domain to
> include possible error reason (similar to cgroup_get_from_fd()).
> 
> This change also fixes commit d4ccaf58a847 ("bpf: Introduce cgroup
> iter") that would use NULL instead of proper error handling in
> d4ccaf58a847 ("bpf: Introduce cgroup iter").
> 
> Additionally, neither of: fc_appid_store, bpf_iter_attach_cgroup,
> mem_cgroup_get_from_ino (callers of cgroup_get_from_fd) is built without
> CONFIG_CGROUPS (depends via CONFIG_BLK_CGROUP, direct, transitive
> CONFIG_MEMCG respectively) transitive, so drop the singular definition
> not needed with !CONFIG_CGROUPS.
> 
> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> Signed-off-by: Michal Koutný <mkoutny at suse.com>
> Signed-off-by: Tejun Heo <tj at kernel.org>
> (backported from commit fa7e439cf90ba23ea473d0b7d85efd02ae6ccf94)
> [philcox: context changes around mem_cgroup_get_from_ino()]
> Signed-off-by: Philip Cox <philip.cox at canonical.com>
> ---
>  include/linux/cgroup.h |  5 -----
>  kernel/cgroup/cgroup.c |  4 ++--
>  mm/memcontrol.c        | 23 +++++++++++++++++++++++
>  3 files changed, 25 insertions(+), 7 deletions(-)

blkcg_set_fc_appid() resides in include/linux/blk-cgroup.h.  Since upstream
commit db05628435aa ("blk-cgroup: move blkcg_{get,set}_fc_appid out of
line") (v5.19-rc1~125^2~40) has not been backported to Jammy, we need to
apply the original hunk [1] for block/blk-cgroup-fc-appid.c to
include/linux/blk-cgroup.h.

    [1]
    --- a/block/blk-cgroup-fc-appid.c
    +++ b/block/blk-cgroup-fc-appid.c
    @@ -19,8 +19,8 @@ int blkcg_set_fc_appid(char *app_id, u64 cgrp_id, size_t app_id_len)
                    return -EINVAL;
    
            cgrp = cgroup_get_from_id(cgrp_id);
    -       if (!cgrp)
    -               return -ENOENT;
    +       if (IS_ERR(cgrp))
    +               return PTR_ERR(cgrp);
            css = cgroup_get_e_css(cgrp, &io_cgrp_subsys);
            if (!css) {
                    ret = -ENOENT;

> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 0d97d1cf660f7..92a147bfacbbd 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -753,11 +753,6 @@ static inline bool task_under_cgroup_hierarchy(struct task_struct *task,
>  
>  static inline void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen)
>  {}
> -
> -static inline struct cgroup *cgroup_get_from_id(u64 id)
> -{
> -	return NULL;
> -}
>  #endif /* !CONFIG_CGROUPS */
>  
>  #ifdef CONFIG_CGROUPS
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 27d503fd17fe5..219a138e22923 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6032,7 +6032,7 @@ void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen)
>  /*
>   * cgroup_get_from_id : get the cgroup associated with cgroup id
>   * @id: cgroup id
> - * On success return the cgrp, on failure return NULL
> + * On success return the cgrp or ERR_PTR on failure
>   * Only cgroups within current task's cgroup NS are valid.
>   */
>  struct cgroup *cgroup_get_from_id(u64 id)
> @@ -6068,7 +6068,7 @@ struct cgroup *cgroup_get_from_id(u64 id)
>  		cgrp = NULL;
>  	}
>  out:
> -	return cgrp;
> +	return cgrp ?: ERR_PTR(-ENOENT);
>  }
>  EXPORT_SYMBOL_GPL(cgroup_get_from_id);
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 63a03ec849353..16619f3b2f119 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5149,6 +5149,29 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id)
>  	return idr_find(&mem_cgroup_idr, id);
>  }
>  
> +#ifdef CONFIG_SHRINKER_DEBUG
> +struct mem_cgroup *mem_cgroup_get_from_ino(unsigned long ino)
> +{
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *memcg;
> +
> +	cgrp = cgroup_get_from_id(ino);
> +	if (IS_ERR(cgrp))
> +		return PTR_ERR(cgrp);
> +
> +	css = cgroup_get_e_css(cgrp, &memory_cgrp_subsys);
> +	if (css)
> +		memcg = container_of(css, struct mem_cgroup, css);
> +	else
> +		memcg = ERR_PTR(-ENOENT);
> +
> +	cgroup_put(cgrp);
> +
> +	return memcg;
> +}
> +#endif
> +

It appears that mem_cgroup_get_from_ino() implementation from upstream
commit c15187a4a2d6 ("mm: memcontrol: introduce mem_cgroup_ino() and
mem_cgroup_get_from_ino()") was accidentally mixed in. It's for shrinker
debugfs, and the entire patch series for the new feature
(https://lore.kernel.org/all/20220601032227.4076670-2-roman.gushchin@linux.dev/)
has not been backported to Jammy.
The original hunk for mem_cgroup_get_from_ino() should just be ignored.

>  static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
> -- 
> 2.43.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team