linux-kernel - Re: [PATCH 2/2] kernfs: Allow creation with external gen + ino numbers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191024175201.GB3622521@devbig004.ftw2.facebook.com>
Date:   Thu, 24 Oct 2019 10:52:01 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Johannes Weiner <hannes@...xchg.org>,
        Li Zefan <lizefan@...wei.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Song Liu <liu.song.a23@...il.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>
Subject: Re: [PATCH 2/2] kernfs: Allow creation with external gen + ino
 numbers

On Wed, Oct 16, 2019 at 09:50:19PM +0900, Namhyung Kim wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 6ebae6bbe6a5..f2e54532c110 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -618,10 +618,10 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
>  					     struct kernfs_node *parent,
>  					     const char *name, umode_t mode,
>  					     kuid_t uid, kgid_t gid,
> +					     u32 gen, int ino,

Shouldn't this be ino_t so that we can use 64bit uniq id as ino
directly where possible?  Also, it might make more sense if ino comes
before gen.

> @@ -635,11 +635,24 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
>  
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(&kernfs_idr_lock);
> -	cursor = idr_get_cursor(&root->ino_idr);
> -	ret = idr_alloc_cyclic(&root->ino_idr, kn, 1, 0, GFP_ATOMIC);
> -	if (ret >= 0 && ret < cursor)
> -		root->next_generation++;
> -	gen = root->next_generation;
> +
> +	if (ino == 0) {
> +		cursor = idr_get_cursor(&root->ino_idr);
> +		ret = idr_alloc_cyclic(&root->ino_idr, kn, 1, 0, GFP_ATOMIC);
> +		if (ret >= 0 && ret < cursor)
> +			root->next_generation++;
> +		gen = root->next_generation;
> +	} else {
> +		ret = idr_alloc(&root->ino_idr, kn, ino, ino + 1, GFP_ATOMIC);
> +		if (ret != ino) {
> +			WARN_ONCE(1, "kernfs ino was used: %d", ino);
> +			ret = -EINVAL;
> +		} else {
> +			WARN_ON(root->next_generation > gen);
> +			root->next_generation = gen;
> +		}
> +	}

Oh, I see, so the code is still depending on idr and thus the use of
32bit ino.  Hmm....

>  static inline struct kernfs_node *
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 44c67d26c1fe..13d0d181a9f8 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3916,16 +3916,22 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
>  	char name[CGROUP_FILE_NAME_MAX];
>  	struct kernfs_node *kn;
>  	struct lock_class_key *key = NULL;
> +	struct cgroup_root *root = cgrp->root;
> +	int ino, gen;
>  	int ret;
>  
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  	key = &cft->lockdep_key;
>  #endif
> +
> +	ino = cgroup_idr_alloc(&root->cgroup_idr, NULL, false, GFP_KERNEL);
> +	gen = root->cgroup_idr.generation;
> +
>  	kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
>  				  cgroup_file_mode(cft),
>  				  GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
>  				  0, cft->kf_ops, cft,
> -				  NULL, key);
> +				  NULL, key, gen, ino);

Can we move this to a separate patch so that this patch can be an
identity conversion?

Thanks.

-- 
tejun