> > > > let alone removing the interface that doesn't require checks to be
> > > > vfsmount-based for all users.
> > >
> > > What users? There are paractically _no_ other users. The ones that
> > > there are (like reiserfs) should not be using them, and there are
> > > already some patches cleaning that mess up.
> >
> > OK, explain me, in small words, WTF should something that wants to do
> > operations on filesystem tree have a vfsmount. Slowly. And "r/o
> > bind loses value if it can be bypassed" is a hogwash - fs methods are
> > still there, so it *can* be bypassed just fine, thank you very much.
>
> And we know what to do with such users.

Which begs the question: why is ecryptfs doing that with the xattr
methods? Does it need to bypass the permission checks? Seems very
fishy to me.

Mike?

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

On Mon, Apr 28, 2008 at 12:15:33PM +0200, Miklos Szeredi wrote:
> Which begs the question: why is ecryptfs doing that with the xattr
> methods? Does it need to bypass the permission checks? Seems very
> fishy to me.

Yes, it was mainly to avoid the permission checks, since eCryptfs
needs to be able to freely manipulate the cryptographic metadata
stored in the xattr region of the lower file when the user mounts with
the option to use the xattr region. I just used the same function to
access the lower xattr (ecryptfs_setxattr(), for instance) for both
xattr passthrough and metadata manipulation. This clearly can be
changed at this point so that at least the xattr passthrough of xattr
ops explicitly done by the user uses the vfs_* xattr calls instead.

However, in terms of permissions that eCryptfs needs, there are some
semantics that I need to work out. For instance, if eCryptfs
absolutely respects a rule that says that the lower file may only be
opened append-only, even by root, then eCryptfs cannot do its job,
which may include writing out the crypto metadata to the xattr of the
lower file. In that case, an operation on the lower fs will succeed,
but that exact same operation on the file under eCryptfs will fail,
since xattr.c::xattr_permission() will return -EPERM if
IS_APPEND(inode), and an open in eCryptfs will automatically entail an
xattr write if the mount is done with instructions to write the
metadata to the xattr regions of the lower files.

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

> > Which begs the question: why is ecryptfs doing that with the xattr
> > methods? Does it need to bypass the permission checks? Seems very
> > fishy to me.
>
> Yes, it was mainly to avoid the permission checks, since eCryptfs
> needs to be able to freely manipulate the cryptographic metadata
> stored in the xattr region of the lower file when the user mounts with
> the option to use the xattr region. I just used the same function to
> access the lower xattr (ecryptfs_setxattr(), for instance) for both
> xattr passthrough and metadata manipulation. This clearly can be
> changed at this point so that at least the xattr passthrough of xattr
> ops explicitly done by the user uses the vfs_* xattr calls instead.
>
> However, in terms of permissions that eCryptfs needs, there are some
> semantics that I need to work out. For instance, if eCryptfs
> absolutely respects a rule that says that the lower file may only be
> opened append-only, even by root, then eCryptfs cannot do its job,
> which may include writing out the crypto metadata to the xattr of the
> lower file. In that case, an operation on the lower fs will succeed,
> but that exact same operation on the file under eCryptfs will fail,
> since xattr.c::xattr_permission() will return -EPERM if
> IS_APPEND(inode), and an open in eCryptfs will automatically entail an
> xattr write if the mount is done with instructions to write the
> metadata to the xattr regions of the lower files.

A more serious problem, is that permissions are not always checked at
the VFS level, but often at some place in the filesystem (as well)
like the NFS server for example. Which means, that the current design
will fail miserably in those cases.

You don't have to care, of course, but I would rather have chosen a
design, where the stack doesn't have to care about implementation
details like that in the underlying filesystem.

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

On Thu, Apr 24, 2008 at 07:13:32PM +0100, Al Viro wrote:
> On Thu, Apr 24, 2008 at 01:29:49PM -0400, Erez Zadok wrote:
> > In message <20080424142857.GF15214@ZenIV.linux.org.uk>, Al Viro writes:
> > > On Thu, Apr 24, 2008 at 04:09:18PM +0200, Miklos Szeredi wrote:
> > [...]
> > > FWIW, I'm not all that happy about the way ecryptfs_interpose() is done,
> > > while we are at it. We get the sucker opened by whoever steps on given
> > > place in the tree first, with subsequent operations done using the resulting
> > > struct file. With fallback to r/o open. What happens to somebody who
> > > tries to open it with enough permissions to do r/w?
> >
> > Yes, ecryptfs_interpose() calls ecryptfs_init_persistent_file() which calls
> > dentry_open(O_RDWR). What's the proposed solution for this in the face of
> > r/o vfsmounts? How could ecryptfs avoid calling this dentry_open in the
> > first place?
>
> Doesn't have anything to do with vfsmounts (you have one to deal with and
> if it's r/o, it's equivalent to just doing the entire thing on top of r/o
> fs; not interesting).
>
> No, what I'm worried about is much simpler. Look: we have a file on
> underlying fs, owned by root.root with 644 for permissions. Comes a
> luser and tries to open the counterpart of that file in ecryptfs; that
> triggers ecryptfs_interpose() and attempts to open file. Of course,
> that's going to fail - it's not world-writable. So then it (actually
> ecryptfs_init_persistent_file()) falls back to opening with O_RDONLY.
> Which succeeds just fine and file (opened r/o) is set as ->lower_file.
>
> Now comes root and tries to open the damn thing r/w. It should be able
> to and if it came first it'd get it; as it is, what it gets is ->lower_file
> and that puppy is opened read-only and you have no guarantee that underlying
> fs will not go bonkers seeing write attempts on it (e.g. open for write
> doing a bit more setup of ->private_data, etc.).

