linux-kernel - Re: [PATCH 2/6] vfs: add d

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: <20081015195318.GA28349@infradead.org>
Date:	Wed, 15 Oct 2008 15:53:18 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc:	viro@...iv.linux.org.uk, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] vfs: add d_ancestor()

On Wed, Oct 15, 2008 at 10:58:10PM +0900, OGAWA Hirofumi wrote:
> 
> 
> This adds d_ancestor() instead of d_isparent(), then use it.
> 
> If new_dentry == old_dentry, is_subdir() returns 1, looks strange. But
> I'm not checking callers for now, so this keeps current behavior.

This change looks good, but a slightly more detailed changelog would be
good.

>  
>  /*
> - * Helper that returns 1 if p1 is a parent of p2, else 0
> + * Helper that returns the ancestor dentry of p2 which is a child of
> + * p1, if p1 is an ancestor of p2, else NULL.
>   */
> -static int d_isparent(struct dentry *p1, struct dentry *p2)
> +struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)

Given that d_ancestor is non-static a kerneldoc comment describing it
instead of a plain one would be good.

>  int is_subdir(struct dentry * new_dentry, struct dentry * old_dentry)
>  {
>  	int result;
> -	struct dentry * saved = new_dentry;
>  	unsigned long seq;
>  
> +	if (new_dentry == old_dentry)
> +		return 1;

Maybe a comment like in your commit description here would be good?

> +
>  	/* need rcu_readlock to protect against the d_parent trashing due to
>  	 * d_move
>  	 */
>  	rcu_read_lock();
>          do {
>  		/* for restarting inner loop in case of seq retry */
> -		new_dentry = saved;
> -		result = 0;
>  		seq = read_seqbegin(&rename_lock);
> -		for (;;) {
> -			if (new_dentry != old_dentry) {
> -				if (IS_ROOT(new_dentry))
> -					break;
> -				new_dentry = new_dentry->d_parent;
> -				continue;
> -			}
> -			result = 1;
> -			break;
> -		}
> +		result = (d_ancestor(old_dentry, new_dentry) != NULL);
>  	} while (read_seqretry(&rename_lock, seq));

That's a big improvement, but with a better loop and fixing all the
formatting mess in the old code it could become even better:

	/*
	 * Need rcu_readlock to protect against the d_parent trashing due to
	 * d_move.
	 */
	rcu_read_lock();
	do {
		/* for restarting inner loop in case of seq retry */
 		seq = read_seqbegin(&rename_lock);
		if (d_ancestor(old_dentry, new_dentry))
			result = 1;
		else
			result = 0;
	} while (read_seqretry(&rename_lock, seq));
 	rcu_read_unlock();


Nice set of patches!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/