[PATCH] fscrypto: make filename crypto functions return 0 on success

[PATCH] fscrypto: make filename crypto functions return 0 on success

[Eric Biggers]

Several filename crypto functions: fname_decrypt(),
fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
the output length on success or -errno on failure.  However, the output
length was redundant with the value written to 'oname->len'.  It is also
potentially error-prone to make callers have to check for '< 0' instead
of '!= 0'.

Therefore, make these functions return 0 instead of a length, and make
the callers who cared about the return value being a length use
'oname->len' instead.

This change also fixes the inconsistency of fname_encrypt() actually
already returning 0 on success, not a length like the other filename
crypto functions and as documented in its function comment.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c | 52 ++++++++++++++++++++++++++++++----------------------
 fs/ext4/dir.c     |  5 +++--
 fs/ext4/symlink.c |  5 ++---
 fs/f2fs/namei.c   |  4 ++--
 4 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6bbc3b1..49fd564 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
 }
 
 /**
- * fname_encrypt() -
+ * fname_encrypt() - encrypt a filename
  *
- * This function encrypts the input filename, and returns the length of the
- * ciphertext. Errors are returned as negative numbers.  We trust the caller to
- * allocate sufficient memory to oname string.
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
  */
 static int fname_encrypt(struct inode *inode,
 			const struct qstr *iname, struct fscrypt_str *oname)
@@ -105,20 +105,22 @@ static int fname_encrypt(struct inode *inode,
 	}
 	kfree(alloc_buf);
 	skcipher_request_free(req);
-	if (res < 0)
+	if (res < 0) {
 		printk_ratelimited(KERN_ERR
 				"%s: Error (error code %d)\n", __func__, res);
+		return res;
+	}
 
 	oname->len = ciphertext_len;
-	return res;
+	return 0;
 }
 
-/*
- * fname_decrypt()
- *	This function decrypts the input filename, and returns
- *	the length of the plaintext.
- *	Errors are returned as negative numbers.
- *	We trust the caller to allocate sufficient memory to oname string.
+/**
+ * fname_decrypt() - decrypt a filename
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
  */
 static int fname_decrypt(struct inode *inode,
 				const struct fscrypt_str *iname,
@@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
 	}
 
 	oname->len = strnlen(oname->name, iname->len);
-	return oname->len;
+	return 0;
 }
 
 static const char *lookup_table =
@@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
 /**
  * fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
  * space
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
  */
 int fscrypt_fname_disk_to_usr(struct inode *inode,
 			u32 hash, u32 minor_hash,
@@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
 {
 	const struct qstr qname = FSTR_TO_QSTR(iname);
 	char buf[24];
-	int ret;
 
 	if (fscrypt_is_dot_dotdot(&qname)) {
 		oname->name[0] = '.';
 		oname->name[iname->len - 1] = '.';
 		oname->len = iname->len;
-		return oname->len;
+		return 0;
 	}
 
 	if (iname->len < FS_CRYPTO_BLOCK_SIZE)
@@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
 		return fname_decrypt(inode, iname, oname);
 
 	if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
-		ret = digest_encode(iname->name, iname->len, oname->name);
-		oname->len = ret;
-		return ret;
+		oname->len = digest_encode(iname->name, iname->len,
+					   oname->name);
+		return 0;
 	}
 	if (hash) {
 		memcpy(buf, &hash, 4);
@@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
 	}
 	memcpy(buf + 8, iname->name + iname->len - 16, 16);
 	oname->name[0] = '_';
-	ret = digest_encode(buf, 24, oname->name + 1);
-	oname->len = ret + 1;
-	return ret + 1;
+	oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
+	return 0;
 }
 EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
 
 /**
  * fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
  * space
+ *
+ * The caller must have allocated sufficient memory for the @oname string.
+ *
+ * Return: 0 on success, -errno on failure
  */
 int fscrypt_fname_usr_to_disk(struct inode *inode,
 			const struct qstr *iname,
@@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
 		oname->name[0] = '.';
 		oname->name[iname->len - 1] = '.';
 		oname->len = iname->len;
-		return oname->len;
+		return 0;
 	}
 	if (inode->i_crypt_info)
 		return fname_encrypt(inode, iname, oname);
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 67415e0..4d4b910 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 					/* Directory is encrypted */
 					err = fscrypt_fname_disk_to_usr(inode,
 						0, 0, &de_name, &fstr);
+					de_name = fstr;
 					fstr.len = save_len;
