Re: [PATCH 0/13: eCryptfs] eCryptfs Patch Set

Christoph Hellwig
2006-07-07T11:54:27Z

On Sat, May 20, 2006 at 10:57:40AM +0100, Christoph Hellwig wrote:
> I gave the code in -mm a quick look, and it still needs a lot of work.

looking over the code again:

> - please kill ECRYPTFS_SET_FLAG, ECRYPTFS_CLEAR_FLAG and ECRYPTFS_CHECK_FLAG
> and just opencode them, it'll make the code a whole lot more readable

these are still there..

> - please make sure touch_atime goes down to ->setattr for atime updates,
> that way you don't need all that mess in your read/write. and in -mm
> those routines need update for vectored and aio support

you don't seem to have updated the common code for this yet.

> - NEVER EVER do things like copying locks_delete_block and
> posix_lock_file_wait (as ecryptfs_posix_lock and based on a previous
> version) to you code. It will get stale and create a maintaince nightmare.
> talk with the subsystem maintainers on how to make the core functionality
> accesible to you.
> - similarly ecryptfs_setlk is totally non-acceptable. find a way with the
> maintainer to reuse things from fcntl_setlk with a common helper

dito

> - copying things like lock_parent, unlock_parent and unlock_dir

still there. sorry, but there's zero changes to get things merged that
opencode

>
> - please split all the generic stackable filesystem passthorugh routines
> into a separated stackfs layer, in a few files in fs/stackfs/ that
> you depend on. They'll get _GPL exported to all possible stackable
> filesystem. They'll need their own store underlying object helpers,
> but that can be made to work by embedding the generic stackfs data
> as first thing in the ecryptfs object.
>
>
>

More comments:

- what protects accessing d_parent in lock_parent / unlock_parent?
- no need to cast the return value of kmem_cache_alloc, it's void *
- any reason to use the SLAB_* flags instead of GFP_? I'm a bit surprised
SLAB_* still exists at all..

- follow_link handling is wrong. you need to call the underlying
->follow_link in your follow_link implementation and you need to keep
a separate nameidata for it

- please kill that ugly ecryptfs_allocated_caches hack and do normal
goto based unwinding on failure

- using iget with the lower filesystems i_ino does not work. There are
various filesystems were i_ino does not uniqueuely identify an inode.
You will probably need your own sequence numbers. Also please don't
use iget in new code but always the iget_locked variant.

And a more general issue with implementing stackable filesystems:

I think it's probably much better to not stack ontop of a part of the
existing namespace but rather let ecryptfs mount the underlying filesystem
internally using vfs_kern_mount. This gets out of the way of possible
lock order problems when doing namespace operation involving both the
stacked and underlying filesystem aswell as allowing using a stackable
filesystem as the root filesystem.