| Message ID | 1576030801-8609-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] fs: introduce is_dot_or_dotdot helper for cleanup | expand |
On Wed, Dec 11, 2019 at 10:20:01AM +0800, Tiezhu Yang wrote: > diff --git a/include/linux/namei.h b/include/linux/namei.h > index 7fe7b87..0fd9315 100644 > --- a/include/linux/namei.h > +++ b/include/linux/namei.h > @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) > return error == -ESTALE && !(flags & LOOKUP_REVAL); > } > > +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > +{ > + if (unlikely(name[0] == '.')) { > + if (len == 1 || (len == 2 && name[1] == '.')) > + return true; > + } > + > + return false; > +} > + > #endif /* _LINUX_NAMEI_H */ I had suggested adding a len >= 1 check to handle the empty name case correctly. What I had in mind was static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) { if (len >= 1 && unlikely(name[0] == '.')) { if (len < 2 || (len == 2 && name[1] == '.')) return true; } return false; } As is, you're proposing that it always dereference the first byte even when len=0, which seems like a bad idea for a shared helper function. Did you check whether it's okay for all the existing callers? fscrypt_fname_disk_to_usr() is called from 6 places, did you check all of them? How about keeping the existing optimized code for the hot path in fs/namei.c (i.e. not using the helper function), while having the helper function do the extra check to handle len=0 correctly? - Eric
On 12/11/2019 10:48 AM, Eric Biggers wrote: > On Wed, Dec 11, 2019 at 10:20:01AM +0800, Tiezhu Yang wrote: >> diff --git a/include/linux/namei.h b/include/linux/namei.h >> index 7fe7b87..0fd9315 100644 >> --- a/include/linux/namei.h >> +++ b/include/linux/namei.h >> @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) >> return error == -ESTALE && !(flags & LOOKUP_REVAL); >> } >> >> +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) >> +{ >> + if (unlikely(name[0] == '.')) { >> + if (len == 1 || (len == 2 && name[1] == '.')) >> + return true; >> + } >> + >> + return false; >> +} >> + >> #endif /* _LINUX_NAMEI_H */ > I had suggested adding a len >= 1 check to handle the empty name case correctly. > What I had in mind was > > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > { > if (len >= 1 && unlikely(name[0] == '.')) { > if (len < 2 || (len == 2 && name[1] == '.')) > return true; > } > > return false; > } > > As is, you're proposing that it always dereference the first byte even when > len=0, which seems like a bad idea for a shared helper function. Did you check > whether it's okay for all the existing callers? fscrypt_fname_disk_to_usr() is > called from 6 places, did you check all of them? > > How about keeping the existing optimized code for the hot path in fs/namei.c > (i.e. not using the helper function), while having the helper function do the > extra check to handle len=0 correctly? Hi Eric, Thank you for reminding me. How about using the following helper for all callers? static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) { if (len == 1 && name[0] == '.') return true; if (len == 2 && name[0] == '.' && name[1] == '.') return true; return false; } Hi Matthew, How do you think? I think the performance influence is very small due to is_dot_or_dotdot() is a such short static inline function. Thanks, Tiezhu Yang > > - Eric
On Wed, Dec 11, 2019 at 11:59:40AM +0800, Tiezhu Yang wrote: > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > { > if (len == 1 && name[0] == '.') > return true; > > if (len == 2 && name[0] == '.' && name[1] == '.') > return true; > > return false; > } > > Hi Matthew, > > How do you think? I think the performance influence is very small > due to is_dot_or_dotdot() is a such short static inline function. It's a very short inline function called on a very hot codepath. Often. I mean it - it's done literally for every pathname component of every pathname passed to a syscall.
On 12/11/2019 12:47 PM, Al Viro wrote: > On Wed, Dec 11, 2019 at 11:59:40AM +0800, Tiezhu Yang wrote: > >> static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) >> { >> if (len == 1 && name[0] == '.') >> return true; >> >> if (len == 2 && name[0] == '.' && name[1] == '.') >> return true; >> >> return false; >> } >> >> Hi Matthew, >> >> How do you think? I think the performance influence is very small >> due to is_dot_or_dotdot() is a such short static inline function. > It's a very short inline function called on a very hot codepath. > Often. > > I mean it - it's done literally for every pathname component of > every pathname passed to a syscall. OK. I understand. Let us do not use the helper function in fs/namei.c, just use the following implementation for other callers: static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) { if (len >= 1 && unlikely(name[0] == '.')) { if (len < 2 || (len == 2 && name[1] == '.')) return true; } return false; } Special thanks for Matthew, Darrick, Al and Eric. If you have any more suggestion, please let me know. Thanks, Tiezhu Yang
On Wed, Dec 11, 2019 at 02:38:34PM +0800, Tiezhu Yang wrote: > On 12/11/2019 12:47 PM, Al Viro wrote: > > On Wed, Dec 11, 2019 at 11:59:40AM +0800, Tiezhu Yang wrote: > > > > > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > > > { > > > if (len == 1 && name[0] == '.') > > > return true; > > > > > > if (len == 2 && name[0] == '.' && name[1] == '.') > > > return true; > > > > > > return false; > > > } > > > > > > Hi Matthew, > > > > > > How do you think? I think the performance influence is very small > > > due to is_dot_or_dotdot() is a such short static inline function. > > It's a very short inline function called on a very hot codepath. > > Often. > > > > I mean it - it's done literally for every pathname component of > > every pathname passed to a syscall. > > OK. I understand. Let us do not use the helper function in fs/namei.c, > just use the following implementation for other callers: > > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > { > if (len >= 1 && unlikely(name[0] == '.')) { And I suggest drop "unlikely" here since files start with prefix '.' (plus specical ".", "..") are not as uncommon as you expected... Thanks, Gao Xiang > if (len < 2 || (len == 2 && name[1] == '.')) > return true; > } > > return false; > } > > Special thanks for Matthew, Darrick, Al and Eric. > If you have any more suggestion, please let me know. > > Thanks, > > Tiezhu Yang >
On Wed, Dec 11, 2019 at 03:17:11PM +0800, Gao Xiang wrote: > > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > > { > > if (len >= 1 && unlikely(name[0] == '.')) { > > > And I suggest drop "unlikely" here since files start with prefix > '.' (plus specical ".", "..") are not as uncommon as you expected... They absolutely are uncommon. Even if you just consider /home/willy/kernel/linux/.git/config, only one of those six path elements starts with a '.'.
Hi Matthew, On Wed, Dec 11, 2019 at 05:40:14AM -0800, Matthew Wilcox wrote: > On Wed, Dec 11, 2019 at 03:17:11PM +0800, Gao Xiang wrote: > > > static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) > > > { > > > if (len >= 1 && unlikely(name[0] == '.')) { > > > > > > And I suggest drop "unlikely" here since files start with prefix > > '.' (plus specical ".", "..") are not as uncommon as you expected... > > They absolutely are uncommon. Even if you just consider > /home/willy/kernel/linux/.git/config, only one of those six path elements > starts with a '.'. Okay, I think it depends on userdata and access patterns. I admit I have no statistics on all those callers. Just considering introducing an inline helper for cleanup, except for lookup_one_len_common() (since it's on an error path), others were all without unlikely() before. Ignore my words if it seems unreasonable or unlikely() is an improvement in this patch and sorry for annoying. Thanks, Gao Xiang
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 3da3707..bb41f5d 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -11,21 +11,11 @@ * This has not yet undergone a rigorous security audit. */ +#include <linux/namei.h> #include <linux/scatterlist.h> #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 * @@ -252,10 +242,9 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, const struct fscrypt_str *iname, struct fscrypt_str *oname) { - const struct qstr qname = FSTR_TO_QSTR(iname); struct fscrypt_digested_name digested_name; - if (fscrypt_is_dot_dotdot(&qname)) { + if (is_dot_or_dotdot(iname->name, iname->len)) { 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_or_dotdot(iname->name, iname->len)) { 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..c3bcbf0 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 @@ -2027,7 +2017,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_or_dotdot(name, name_size)) { 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/f2fs/hash.c b/fs/f2fs/hash.c index 5bc4dcd..ef155c2 100644 --- a/fs/f2fs/hash.c +++ b/fs/f2fs/hash.c @@ -15,6 +15,7 @@ #include <linux/cryptohash.h> #include <linux/pagemap.h> #include <linux/unicode.h> +#include <linux/namei.h> #include "f2fs.h" @@ -82,7 +83,7 @@ static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, if (fname && !fname->disk_name.name) return cpu_to_le32(fname->hash); - if (is_dot_dotdot(name_info)) + if (is_dot_or_dotdot(name, len)) return 0; /* Initialize the default seed for the hash checksum functions */ diff --git a/fs/namei.c b/fs/namei.c index d6c91d1..f3a4439 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2451,10 +2451,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_or_dotdot(name, len)) + return -EACCES; while (len--) { unsigned int c = *(const unsigned char *)name++; diff --git a/include/linux/namei.h b/include/linux/namei.h index 7fe7b87..0fd9315 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -92,4 +92,14 @@ retry_estale(const long error, const unsigned int flags) return error == -ESTALE && !(flags & LOOKUP_REVAL); } +static inline bool is_dot_or_dotdot(const unsigned char *name, size_t len) +{ + if (unlikely(name[0] == '.')) { + if (len == 1 || (len == 2 && name[1] == '.')) + return true; + } + + return false; +} + #endif /* _LINUX_NAMEI_H */
There exists many similar and duplicate codes to check "." and "..", so introduce is_dot_or_dotdot helper to make the code more clean. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- v5: - remove "qname" variable in fscrypt_fname_disk_to_usr() - modify "len < 2" to "len == 1" in is_dot_or_dotdot() v4: - rename is_dot_dotdot() to is_dot_or_dotdot() v3: - use "name" and "len" as arguments instead of qstr - move is_dot_dotdot() to include/linux/namei.h 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 | 17 +++-------------- fs/ecryptfs/crypto.c | 12 +----------- fs/f2fs/f2fs.h | 11 ----------- fs/f2fs/hash.c | 3 ++- fs/namei.c | 6 ++---- include/linux/namei.h | 10 ++++++++++ 6 files changed, 18 insertions(+), 41 deletions(-)