-					if (err < 0)
+					if (err)
 						goto errout;
 					if (!dir_emit(ctx,
-					    fstr.name, err,
+					    de_name.name, de_name.len,
 					    le32_to_cpu(de->inode),
 					    get_dtype(sb, de->file_type)))
 						goto done;
diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
index 4d83d9e..55aaf30 100644
--- a/fs/ext4/symlink.c
+++ b/fs/ext4/symlink.c
@@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
 		goto errout;
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
-	if (res < 0)
+	if (res)
 		goto errout;
 
 	paddr = pstr.name;
 
 	/* Null-terminate the name */
-	if (res <= pstr.len)
-		paddr[res] = '\0';
+	paddr[pstr.len] = '\0';
 	if (cpage)
 		put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 73fa356..a5468c2 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 		goto errout;
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
-	if (res < 0)
+	if (res)
 		goto errout;
 
 	/* this is broken symlink case */
@@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	paddr = pstr.name;
 
 	/* Null-terminate the name */
-	paddr[res] = '\0';
+	paddr[pstr.len] = '\0';
 
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] fscrypto: make filename crypto functions return 0 on success

[Jaegeuk Kim]

Hi Eric,

On Wed, Sep 14, 2016 at 04:00:45PM -0700, Eric Biggers wrote:
> Several filename crypto functions: fname_decrypt(),
> fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> the output length on success or -errno on failure.  However, the output
> length was redundant with the value written to 'oname->len'.  It is also
> potentially error-prone to make callers have to check for '< 0' instead
> of '!= 0'.

At a glance, you missed some places like
- fscrypt_fname_usr_to_disk() in ext4_symlink() and f2fs_symlink()
- fscrypt_fname_disk_to_usr() in dx_show_leaf() and f2fs_fill_dentries()

Thanks,

> Therefore, make these functions return 0 instead of a length, and make
> the callers who cared about the return value being a length use
> 'oname->len' instead.
> 
> This change also fixes the inconsistency of fname_encrypt() actually
> already returning 0 on success, not a length like the other filename
> crypto functions and as documented in its function comment.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/crypto/fname.c | 52 ++++++++++++++++++++++++++++++----------------------
>  fs/ext4/dir.c     |  5 +++--
>  fs/ext4/symlink.c |  5 ++---
>  fs/f2fs/namei.c   |  4 ++--
>  4 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 6bbc3b1..49fd564 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
>  }
>  
>  /**
> - * fname_encrypt() -
> + * fname_encrypt() - encrypt a filename
>   *
> - * This function encrypts the input filename, and returns the length of the
> - * ciphertext. Errors are returned as negative numbers.  We trust the caller to
> - * allocate sufficient memory to oname string.
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>   */
>  static int fname_encrypt(struct inode *inode,
>  			const struct qstr *iname, struct fscrypt_str *oname)
> @@ -105,20 +105,22 @@ static int fname_encrypt(struct inode *inode,
>  	}
>  	kfree(alloc_buf);
>  	skcipher_request_free(req);
> -	if (res < 0)
> +	if (res < 0) {
>  		printk_ratelimited(KERN_ERR
>  				"%s: Error (error code %d)\n", __func__, res);
> +		return res;
> +	}
>  
>  	oname->len = ciphertext_len;
> -	return res;
> +	return 0;
>  }
>  
> -/*
> - * fname_decrypt()
> - *	This function decrypts the input filename, and returns
> - *	the length of the plaintext.
> - *	Errors are returned as negative numbers.
> - *	We trust the caller to allocate sufficient memory to oname string.
> +/**
> + * fname_decrypt() - decrypt a filename
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>   */
>  static int fname_decrypt(struct inode *inode,
>  				const struct fscrypt_str *iname,
> @@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
>  	}
>  
>  	oname->len = strnlen(oname->name, iname->len);
> -	return oname->len;
> +	return 0;
>  }
>  
>  static const char *lookup_table =
> @@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
>  /**
>   * fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
>   * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>   */
>  int fscrypt_fname_disk_to_usr(struct inode *inode,
>  			u32 hash, u32 minor_hash,
> @@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
>  {
>  	const struct qstr qname = FSTR_TO_QSTR(iname);
>  	char buf[24];
> -	int ret;
>  
>  	if (fscrypt_is_dot_dotdot(&qname)) {
>  		oname->name[0] = '.';
>  		oname->name[iname->len - 1] = '.';
>  		oname->len = iname->len;
> -		return oname->len;
> +		return 0;
>  	}
>  
>  	if (iname->len < FS_CRYPTO_BLOCK_SIZE)
> @@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
>  		return fname_decrypt(inode, iname, oname);
>  
>  	if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
> -		ret = digest_encode(iname->name, iname->len, oname->name);
> -		oname->len = ret;
> -		return ret;
> +		oname->len = digest_encode(iname->name, iname->len,
> +					   oname->name);
> +		return 0;
>  	}
>  	if (hash) {
>  		memcpy(buf, &hash, 4);
> @@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
>  	}
>  	memcpy(buf + 8, iname->name + iname->len - 16, 16);
>  	oname->name[0] = '_';
> -	ret = digest_encode(buf, 24, oname->name + 1);
> -	oname->len = ret + 1;
> -	return ret + 1;
> +	oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
> +	return 0;
>  }
>  EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
>  
>  /**
>   * fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
>   * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>   */
>  int fscrypt_fname_usr_to_disk(struct inode *inode,
>  			const struct qstr *iname,
> @@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
>  		oname->name[0] = '.';
>  		oname->name[iname->len - 1] = '.';
>  		oname->len = iname->len;
> -		return oname->len;
> +		return 0;
>  	}
>  	if (inode->i_crypt_info)
>  		return fname_encrypt(inode, iname, oname);
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 67415e0..4d4b910 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  					/* Directory is encrypted */
>  					err = fscrypt_fname_disk_to_usr(inode,
>  						0, 0, &de_name, &fstr);
> +					de_name = fstr;
>  					fstr.len = save_len;
> -					if (err < 0)
> +					if (err)
>  						goto errout;
>  					if (!dir_emit(ctx,
> -					    fstr.name, err,
> +					    de_name.name, de_name.len,
>  					    le32_to_cpu(de->inode),
>  					    get_dtype(sb, de->file_type)))
>  						goto done;
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 4d83d9e..55aaf30 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
>  		goto errout;
>  
>  	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> -	if (res < 0)
> +	if (res)
>  		goto errout;
>  
>  	paddr = pstr.name;
>  
>  	/* Null-terminate the name */
> -	if (res <= pstr.len)
> -		paddr[res] = '\0';
> +	paddr[pstr.len] = '\0';
>  	if (cpage)
>  		put_page(cpage);
>  	set_delayed_call(done, kfree_link, paddr);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 73fa356..a5468c2 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>  		goto errout;
>  
>  	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> -	if (res < 0)
> +	if (res)
>  		goto errout;
>  
>  	/* this is broken symlink case */
> @@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>  	paddr = pstr.name;
>  
>  	/* Null-terminate the name */
> -	paddr[res] = '\0';
> +	paddr[pstr.len] = '\0';
>  
>  	put_page(cpage);
>  	set_delayed_call(done, kfree_link, paddr);
> -- 
> 2.8.0.rc3.226.g39d4020

