Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup
@ 2019-12-03 12:56 Tiezhu Yang
  2019-12-03 13:56 ` Matthew Wilcox
  0 siblings, 1 reply; 5+ messages in thread
From: Tiezhu Yang @ 2019-12-03 12:56 UTC (permalink / raw)
  To: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks
  Cc: linux-fsdevel, ecryptfs, linux-fscrypt, linux-f2fs-devel, linux-kernel

There exists many similar and duplicate codes to check "." and "..",
so introduce is_dot_dotdot helper to make the code more clean.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - use the better performance implementation of is_dot_dotdot
  - make it static inline and move it to include/linux/fs.h

 fs/crypto/fname.c    | 15 ++-------------
 fs/ecryptfs/crypto.c | 13 ++-----------
 fs/f2fs/f2fs.h       | 11 -----------
 fs/namei.c           |  6 ++----
 include/linux/fs.h   | 10 ++++++++++
 5 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 3da3707..36be864 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -15,17 +15,6 @@
 #include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
-static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
-{
-	if (str->len == 1 && str->name[0] == '.')
-		return true;
-
-	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
-		return true;
-
-	return false;
-}
-
 /**
  * fname_encrypt() - encrypt a filename
  *
@@ -255,7 +244,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
 	const struct qstr qname = FSTR_TO_QSTR(iname);
 	struct fscrypt_digested_name digested_name;
 
-	if (fscrypt_is_dot_dotdot(&qname)) {
+	if (is_dot_dotdot(&qname)) {
 		oname->name[0] = '.';
 		oname->name[iname->len - 1] = '.';
 		oname->len = iname->len;
@@ -323,7 +312,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	memset(fname, 0, sizeof(struct fscrypt_name));
 	fname->usr_fname = iname;
 
-	if (!IS_ENCRYPTED(dir) || fscrypt_is_dot_dotdot(iname)) {
+	if (!IS_ENCRYPTED(dir) || is_dot_dotdot(iname)) {
 		fname->disk_name.name = (unsigned char *)iname->name;
 		fname->disk_name.len = iname->len;
 		return 0;
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index f91db24..6f4db74 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1991,16 +1991,6 @@ int ecryptfs_encrypt_and_encode_filename(
 	return rc;
 }
 
-static bool is_dot_dotdot(const char *name, size_t name_size)
-{
-	if (name_size == 1 && name[0] == '.')
-		return true;
-	else if (name_size == 2 && name[0] == '.' && name[1] == '.')
-		return true;
-
-	return false;
-}
-
 /**
  * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext
  * @plaintext_name: The plaintext name
@@ -2020,6 +2010,7 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name,
 {
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
 		&ecryptfs_superblock_to_private(sb)->mount_crypt_stat;
+	const struct qstr file_name = {.name = name, .len = name_size};
 	char *decoded_name;
 	size_t decoded_name_size;
 	size_t packet_size;
@@ -2027,7 +2018,7 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name,
 
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) &&
 	    !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)) {
-		if (is_dot_dotdot(name, name_size)) {
+		if (is_dot_dotdot(&file_name)) {
 			rc = ecryptfs_copy_filename(plaintext_name,
 						    plaintext_name_size,
 						    name, name_size);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5a888a0..3d5e684 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2767,17 +2767,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
 	return is_set_ckpt_flags(sbi, CP_ERROR_FLAG);
 }
 
-static inline bool is_dot_dotdot(const struct qstr *str)
-{
-	if (str->len == 1 && str->name[0] == '.')
-		return true;
-
-	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
-		return true;
-
-	return false;
-}
-
 static inline bool f2fs_may_extent_tree(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
diff --git a/fs/namei.c b/fs/namei.c
index 2dda552..babe7e8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2458,10 +2458,8 @@ static int lookup_one_len_common(const char *name, struct dentry *base,
 	if (!len)
 		return -EACCES;
 
-	if (unlikely(name[0] == '.')) {
-		if (len < 2 || (len == 2 && name[1] == '.'))
-			return -EACCES;
-	}
+	if (is_dot_dotdot(this))
+		return -EACCES;
 
 	while (len--) {
 		unsigned int c = *(const unsigned char *)name++;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98e0349..78a2932 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3627,4 +3627,14 @@ static inline int inode_drain_writes(struct inode *inode)
 	return filemap_write_and_wait(inode->i_mapping);
 }
 
+static inline bool is_dot_dotdot(const struct qstr *str)
+{
+	if (unlikely(str->name[0] == '.')) {
+		if (str->len < 2 || (str->len == 2 && str->name[1] == '.'))
+			return true;
+	}
+
+	return false;
+}
+
 #endif /* _LINUX_FS_H */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03 12:56 [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup Tiezhu Yang
@ 2019-12-03 13:56 ` Matthew Wilcox
  2019-12-05  0:56   ` Tiezhu Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2019-12-03 13:56 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

On Tue, Dec 03, 2019 at 08:56:50PM +0800, Tiezhu Yang wrote:
> There exists many similar and duplicate codes to check "." and "..",
> so introduce is_dot_dotdot helper to make the code more clean.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
> 
> v2:
>   - use the better performance implementation of is_dot_dotdot
>   - make it static inline and move it to include/linux/fs.h

Ugh, not more crap in fs.h.

$ ls -l --sort=size include/linux |head
-rw-r--r--  1 willy willy 154148 Nov 29 22:35 netdevice.h
-rw-r--r--  1 willy willy 130488 Nov 29 22:35 skbuff.h
-rw-r--r--  1 willy willy 123540 Nov 29 22:35 pci_ids.h
-rw-r--r--  1 willy willy 118991 Nov 29 22:35 fs.h

I think this probably fits well in namei.h.  And I think it works
better with bare 'name' and 'len' arguments, rather than taking a qstr.

And, as I asked twice in the last round of review, did you benchmark
this change?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-03 13:56 ` Matthew Wilcox
@ 2019-12-05  0:56   ` Tiezhu Yang
  2019-12-05  7:06     ` Matthew Wilcox
  0 siblings, 1 reply; 5+ messages in thread
