linux-kernel - Re: [PATCH V3 03/12] kernfs: add an API to get kernfs node from inode number

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: <20170619185905.GH12062@htj.duckdns.org>
Date:   Mon, 19 Jun 2017 14:59:05 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Shaohua Li <shli@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        gregkh@...uxfoundation.org, hch@....de, axboe@...com,
        rostedt@...dmis.org, lizefan@...wei.com, Kernel-team@...com,
        Shaohua Li <shli@...com>
Subject: Re: [PATCH V3 03/12] kernfs: add an API to get kernfs node from
 inode number

Hello,

On Thu, Jun 15, 2017 at 11:17:11AM -0700, Shaohua Li wrote:
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 33f711f..7a4f327 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -508,6 +508,10 @@ void kernfs_put(struct kernfs_node *kn)
>  	struct kernfs_node *parent;
>  	struct kernfs_root *root;
>  
> +	/*
> +	 * kernfs_node is freed with ->count 0, kernfs_find_and_get_node_by_ino
> +	 * depends on this to filter reused stale node
> +	 */
>  	if (!kn || !atomic_dec_and_test(&kn->count))
>  		return;
>  	root = kernfs_root(kn);
> @@ -649,6 +653,8 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
>  	kn->ino = ret;
>  	kn->generation = gen;
>  
> +	/* set ino first. */
> +	smp_mb__before_atomic();

Can you please note what this is paired with here too?

> +/*

/**

> + * kernfs_find_and_get_node_by_ino - get kernfs_node from inode number
> + * @root: the kernfs root
> + * @ino: inode number
> + *
> + * RETURNS:
> + * NULL on failure. Return a kernfs node with reference counter incremented
> + */
> +struct kernfs_node *kernfs_find_and_get_node_by_ino(struct kernfs_root *root,
> +						    unsigned int ino)
> +{
> +	struct kernfs_node *kn;
> +
> +	rcu_read_lock();
> +	kn = idr_find(&root->ino_idr, ino);
> +	if (!kn)
> +		goto out;
> +
> +	/*
> +	 * Since kernfs_node is freed in RCU, it's possible an old node for ino
> +	 * is freed, but reused before RCU grace period. But a freed node (see
> +	 * kernfs_put) or an incompletedly initialized node (see
> +	 * __kernfs_new_node) should have 'count' 0. We can use this fact to
> +	 * filter out such node.
> +	 */
> +	if (!atomic_inc_not_zero(&kn->count)) {
> +		kn = NULL;
> +		goto out;
> +	}
> +
> +	/*
> +	 * The node could be a new node or a reused node. If it's a new node,
> +	 * we are ok. If it's reused because of RCU, the __kernfs_new_node
> +	 * always sets its 'ino' before 'count'. So if 'count' is uptodate,
> +	 * 'ino' should be uptodate, hence we can use 'ino' to filter stale
> +	 * node.
> +	 */

Maybe refer to SLAB_TYPESAFE_BY_RCU?  I still have a lingering sense
that we're overdoing the synchronization here.  I'm not sure this path
needs this level of sophisticated optimization.

> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index d5b149a..343dfeb 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -332,5 +332,7 @@ void __init kernfs_init(void)
>  {
>  	kernfs_node_cache = kmem_cache_create("kernfs_node_cache",
>  					      sizeof(struct kernfs_node),
> -					      0, SLAB_PANIC, NULL);
> +					      0,
> +					      SLAB_PANIC | SLAB_TYPESAFE_BY_RCU,
> +					      NULL);

Please point to the usage in kernfs_find_and_get_node_by_ino() here.

Thanks.

-- 
tejun