Re: [PATCH] fscrypto: make filename crypto functions return 0 on success

[Eric Biggers]

On Wed, Sep 14, 2016 at 05:10:21PM -0700, Jaegeuk Kim wrote:
> Hi Eric,
> 
> On Wed, Sep 14, 2016 at 04:00:45PM -0700, Eric Biggers wrote:
> > Several filename crypto functions: fname_decrypt(),
> > fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> > the output length on success or -errno on failure.  However, the output
> > length was redundant with the value written to 'oname->len'.  It is also
> > potentially error-prone to make callers have to check for '< 0' instead
> > of '!= 0'.
> 
> At a glance, you missed some places like
> - fscrypt_fname_usr_to_disk() in ext4_symlink() and f2fs_symlink()
> - fscrypt_fname_disk_to_usr() in dx_show_leaf() and f2fs_fill_dentries()
> 

Those callers don't have to be updated because they only check for an error
value --- they don't use the return value as a length.  I did check all the
callers of fscrypt_fname_{usr_to_disk,disk_to_usr()}, and only the three I
update in the patch used the return value as a length.

Eric

Re: [PATCH] fscrypto: make filename crypto functions return 0 on success

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 7531 bytes --]

On Sep 14, 2016, at 5:00 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> Several filename crypto functions: fname_decrypt(),
> fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> the output length on success or -errno on failure.  However, the output
> length was redundant with the value written to 'oname->len'.  It is also
> potentially error-prone to make callers have to check for '< 0' instead
> of '!= 0'.
> 
> Therefore, make these functions return 0 instead of a length, and make
> the callers who cared about the return value being a length use
> 'oname->len' instead.
> 
> This change also fixes the inconsistency of fname_encrypt() actually
> already returning 0 on success, not a length like the other filename
> crypto functions and as documented in its function comment.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/crypto/fname.c | 52 ++++++++++++++++++++++++++++++----------------------
> fs/ext4/dir.c     |  5 +++--
> fs/ext4/symlink.c |  5 ++---
> fs/f2fs/namei.c   |  4 ++--
> 4 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 6bbc3b1..49fd564 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -35,11 +35,11 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
> }
> 
> /**
> - * fname_encrypt() -
> + * fname_encrypt() - encrypt a filename
>  *
> - * This function encrypts the input filename, and returns the length of the
> - * ciphertext. Errors are returned as negative numbers.  We trust the caller to
> - * allocate sufficient memory to oname string.
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>  */
> static int fname_encrypt(struct inode *inode,
> 			const struct qstr *iname, struct fscrypt_str *oname)
> @@ -105,20 +105,22 @@ static int fname_encrypt(struct inode *inode,
> 	}
> 	kfree(alloc_buf);
> 	skcipher_request_free(req);
> -	if (res < 0)
> +	if (res < 0) {
> 		printk_ratelimited(KERN_ERR
> 				"%s: Error (error code %d)\n", __func__, res);
> +		return res;
> +	}
> 
> 	oname->len = ciphertext_len;
> -	return res;
> +	return 0;
> }
> 
> -/*
> - * fname_decrypt()
> - *	This function decrypts the input filename, and returns
> - *	the length of the plaintext.
> - *	Errors are returned as negative numbers.
> - *	We trust the caller to allocate sufficient memory to oname string.
> +/**
> + * fname_decrypt() - decrypt a filename
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>  */
> static int fname_decrypt(struct inode *inode,
> 				const struct fscrypt_str *iname,
> @@ -168,7 +170,7 @@ static int fname_decrypt(struct inode *inode,
> 	}
> 
> 	oname->len = strnlen(oname->name, iname->len);
> -	return oname->len;
> +	return 0;
> }
> 
> static const char *lookup_table =
> @@ -279,6 +281,10 @@ EXPORT_SYMBOL(fscrypt_fname_free_buffer);
> /**
>  * fscrypt_fname_disk_to_usr() - converts a filename from disk space to user
>  * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>  */
> int fscrypt_fname_disk_to_usr(struct inode *inode,
> 			u32 hash, u32 minor_hash,
> @@ -287,13 +293,12 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> {
> 	const struct qstr qname = FSTR_TO_QSTR(iname);
> 	char buf[24];
> -	int ret;
> 
> 	if (fscrypt_is_dot_dotdot(&qname)) {
> 		oname->name[0] = '.';
> 		oname->name[iname->len - 1] = '.';
> 		oname->len = iname->len;
> -		return oname->len;
> +		return 0;
> 	}
> 
> 	if (iname->len < FS_CRYPTO_BLOCK_SIZE)
> @@ -303,9 +308,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> 		return fname_decrypt(inode, iname, oname);
> 
> 	if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) {
> -		ret = digest_encode(iname->name, iname->len, oname->name);
> -		oname->len = ret;
> -		return ret;
> +		oname->len = digest_encode(iname->name, iname->len,
> +					   oname->name);
> +		return 0;
> 	}
> 	if (hash) {
> 		memcpy(buf, &hash, 4);
> @@ -315,15 +320,18 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> 	}
> 	memcpy(buf + 8, iname->name + iname->len - 16, 16);
> 	oname->name[0] = '_';
> -	ret = digest_encode(buf, 24, oname->name + 1);
> -	oname->len = ret + 1;
> -	return ret + 1;
> +	oname->len = 1 + digest_encode(buf, 24, oname->name + 1);
> +	return 0;
> }
> EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
> 
> /**
>  * fscrypt_fname_usr_to_disk() - converts a filename from user space to disk
>  * space
> + *
> + * The caller must have allocated sufficient memory for the @oname string.
> + *
> + * Return: 0 on success, -errno on failure
>  */
> int fscrypt_fname_usr_to_disk(struct inode *inode,
> 			const struct qstr *iname,
> @@ -333,7 +341,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
> 		oname->name[0] = '.';
> 		oname->name[iname->len - 1] = '.';
> 		oname->len = iname->len;
> -		return oname->len;
> +		return 0;
> 	}
> 	if (inode->i_crypt_info)
> 		return fname_encrypt(inode, iname, oname);
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 67415e0..4d4b910 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -260,11 +260,12 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> 					/* Directory is encrypted */
> 					err = fscrypt_fname_disk_to_usr(inode,
> 						0, 0, &de_name, &fstr);
> +					de_name = fstr;
> 					fstr.len = save_len;
> -					if (err < 0)
> +					if (err)
> 						goto errout;
> 					if (!dir_emit(ctx,
> -					    fstr.name, err,
> +					    de_name.name, de_name.len,
> 					    le32_to_cpu(de->inode),
> 					    get_dtype(sb, de->file_type)))
> 						goto done;
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 4d83d9e..55aaf30 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -67,14 +67,13 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
> 		goto errout;
> 
> 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> -	if (res < 0)
> +	if (res)
> 		goto errout;
> 
> 	paddr = pstr.name;
> 
> 	/* Null-terminate the name */
> -	if (res <= pstr.len)
> -		paddr[res] = '\0';
> +	paddr[pstr.len] = '\0';
> 	if (cpage)
> 		put_page(cpage);
> 	set_delayed_call(done, kfree_link, paddr);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 73fa356..a5468c2 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -1048,7 +1048,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> 		goto errout;
> 
> 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
> -	if (res < 0)
> +	if (res)
> 		goto errout;
> 
> 	/* this is broken symlink case */
> @@ -1060,7 +1060,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> 	paddr = pstr.name;
> 
> 	/* Null-terminate the name */
> -	paddr[res] = '\0';
> +	paddr[pstr.len] = '\0';
> 
> 	put_page(cpage);
> 	set_delayed_call(done, kfree_link, paddr);
> --
> 2.8.0.rc3.226.g39d4020
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fscrypto: make filename crypto functions return 0 on success