This looks like the same ugly stuff that the nfsv4 server is currently
doing; see, e.g., fs/nfsd/nfs4state.c:nfs4_upgrade_open().

A concrete example of a bug (e.g., export FOOfs over nfsd, do an open
upgrade, watch the oops) would be great motivation to finally get this
fixed, if you know of such an example off the top of your head....

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

On Thu, 2008-04-24 at 13:39 +0200, Miklos Szeredi wrote:
> R/O bind mounts patches have been reviewed numerous times. Still a
> relatively large number of places remained where checking for R/O
> mounts was missed.

In all the other... erm... interesting discussion, I missed this point.

Were there some bits outside of the nfsd code where I missed the r/o
mount checks?

-- Dave

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

> > R/O bind mounts patches have been reviewed numerous times. Still a
> > relatively large number of places remained where checking for R/O
> > mounts was missed.
>
> In all the other... erm... interesting discussion, I missed this point.
>
> Were there some bits outside of the nfsd code where I missed the r/o
> mount checks?

Yes, the removexattr syscalls. Al fixed those in the final
submission.

And the whole of ecryptfs was missed as a source of filesystem
modification. How that's supposed to get fixed, along with nfsd is
arguable.

But regardless of all that, I think the path_* interface is a good
one, even if it has just a couple of users that actually _require_ the
r/o bracketing. It's good because:

- it's consistent

- it provides some (not all) guarantees, i.e. it's easier to prove
that all callers play by the rules

- for the syscall case it has zero cost

- for all the other cases it has either zero, or minimal cost

Yes, it does require the caller to have a vfsmount available, but it's
hard to imagine that the caller does not have it:

- most userspace calls do have it, as they are either operating on a
path or a file descriptor. There are some exceptions like sync(2)
and ustat(2), the latter not being a very exemplary interface, and
neither of them being relevant to this discussion.

- all kernel callers (nfs export, stacking) should have it, as they
need it for open() anyway

And even if some theoretical caller didn't have the vfsmount, it still
should be easy to allocate one, providing a clean way to do the r/o
bracketing, and not requiring another mechanism to be exported that
provides the same functionality on the superblock.

Sorry for the claptrap. I'm going to resubmit this one more time
with some slight modifications and additions, and it'd be really nice
if you or other interested parties would provide comments and opinions
(either way), because I don't think me and Al have anything new to say
to each other.

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

On Thu, 2008-05-01 at 10:08 +0200, Miklos Szeredi wrote:
> Sorry for the claptrap. I'm going to resubmit this one more time
> with some slight modifications and additions, and it'd be really nice
> if you or other interested parties would provide comments and opinions
> (either way), because I don't think me and Al have anything new to say
> to each other.

I was trying to stay out of it a bit, since you both have very good
points.

But, from a purely aesthetic point of view, I like the patches. They do
make r/o mount detection less error-prone in coding. They also remove
more code than they add.

I also certainly understand Al's point about why it puts vfsmounts in a
place where they probably shouldn't be exposed.

Is there a way we could pass around a vfs-internal object to these
things? Maybe something that is like an opaque token only known to the
vfs?

linux/mount.h:

struct hidden_mount;

fs/namespace.c:

struct hidden_mount {
struct vfsmount *mnt;
};

int mnt_want_write(struct hidden_mount *hidmnt)
{
return __mnt_want_write(hidmnt->mnt);
}

We could have *that* passed down the way that you're currently passing
the vfsmount.

-- Dave

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

> Is there a way we could pass around a vfs-internal object to these
> things? Maybe something that is like an opaque token only known to the
> vfs?

vfsmount itself should be an opaque token to anything but the
namespace code. So the definition could just be moved into say
fs/vfsmount.h, and be done with that.

Probably not a trivial cleanup to do (s_op->show_options() is a major
offender), but I don't see any major roadblocks.

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