linux-kernel - Re: [PATCH 4/4] eCryptfs: ino

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]

Date:	Wed, 06 Sep 2006 10:44:45 +0100
From:	David Howells <dhowells@...hat.com>
To:	Michael Halcrow <mhalcrow@...ibm.com>
Cc:	David Howells <dhowells@...hat.com>, akpm@...l.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] eCryptfs: ino_t to u64 for filldir 

Michael Halcrow <mhalcrow@...ibm.com> wrote:

> Is it safe to assume that a pointer will always be equal to or smaller
> than an unsigned long for all architectures?

I presume you're talking about the sizes of the types.  Inside the Linux
kernel:

	sizeof(void *) == sizeof(unsigned long)

must always hold true.  There are too many things that depend on it to do
otherwise.

> Casting a pointer to an unsigned long, in general, makes me a bit
> uncomfortable.

Pointers are just numbers that are used in a particular way.  Go and look in
linux/err.h:-)

> Dave Chinner told me that XFS uses 32-bit inode numbers on 32-bit
> machines, so I imagine that this patch really is only helpful for NFS.

At the moment, maybe, but they could always change it.  And what about Reiser4?

> +int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode)
> +{
> +	if (ecryptfs_inode_to_private(inode) && 

Is this part of the condition actually necessary?  Can you not guarantee that
this will always be true?

> +	ecryptfs_set_inode_lower(inode, igrab((struct inode *)lower_inode));

igrab() might fail.  I would recommend doing it before calling iget5_unlocked()
and drop the extraneous reference to lower_inode afterwards if the eCryptFS
inode returned is already set up.

You're also casting lower_inode twice.  Whilst there's nothing actually wrong
with that, it might look better if you assigned it to its own variable at the
top of the function and only do the cast once.

> +static void ecryptfs_read_inode(struct inode *inode) { }
> +

You shouldn't need that any more.  Just leave the read_inode op pointer unset.
The NULL pointer exception handler will let you know if anyone tries to access
it (which they shouldn't - only *you* should call iget*() on your own inodes).

Looks good otherwise.  Just the igrab() thing is a real problem.

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