[RFC PATCH] afs: Create a mountpoint through symlink() and remove through rmdir()

[RFC PATCH] afs: Create a mountpoint through symlink() and remove through rmdir()

[David Howells]

If symlink() is given a magic prefix in the target string, turn it into a
mountpoint instead.

The prefix is "//_afs3_mount:".  POSIX says that a double slash at the
beginning of a filename is special:

    A pathname consisting of a single slash shall resolve to the root
    directory of the process.  A null pathname shall not be successfully
    resolved.  A pathname that begins with two successive slashes may be
    interpreted in an implementation-defined manner, although more than two
    leading slashes shall be treated as a single slash.

however, that might be validly interpreted by Windows as a UNC name.  So
the prefix is made a bit more than that to make that less likely.  The
prefix is then stripped off and a "." is added for transmission to the
server.

afs_mntpt_lookup() is removed so that the new dentry is marked as being an
autodir type.  afs_rmdir() then checks for this and switches over to
afs_unlink() to remove a mountpoint (as far as the server is concerned,
it's not a directory).  The unlink() system call can't be used to remove
the mountpoint without alteration to the core VFS as may_delete() throws an
error.  This allows rmdir() to remove an automount point (provided it's not
mounted over).

Note that this behaviour varies from other AFS implementations in that the
proposed symlink *could* be valid content (though it seems unlikely) and in
those implementations rmdir can't remove a mountpoint.  Since the Linux
symlink() system can handle a target string of 4095 characters, but AFS can
only handle a symlink of 1024 chars, it might be better to make the magic
prefix large enough to force the target string length over the server
limit.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/dir.c       |   38 +++++++++++++++++++++++++++++++++++---
 fs/afs/fsclient.c  |    9 +++++----
 fs/afs/internal.h  |    6 ++++--
 fs/afs/mntpt.c     |   15 ---------------
 fs/afs/yfsclient.c |    7 ++++---
 5 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 5c794f4b051a..40552036cd6a 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -1365,6 +1365,9 @@ static int afs_rmdir(struct inode *dir, struct dentry *dentry)
 	_enter("{%llx:%llu},{%pd}",
 	       dvnode->fid.vid, dvnode->fid.vnode, dentry);
 
+	if (d_is_autodir(dentry))
+		return afs_unlink(dir, dentry);
+
 	scb = kzalloc(sizeof(struct afs_status_cb), GFP_KERNEL);
 	if (!scb)
 		return -ENOMEM;
@@ -1721,17 +1724,21 @@ static int afs_link(struct dentry *from, struct inode *dir,
 	return ret;
 }
 
+static const char afs_mount_prefix[] = "//_afs3_mount:";
+
 /*
- * create a symlink in an AFS filesystem
+ * create a symlink or a mountpoint in an AFS filesystem
  */
 static int afs_symlink(struct inode *dir, struct dentry *dentry,
-		       const char *content)
+		       const char *specified_content)
 {
 	struct afs_iget_data iget_data;
 	struct afs_fs_cursor fc;
 	struct afs_status_cb *scb;
 	struct afs_vnode *dvnode = AFS_FS_I(dir);
 	struct key *key;
+	char *content = (char *)specified_content;
+	bool is_mountpoint = false;
 	int ret;
 
 	_enter("{%llx:%llu},{%pd},%s",
@@ -1746,6 +1753,28 @@ static int afs_symlink(struct inode *dir, struct dentry *dentry,
 	if (strlen(content) >= AFSPATHMAX)
 		goto error;
 
+	if (memcmp(content, afs_mount_prefix,
+		   sizeof(afs_mount_prefix) - 1) == 0) {
+		/* This is going to be a mountpoint. */
+		char *p = content + sizeof(afs_mount_prefix) - 1, *c;
+		size_t clen = strlen(p);
+
+		if (clen < 2 ||
+		    (p[0] != '%' && p[0] != '#'))
+			goto error;
+
+		/* Snip off the prefix and append a dot */
+		ret = -ENOMEM;
+		c = kmalloc(clen + 2, GFP_KERNEL);
+		if (!c)
+			goto error;
+		memcpy(c, p, clen);
+		c[clen] = '.';
+		c[clen + 1] = 0;
+		content = c;
+		is_mountpoint = true;
+	}
+
 	ret = -ENOMEM;
 	scb = kcalloc(2, sizeof(struct afs_status_cb), GFP_KERNEL);
 	if (!scb)
@@ -1765,7 +1794,8 @@ static int afs_symlink(struct inode *dir, struct dentry *dentry,
 			fc.cb_break = afs_calc_vnode_cb_break(dvnode);
 			afs_prep_for_new_inode(&fc, &iget_data);
 			afs_fs_symlink(&fc, dentry->d_name.name, content,
-				       &scb[0], &iget_data.fid, &scb[1]);
+				       &scb[0], &iget_data.fid, &scb[1],
+				       is_mountpoint);
 		}
 
 		afs_check_for_remote_deletion(&fc, dvnode);
@@ -1794,6 +1824,8 @@ static int afs_symlink(struct inode *dir, struct dentry *dentry,
 error_scb:
 	kfree(scb);
 error:
+	if (content != specified_content)
+		kfree(content);
 	d_drop(dentry);
 	_leave(" = %d", ret);
 	return ret;
diff --git a/fs/afs/fsclient.c b/fs/afs/fsclient.c
index 1f9c5d8e6fe5..e2a2abe3a9aa 100644
--- a/fs/afs/fsclient.c
+++ b/fs/afs/fsclient.c
@@ -896,14 +896,15 @@ static const struct afs_call_type afs_RXFSSymlink = {
 };
 
 /*
- * create a symbolic link
+ * Create a symbolic link or a mountpoint (differentiated by mode).
  */
 int afs_fs_symlink(struct afs_fs_cursor *fc,
 		   const char *name,
 		   const char *contents,
 		   struct afs_status_cb *dvnode_scb,
 		   struct afs_fid *newfid,
-		   struct afs_status_cb *new_scb)
+		   struct afs_status_cb *new_scb,
+		   bool is_mountpoint)
 {
 	struct afs_vnode *dvnode = fc->vnode;
 	struct afs_call *call;
@@ -913,7 +914,7 @@ int afs_fs_symlink(struct afs_fs_cursor *fc,
 
 	if (test_bit(AFS_SERVER_FL_IS_YFS, &fc->cbi->server->flags))
 		return yfs_fs_symlink(fc, name, contents, dvnode_scb,
-				      newfid, new_scb);
+				      newfid, new_scb, is_mountpoint);
 
 	_enter("");
 
@@ -959,7 +960,7 @@ int afs_fs_symlink(struct afs_fs_cursor *fc,
 	*bp++ = htonl(dvnode->vfs_inode.i_mtime.tv_sec); /* mtime */
 	*bp++ = 0; /* owner */
 	*bp++ = 0; /* group */
-	*bp++ = htonl(S_IRWXUGO); /* unix mode */
+	*bp++ = htonl(is_mountpoint ? 0644 : S_IRWXUGO); /* unix mode */
 	*bp++ = 0; /* segment size */
 
 	afs_use_fs_server(call, fc->cbi);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 1d81fc4c3058..70509f2ddd00 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -965,7 +965,8 @@ extern int afs_fs_remove(struct afs_fs_cursor *, struct afs_vnode *, const char
 extern int afs_fs_link(struct afs_fs_cursor *, struct afs_vnode *, const char *,
 		       struct afs_status_cb *, struct afs_status_cb *);
 extern int afs_fs_symlink(struct afs_fs_cursor *, const char *, const char *,
-			  struct afs_status_cb *, struct afs_fid *, struct afs_status_cb *);
+			  struct afs_status_cb *, struct afs_fid *, struct afs_status_cb *,
+			  bool);
 extern int afs_fs_rename(struct afs_fs_cursor *, const char *,
 			 struct afs_vnode *, const char *,
 			 struct afs_status_cb *, struct afs_status_cb *);
@@ -1370,7 +1371,8 @@ extern int yfs_fs_remove(struct afs_fs_cursor *, struct afs_vnode *, const char
 extern int yfs_fs_link(struct afs_fs_cursor *, struct afs_vnode *, const char *,
 		       struct afs_status_cb *, struct afs_status_cb *);
 extern int yfs_fs_symlink(struct afs_fs_cursor *, const char *, const char *,
-			  struct afs_status_cb *, struct afs_fid *, struct afs_status_cb *);
+			  struct afs_status_cb *, struct afs_fid *, struct afs_status_cb *,
+			  bool);
 extern int yfs_fs_rename(struct afs_fs_cursor *, const char *, struct afs_vnode *, const char *,
 			 struct afs_status_cb *, struct afs_status_cb *);
 extern int yfs_fs_store_data(struct afs_fs_cursor *, struct address_space *,
diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index 79bc5f1338ed..b06ceed9b8f5 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -17,9 +17,6 @@
 #include "internal.h"
 
 
-static struct dentry *afs_mntpt_lookup(struct inode *dir,
-				       struct dentry *dentry,
-				       unsigned int flags);
 static int afs_mntpt_open(struct inode *inode, struct file *file);
 static void afs_mntpt_expiry_timed_out(struct work_struct *work);
 
@@ -29,7 +26,6 @@ const struct file_operations afs_mntpt_file_operations = {
 };
 
 const struct inode_operations afs_mntpt_inode_operations = {
-	.lookup		= afs_mntpt_lookup,
 	.readlink	= page_readlink,
 	.getattr	= afs_getattr,
 	.listxattr	= afs_listxattr,
@@ -46,17 +42,6 @@ static unsigned long afs_mntpt_expiry_timeout = 10 * 60;
 
 static const char afs_root_volume[] = "root.cell";
 
-/*
- * no valid lookup procedure on this sort of dir
- */
-static struct dentry *afs_mntpt_lookup(struct inode *dir,
-				       struct dentry *dentry,
-				       unsigned int flags)
-{
-	_enter("%p,%p{%pd2}", dir, dentry, dentry);
-	return ERR_PTR(-EREMOTE);
-}
-
 /*
  * no valid open procedure on this sort of dir
  */
diff --git a/fs/afs/yfsclient.c b/fs/afs/yfsclient.c
index a26126ac7bf1..5ff95d5643f5 100644
--- a/fs/afs/yfsclient.c
+++ b/fs/afs/yfsclient.c
@@ -1080,14 +1080,15 @@ static const struct afs_call_type yfs_RXYFSSymlink = {
 };
 
 /*
- * Create a symbolic link.
+ * Create a symbolic link or a mountpoint (differentiated by mode).
  */
 int yfs_fs_symlink(struct afs_fs_cursor *fc,
 		   const char *name,
 		   const char *contents,
 		   struct afs_status_cb *dvnode_scb,
 		   struct afs_fid *newfid,
-		   struct afs_status_cb *vnode_scb)
+		   struct afs_status_cb *vnode_scb,
+		   bool is_mountpoint)
 {
 	struct afs_vnode *dvnode = fc->vnode;
 	struct afs_call *call;
@@ -1125,7 +1126,7 @@ int yfs_fs_symlink(struct afs_fs_cursor *fc,
 	bp = xdr_encode_YFSFid(bp, &dvnode->fid);
 	bp = xdr_encode_string(bp, name, namesz);
 	bp = xdr_encode_string(bp, contents, contents_sz);
-	bp = xdr_encode_YFSStoreStatus_mode(bp, S_IRWXUGO);
+	bp = xdr_encode_YFSStoreStatus_mode(bp, is_mountpoint ? 0644 : S_IRWXUGO);
 	yfs_check_req(call, bp);
 
 	afs_use_fs_server(call, fc->cbi);

Re: [RFC PATCH] afs: Create a mountpoint through symlink() and remove through rmdir()

[Linus Torvalds]

On Wed, Feb 19, 2020 at 4:11 PM David Howells <dhowells@redhat.com> wrote:
>
> If symlink() is given a magic prefix in the target string, turn it into a
> mountpoint instead.
>
> The prefix is "//_afs3_mount:".

That sounds sane.

Your argument that if the prefix is made really long it couldn't be a
valid symlink at all on AFS is fair, but seems somewhat excessive.

The only issue I see with this interface is that you can now create
these kinds of things by untarring a tar-ball etc.

I can see that being both very convenient and a possible security
pain. But I'm assuming that the real security is on the server side
anyway and not just anybody can create arbitrary things like these?

         Linus

Re: [RFC PATCH] afs: Create a mountpoint through symlink() and remove through rmdir()

[Jeffrey E Altman]

[-- Attachment #1.1: Type: text/plain, Size: 3342 bytes --]

Hi Linus,

response inline ...

On 2/19/2020 8:21 PM, Linus Torvalds wrote:
> On Wed, Feb 19, 2020 at 4:11 PM David Howells <dhowells@redhat.com> wrote:
>>
>> If symlink() is given a magic prefix in the target string, turn it into a
>> mountpoint instead.
>>
>> The prefix is "//_afs3_mount:".
> 
> That sounds sane.
> 
> Your argument that if the prefix is made really long it couldn't be a
> valid symlink at all on AFS is fair, but seems somewhat excessive.

My understanding is that the prefix is not something that will be stored
on the server. Isn't the prefix intended to provide a namespace so that
a symlink target passed to a smb3/cifs filesystem can filter out the
invalid server name?

AFS symlinks have a maximum target string length of 1024 octets
including the trailing NUL.  The maximum cell name is 255 which matches
the maximum DNS name length.  The maximum volume name length is 255
in AuriStorFS and 31 in AFS3.

> The only issue I see with this interface is that you can now create
> these kinds of things by untarring a tar-ball etc.

There is no security issue here.  AFS volumes do not need to be mounted
in the /afs file namespace in order to access the volume's root
directory.  A mount point such as implemented here is just a reference
to a volume root.

OpenAFS provides a generic interface

  /afs/.:mount/<cell-name>:<volume-name-or-id>/

to access any volume's root directory and

  /afs/.:mount/<cell-name>:<volume-name-or-id>:<vnode-id>:<unique-id>

to access any AFS3 object by its object identifier whether its a
directory, file, symlink, etc.

Linux can of course mount arbitrary volumes using mount. For example:

 mount -t afs "#auristor.com:user.jaltman." /home/jaltman

There are existing AFS tools such as afs-up which clones a directory
tree from one place to another.  On Windows the AFS3 mount points and
symlinks are exposed as reparse points and can be copied from place to
place using tools such as Microsoft's robocopy; including to NTFS.

AFS3 mount points are also stored in AFS dump files which are similar to
tar files.

> I can see that being both very convenient and a possible security
> pain. But I'm assuming that the real security is on the server side
> anyway and not just anybody can create arbitrary things like these?

The security of AFS3 is provided by the enforcement of Access Controls
as interpreted by the AFS3 fileservers.  For OpenAFS the ACLs are stored
only on directories.  AuriStorFS cells enforce Volume ACLs and Object
ACLs which include ACLs on directories, files, symlinks and mount points.

ACL interpretation is performed by evaluating the applicable ACLs
against the current protection set (user id and membership group ids)
attributed to the combined authentication identities bound to the RPC
request.

If curious,

 https://www.auristor.com/documentation/man/linux/7/auristorfs_acls.html

The computed rights granted to the caller are transmitted to the
requesting client as part of the status response.  This permits the
caller to cache the permissions along with the Callback promise.  The
permissions are discarded by the client when the Callback promise is
broken or expires.

Thanks for your assistance with refining the functional implementation.

Jeffrey Altman


[-- Attachment #1.2: jaltman.vcf --]
[-- Type: text/x-vcard, Size: 283 bytes --]

begin:vcard
fn:Jeffrey Altman
n:Altman;Jeffrey
org:AuriStor, Inc.
adr:;;255 W 94TH ST STE 6B;New York;NY;10025-6985;United States
email;internet:jaltman@auristor.com
title:CEO
tel;work:+1-212-769-9018
url:https://www.linkedin.com/in/jeffreyaltman/
version:2.1
end:vcard


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4033 bytes --]

Re: [RFC PATCH] afs: Create a mountpoint through symlink() and remove through rmdir()

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Your argument that if the prefix is made really long it couldn't be a
> valid symlink at all on AFS is fair, but seems somewhat excessive.

Normally, mountpoint creation and removal would be hidden behind the
appropriate command line tool and not done directly with ln/rmdir.

> The only issue I see with this interface is that you can now create
> these kinds of things by untarring a tar-ball etc.

That would be true with mknod approach too.  Even if tar couldn't do the
second-stage write, it could still create a lot of half-formed mountpoints
by extracting chardevs.

David