From: Tiezhu Yang @ 2019-12-05  0:56 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

On 12/03/2019 09:56 PM, Matthew Wilcox wrote:
> On Tue, Dec 03, 2019 at 08:56:50PM +0800, Tiezhu Yang wrote:
>> There exists many similar and duplicate codes to check "." and "..",
>> so introduce is_dot_dotdot helper to make the code more clean.
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>
>> v2:
>>    - use the better performance implementation of is_dot_dotdot
>>    - make it static inline and move it to include/linux/fs.h
> Ugh, not more crap in fs.h.
>
> $ ls -l --sort=size include/linux |head
> -rw-r--r--  1 willy willy 154148 Nov 29 22:35 netdevice.h
> -rw-r--r--  1 willy willy 130488 Nov 29 22:35 skbuff.h
> -rw-r--r--  1 willy willy 123540 Nov 29 22:35 pci_ids.h
> -rw-r--r--  1 willy willy 118991 Nov 29 22:35 fs.h
>
> I think this probably fits well in namei.h.  And I think it works
> better with bare 'name' and 'len' arguments, rather than taking a qstr.

According to the definition of struct qstr:
"quick string" -- eases parameter passing, but more importantly saves
"metadata" about the string (ie length and the hash), it seems there
is no need to use qstr to only check "." and "..", I will use "name"
and "len" as arguments in the v3 patch and move it to namei.h:

static inline bool is_dot_dotdot(const unsigned char *name, size_t len)
{
         if (unlikely(name[0] == '.')) {
                 if (len < 2 || (len == 2 && name[1] == '.'))
                         return true;
         }

         return false;
}

>
> And, as I asked twice in the last round of review, did you benchmark
> this change?

Before sending this v2 patch, I have done the test used with your test
program and already pointed out the following implementation is better:

