linux-kernel - Re: [PATCH v2] d_ino considered harmful

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: <2F857D9A-3B76-4309-8967-4405C6D609F0@oracle.com>
Date:	Thu, 17 Jun 2010 17:39:17 -0600
From:	Andreas Dilger <andreas.dilger@...cle.com>
To:	Valerie Aurora <vaurora@...hat.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	Christoph Hellwig <hch@...radead.org>,
	Miklos Szeredi <miklos@...redi.hu>,
	Jan Blunck <jblunck@...e.de>,
	Jamie Lokier <jamie@...reable.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Arnd Bergmann <arnd@...db.de>,
	"linux-kernel@...r.kernel.org Mailinglist" 
	<linux-kernel@...r.kernel.org>, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] d_ino considered harmful

On 2010-06-16, at 13:54, Valerie Aurora wrote:
> On Wed, Jun 16, 2010 at 02:59:13PM -0400, Valerie Aurora wrote:
>> Who needs d_ino anyway?  I am running a kernel with this patch -
>> Gnome, a browser, IRC, kernel compile, etc. and everything works.
> 
> Gosh, maybe it would help to patch the currently used readdir instead
> of just old_readdir() (thanks, Arnd).  And return 1 instead of 0 so ls
> doesn't think all files are deleted (thanks, Andreas).
> 
> I'm running a kernel with the below patch and everything still works.
> Apparently "ls -i" is still using the bogus d_ino performance
> improvement mentioned here because it returns all 1's for inode
> number.

I don't see why the presence of d_ino is a "bogus" performance optimization.  It is useful for some things, and replacing this with "1" by no means helps anything IMHO, and destroys some useful optimizations (e.g. finding which inodes may be hard links), so I'm against this patch.

> http://www.mail-archive.com/bug-findutils@gnu.org/msg02531.html
> 
> -VAL
> 
> commit 5902fd7b7407e059c5cea1bf1ea101a1ff8a6072
> Author: Valerie Aurora <vaurora@...hat.com>
> Date:   Wed Jun 16 11:05:06 2010 -0700
> 
>    VFS: Always return 1 for d_ino
> 
>    Use of d_ino without the corresponding st_dev is always buggy in the
>    presence of submounts, bind mounts, and union mounts.  E.g., the d_ino
>    of a mountpoint will be the inode number of the directory under the
>    mountpoint, not the mounted directory.  Correct code must call stat(),
>    which returns the correct device ID and inode in st_dev and st_ino.
>    Since no one should be using d_ino anyway, always return 1 to detect
>    bugs.
> 
> diff --git a/fs/readdir.c b/fs/readdir.c
> index dd3eae1..5ff8f10 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -91,11 +91,8 @@ static int fillonedir(void * __buf, const char * name, int namlen, loff_t offset
> 
> 	if (buf->result)
> 		return -EINVAL;
> -	d_ino = ino;
> -	if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
> -		buf->result = -EOVERFLOW;
> -		return -EOVERFLOW;
> -	}
> +	/* Use of d_ino without st_dev is always buggy. */
> +	d_ino = 1;
> 	buf->result++;
> 	dirent = buf->dirent;
> 	if (!access_ok(VERIFY_WRITE, dirent,
> @@ -172,11 +169,8 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> 	buf->error = -EINVAL;	/* only used if we fail.. */
> 	if (reclen > buf->count)
> 		return -EINVAL;
> -	d_ino = ino;
> -	if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
> -		buf->error = -EOVERFLOW;
> -		return -EOVERFLOW;
> -	}
> +	/* Use of d_ino without st_dev is always buggy. */
> +	d_ino = 1;
> 	dirent = buf->previous;
> 	if (dirent) {
> 		if (__put_user(offset, &dirent->d_off))


Cheers, Andreas
--
Andreas Dilger
Lustre Technical Lead
Oracle Corporation Canada Inc.

--
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/