[Jaegeuk Kim]

On Wed, Sep 14, 2016 at 07:54:41PM -0700, Eric Biggers wrote:
> On Wed, Sep 14, 2016 at 05:10:21PM -0700, Jaegeuk Kim wrote:
> > Hi Eric,
> > 
> > On Wed, Sep 14, 2016 at 04:00:45PM -0700, Eric Biggers wrote:
> > > Several filename crypto functions: fname_decrypt(),
> > > fscrypt_fname_disk_to_usr(), and fscrypt_fname_usr_to_disk(), returned
> > > the output length on success or -errno on failure.  However, the output
> > > length was redundant with the value written to 'oname->len'.  It is also
> > > potentially error-prone to make callers have to check for '< 0' instead
> > > of '!= 0'.
> > 
> > At a glance, you missed some places like
> > - fscrypt_fname_usr_to_disk() in ext4_symlink() and f2fs_symlink()
> > - fscrypt_fname_disk_to_usr() in dx_show_leaf() and f2fs_fill_dentries()
> > 
> 
> Those callers don't have to be updated because they only check for an error
> value --- they don't use the return value as a length.  I did check all the
> callers of fscrypt_fname_{usr_to_disk,disk_to_usr()}, and only the three I
> update in the patch used the return value as a length.

Hmm, your patch tries to change the return value and make its usage more
consistent. So, I think it should have been considered to change if (err < 0)
to if (err) entirely in order to avoid any potential mistake; this patch already
addresses it in some cases, but not all.