bool is_dot_dotdot(const struct qstr *str)
{
         if (unlikely(str->name[0] == '.')) {
                 if (str->len < 2 || (str->len == 2 && str->name[1] == '.'))
                         return true;
         }

         return false;
}

[enjoy@linux ~]$ lscpu | grep "Model name"
Model name:            Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz

[enjoy@linux ~]$ ./test
qstr . time_1 0.025204 time_2 0.023717
qstr .. time_1 0.036542 time_2 0.034330
qstr a time_1 0.028123 time_2 0.022514
qstr matthew time_1 0.024282 time_2 0.022617
qstr .a time_1 0.037132 time_2 0.034118
qstr , time_1 0.028121 time_2 0.022527
[enjoy@linux ~]$ ./test
qstr . time_1 0.025200 time_2 0.023666
qstr .. time_1 0.036486 time_2 0.034275
qstr a time_1 0.028113 time_2 0.022514
qstr matthew time_1 0.024289 time_2 0.022515
qstr .a time_1 0.037058 time_2 0.034063
qstr , time_1 0.028117 time_2 0.022523
[enjoy@linux ~]$ ./test
qstr . time_1 0.025128 time_2 0.023692
qstr .. time_1 0.036687 time_2 0.034284
qstr a time_1 0.028133 time_2 0.022527
qstr matthew time_1 0.024246 time_2 0.022589
qstr .a time_1 0.037063 time_2 0.034106
qstr , time_1 0.028127 time_2 0.022522

Additionally, by declaring is_dot_dotdot inline, we can make execution
faster by eliminating the function-call overhead, so the performance
influence is very small with this patch.

If you have any more suggestion, please let me know.

Thanks,

Tiezhu Yang


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-05  0:56   ` Tiezhu Yang
@ 2019-12-05  7:06     ` Matthew Wilcox
  2019-12-05  7:55       ` Tiezhu Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2019-12-05  7:06 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

On Thu, Dec 05, 2019 at 08:56:07AM +0800, Tiezhu Yang wrote:
> > And, as I asked twice in the last round of review, did you benchmark
> > this change?
> 
> Before sending this v2 patch, I have done the test used with your test
> program and already pointed out the following implementation is better:

I didn't mean "have you run the test program i wrote".  I meant "have you
booted a kernel with this change and done some performance measurements
to see if you've changed anything".

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup
  2019-12-05  7:06     ` Matthew Wilcox
@ 2019-12-05  7:55       ` Tiezhu Yang
  0 siblings, 0 replies; 5+ messages in thread
From: Tiezhu Yang @ 2019-12-05  7:55 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Alexander Viro, Theodore Y. Ts'o, Jaegeuk Kim, Chao Yu,
	Eric Biggers, Tyler Hicks, linux-fsdevel, ecryptfs,
	linux-fscrypt, linux-f2fs-devel, linux-kernel

On 12/05/2019 03:06 PM, Matthew Wilcox wrote:
> On Thu, Dec 05, 2019 at 08:56:07AM +0800, Tiezhu Yang wrote:
>>> And, as I asked twice in the last round of review, did you benchmark
>>> this change?
>> Before sending this v2 patch, I have done the test used with your test
>> program and already pointed out the following implementation is better:
> I didn't mean "have you run the test program i wrote".  I meant "have you
> booted a kernel with this change and done some performance measurements
> to see if you've changed anything".

Oh, no, it is hard to measure the performance influence with this patch.
Based on the above analysis, I think the performance influence is very
small due to is_dot_dotdot() is a such short static inline function.

Thanks,

Tiezhu Yang


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 12:56 [PATCH v2] fs: introduce is_dot_dotdot helper for cleanup Tiezhu Yang
2019-12-03 13:56 ` Matthew Wilcox
2019-12-05  0:56   ` Tiezhu Yang
2019-12-05  7:06     ` Matthew Wilcox
2019-12-05  7:55       ` Tiezhu Yang

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git