| Message ID | 20220622194603.102655-4-krisman@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support negative dentries on case-insensitive directories | expand |
On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote: > Introduce a dentry revalidation helper to be used by case-insensitive > filesystems to check if it is safe to reuse a negative dentry. > > A negative dentry is safe to be reused on a case-insensitive lookup if > it was created during a case-insensitive lookup and this is not a lookup > that will instantiate a dentry. If this is a creation lookup, we also > need to make sure the name matches sensitively the name under lookup in > order to assure the name preserving semantics. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> Reviewed-by: Theodore Ts'o <tytso@mit.edu>
On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote: > +static inline int generic_ci_d_revalidate(struct dentry *dentry, > + const struct qstr *name, > + unsigned int flags) > +{ > + int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET); > + > + if (d_is_negative(dentry)) { > + const struct dentry *parent = READ_ONCE(dentry->d_parent); > + const struct inode *dir = READ_ONCE(parent->d_inode); > + > + if (dir && needs_casefold(dir)) { > + if (!d_is_casefold_lookup(dentry)) > + return 0; In which conditions does that happen? > + if (is_creation && > + (dentry->d_name.len != name->len || > + memcmp(dentry->d_name.name, name->name, name->len))) > + return 0; > + } > + } > + return 1; > +} Analysis of stability of ->d_name, please. It's *probably* safe, but the details are subtle and IMO should be accompanied by several asserts. E.g. "we never get LOOKUP_CREATE in op->intent without O_CREAT in op->open_flag for such and such reasons, and we verify that in such and such place"... A part of that would be "the call in lookup_dcache() can only get there with non-zero flags when coming from __lookup_hash(), and that has parent locked, stabilizing the name; the same goes for the call in __lookup_slow(), with the only call chain with possibly non-zero flags is through lookup_slow(), where we have the parent locked". However, lookup_fast() and lookup_open() have the flags come from nd->flags, and LOOKUP_CREATE can be found there in several areas. I _think_ we are guaranteed the parent locked in all such call chains, but that is definitely worth at least a comment.
Al Viro <viro@zeniv.linux.org.uk> writes: > On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote: > >> +static inline int generic_ci_d_revalidate(struct dentry *dentry, >> + const struct qstr *name, >> + unsigned int flags) >> +{ >> + int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET); >> + >> + if (d_is_negative(dentry)) { >> + const struct dentry *parent = READ_ONCE(dentry->d_parent); >> + const struct inode *dir = READ_ONCE(parent->d_inode); >> + >> + if (dir && needs_casefold(dir)) { >> + if (!d_is_casefold_lookup(dentry)) >> + return 0; > > In which conditions does that happen? Hi Al, This can happen right after a case-sensitive directory is converted to case-insensitive. A previous case-sensitive lookup could have left a negative dentry in the dcache that we need to reject, because it doesn't have the same assurance of absence of all-variation of names as a negative dentry created during a case-insensitive lookup. >> + if (is_creation && >> + (dentry->d_name.len != name->len || >> + memcmp(dentry->d_name.name, name->name, name->len))) >> + return 0; >> + } >> + } >> + return 1; >> +} > > Analysis of stability of ->d_name, please. It's *probably* safe, but > the details are subtle and IMO should be accompanied by several asserts. > E.g. "we never get LOOKUP_CREATE in op->intent without O_CREAT in op->open_flag > for such and such reasons, and we verify that in such and such place"... > > A part of that would be "the call in lookup_dcache() can only get there > with non-zero flags when coming from __lookup_hash(), and that has parent locked, > stabilizing the name; the same goes for the call in __lookup_slow(), with the > only call chain with possibly non-zero flags is through lookup_slow(), where we > have the parent locked". However, lookup_fast() and lookup_open() have the > flags come from nd->flags, and LOOKUP_CREATE can be found there in several areas. > I _think_ we are guaranteed the parent locked in all such call chains, but that > is definitely worth at least a comment. Thanks for the example of the analysis what you are looking for here. That will help me quite a bit. I wrote this code a while ago and I don't recall the exact details. I will go through the code again and send a new version with the detailed analysis.
diff --git a/fs/libfs.c b/fs/libfs.c index 31b0ddf01c31..de43f3f585f1 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1450,9 +1450,33 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str) return 0; } +static inline int generic_ci_d_revalidate(struct dentry *dentry, + const struct qstr *name, + unsigned int flags) +{ + int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET); + + if (d_is_negative(dentry)) { + const struct dentry *parent = READ_ONCE(dentry->d_parent); + const struct inode *dir = READ_ONCE(parent->d_inode); + + if (dir && needs_casefold(dir)) { + if (!d_is_casefold_lookup(dentry)) + return 0; + + if (is_creation && + (dentry->d_name.len != name->len || + memcmp(dentry->d_name.name, name->name, name->len))) + return 0; + } + } + return 1; +} + static const struct dentry_operations generic_ci_dentry_ops = { .d_hash = generic_ci_d_hash, .d_compare = generic_ci_d_compare, + .d_revalidate_name = generic_ci_d_revalidate, }; #endif
Introduce a dentry revalidation helper to be used by case-insensitive filesystems to check if it is safe to reuse a negative dentry. A negative dentry is safe to be reused on a case-insensitive lookup if it was created during a case-insensitive lookup and this is not a lookup that will instantiate a dentry. If this is a creation lookup, we also need to make sure the name matches sensitively the name under lookup in order to assure the name preserving semantics. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/libfs.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)