[WTF?] aafs_create_symlink() weirdness

[WTF?] aafs_create_symlink() weirdness

[Al Viro]

static struct dentry *aafs_create_symlink(const char *name,
                                          struct dentry *parent,
                                          const char *target,
                                          void *private,
                                          const struct inode_operations *iops)
{
        struct dentry *dent;
        char *link = NULL;

        if (target) {
                if (!link)
                        return ERR_PTR(-ENOMEM);
        }

Er...  That's an odd way to spell
	if (target)
		return ERR_PTR(-ENOMEM);
(and an odd error value to use).  Especially since all callers are passing
NULL as target.

Why is that code even there, why does that argument still exist and
how many people have actually read that function?

Re: [WTF?] aafs_create_symlink() weirdness

[John Johansen]

On 9/23/19 7:03 PM, Al Viro wrote:

WTF indeed

> 
> static struct dentry *aafs_create_symlink(const char *name,
>                                           struct dentry *parent,
>                                           const char *target,
>                                           void *private,
>                                           const struct inode_operations *iops)
> {
>         struct dentry *dent;
>         char *link = NULL;
> 
>         if (target) {
>                 if (!link)
>                         return ERR_PTR(-ENOMEM);
>         }
> 
> Er...  That's an odd way to spell
> 	if (target)
> 		return ERR_PTR(-ENOMEM);
> (and an odd error value to use).  Especially since all callers are passing
> NULL as target.
> 
> Why is that code even there, why does that argument still exist and
> how many people have actually read that function?
> 

It looks like 1180b4c757aa failed to drop it as part of the patch, but it
certainly should have. I can't say why it happened but regardless its an ugly
mistake. Thank you very much for catching this Al.

A patch to drop it is below or feel free to cons up an alternate version.

---

commit 5dbc63d4a0aa819be8ecf21a67a352dd377b0221
Author: John Johansen <john.johansen@canonical.com>
Date:   Tue Sep 24 09:46:33 2019 -0700

    apparmor: remove useless aafs_create_symlink
    
    1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata after
    replacement") reworked how the rawdata symlink is handled but failed
    to remove aafs_create_symlink which was reduced to a useless stub .
    
    Fixes: 1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata after replacement")
    Reported-by: Al Viro <viro@zeniv.linux.org.uk>
    Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 9c0e593e30aa..308c99ea3cf8 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -335,38 +335,6 @@ static struct dentry *aafs_create_dir(const char *name, struct dentry *parent)
 			   NULL);
 }
 
-/**
- * aafs_create_symlink - create a symlink in the apparmorfs filesystem
- * @name: name of dentry to create
- * @parent: parent directory for this dentry
- * @target: if symlink, symlink target string
- * @private: private data
- * @iops: struct of inode_operations that should be used
- *
- * If @target parameter is %NULL, then the @iops parameter needs to be
- * setup to handle .readlink and .get_link inode_operations.
- */
-static struct dentry *aafs_create_symlink(const char *name,
-					  struct dentry *parent,
-					  const char *target,
-					  void *private,
-					  const struct inode_operations *iops)
-{
-	struct dentry *dent;
-	char *link = NULL;
-
-	if (target) {
-		if (!link)
-			return ERR_PTR(-ENOMEM);
-	}
-	dent = aafs_create(name, S_IFLNK | 0444, parent, private, link, NULL,
-			   iops);
-	if (IS_ERR(dent))
-		kfree(link);
-
-	return dent;
-}
-
 /**
  * aafs_remove - removes a file or directory from the apparmorfs filesystem
  *
@@ -1757,25 +1725,25 @@ int __aafs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
 	}
 
 	if (profile->rawdata) {
-		dent = aafs_create_symlink("raw_sha1", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_sha1_iops);
+		dent = aafs_create("raw_sha1", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_sha1_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);
 		profile->dents[AAFS_PROF_RAW_HASH] = dent;
 
-		dent = aafs_create_symlink("raw_abi", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_abi_iops);
+		dent = aafs_create("raw_abi", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_abi_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);
 		profile->dents[AAFS_PROF_RAW_ABI] = dent;
 
-		dent = aafs_create_symlink("raw_data", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_data_iops);
+		dent = aafs_create("raw_data", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_data_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);