e.g., in f2fs_symlink,

	err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
	if (err < 0)
		goto err_out;

BTW, in dx_show_leaf(),
	res = fscrypt_fname_disk_to_usr(dir,
		0, 0, &de_name,
		&fname_crypto_str);
	if (res < 0)
		...

IMHO, it'd be better to use err instead of {res|ret} when considering naming
convention.

Thanks,

> 
> Eric

Re: [PATCH] fscrypto: make filename crypto functions return 0 on success

[Eric Biggers]

On Thu, Sep 15, 2016 at 09:54:12AM -0700, Jaegeuk Kim wrote:
> 
> Hmm, your patch tries to change the return value and make its usage more
> consistent. So, I think it should have been considered to change if (err < 0)
> to if (err) entirely in order to avoid any potential mistake; this patch already
> addresses it in some cases, but not all.
> 
> e.g., in f2fs_symlink,
> 
> 	err = fscrypt_fname_usr_to_disk(inode, &istr, &ostr);
> 	if (err < 0)
> 		goto err_out;
> 
> BTW, in dx_show_leaf(),
> 	res = fscrypt_fname_disk_to_usr(dir,
> 		0, 0, &de_name,
> 		&fname_crypto_str);
> 	if (res < 0)
> 		...
> 
> IMHO, it'd be better to use err instead of {res|ret} when considering naming
> convention.

Well, I had wanted to avoid bloating the patch.

I do like the idea of the greater consistency, though.

I'll post a version of the patch that does as you're suggesting.

Eric