[1/3] fs/dcache: Add d_clear_dir_neg_dentries()
diff mbox series

Message ID 20210328144356.12866-2-andrealmeid@collabora.com
State New, archived
Headers show
Series
  • fs: Fix dangling dentries on casefold directories
Related show

Commit Message

André Almeida March 28, 2021, 2:43 p.m. UTC
For directories with negative dentries that are becoming case-insensitive
dirs, we need to remove all those negative dentries, otherwise they will
become dangling dentries. During the creation of a new file, if a d_hash
collision happens and the names match in a case-insensitive way, the name
of the file will be the name defined at the negative dentry, that may be
different from the specified by the user. To prevent this from
happening, we need to remove all dentries in a directory. Given that the
directory must be empty before we call this function we are sure that
all dentries there will be negative.

Create a function to remove all negative dentries from a directory, to
be used as explained above by filesystems that support case-insensitive
lookups.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 fs/dcache.c            | 27 +++++++++++++++++++++++++++
 include/linux/dcache.h |  1 +
 2 files changed, 28 insertions(+)

Comments

Matthew Wilcox March 28, 2021, 3:07 p.m. UTC | #1
On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:
> +/**
> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
> + * @dir: Directory to clear negative dentries
> + *
> + * For directories with negative dentries that are becoming case-insensitive
> + * dirs, we need to remove all those negative dentries, otherwise they will
> + * become dangling dentries. During the creation of a new file, if a d_hash
> + * collision happens and the names match in a case-insensitive, the name of
> + * the file will be the name defined at the negative dentry, that can be
> + * different from the specified by the user. To prevent this from happening, we
> + * need to remove all dentries in a directory. Given that the directory must be
> + * empty before we call this function we are sure that all dentries there will
> + * be negative.
> + */

This is quite the landmine of a function.  It _assumes_ that the directory
is empty, and clears all dentries in it.

> +void d_clear_dir_neg_dentries(struct inode *dir)
> +{
> +	struct dentry *alias, *dentry;
> +
> +	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
> +		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
> +			d_drop(dentry);
> +			dput(dentry);
> +		}

I would be happier if it included a check for negativity.  d_is_negative()
or maybe this newfangled d_really_is_negative() (i haven't stayed up
to speed on the precise difference between the two)

> +	}
> +}
> +EXPORT_SYMBOL(d_clear_dir_neg_dentries);

I'd rather see this _GPL for such an internal thing.
André Almeida March 28, 2021, 3:49 p.m. UTC | #2
Às 12:07 de 28/03/21, Matthew Wilcox escreveu:
> On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:
>> +/**
>> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
>> + * @dir: Directory to clear negative dentries
>> + *
>> + * For directories with negative dentries that are becoming case-insensitive
>> + * dirs, we need to remove all those negative dentries, otherwise they will
>> + * become dangling dentries. During the creation of a new file, if a d_hash
>> + * collision happens and the names match in a case-insensitive, the name of
>> + * the file will be the name defined at the negative dentry, that can be
>> + * different from the specified by the user. To prevent this from happening, we
>> + * need to remove all dentries in a directory. Given that the directory must be
>> + * empty before we call this function we are sure that all dentries there will
>> + * be negative.
>> + */
> 
> This is quite the landmine of a function.  It _assumes_ that the directory
> is empty, and clears all dentries in it.
> 
>> +void d_clear_dir_neg_dentries(struct inode *dir)
>> +{
>> +	struct dentry *alias, *dentry;
>> +
>> +	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
>> +		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
>> +			d_drop(dentry);
>> +			dput(dentry);
>> +		}
> 
> I would be happier if it included a check for negativity.  d_is_negative()
> or maybe this newfangled d_really_is_negative() (i haven't stayed up
> to speed on the precise difference between the two)
> 

Makes sense. And given that this only makes sense if the directory is 
empty, if it founds a non-negative dentry, it should return some error 
right?

>> +	}
>> +}
>> +EXPORT_SYMBOL(d_clear_dir_neg_dentries);
> 
> I'd rather see this _GPL for such an internal thing.
>
Al Viro March 28, 2021, 5:39 p.m. UTC | #3
On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:

> +/**
> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
> + * @dir: Directory to clear negative dentries
> + *
> + * For directories with negative dentries that are becoming case-insensitive
> + * dirs, we need to remove all those negative dentries, otherwise they will
> + * become dangling dentries. During the creation of a new file, if a d_hash
> + * collision happens and the names match in a case-insensitive, the name of
> + * the file will be the name defined at the negative dentry, that can be
> + * different from the specified by the user. To prevent this from happening, we
> + * need to remove all dentries in a directory. Given that the directory must be
> + * empty before we call this function we are sure that all dentries there will
> + * be negative.
> + */
> +void d_clear_dir_neg_dentries(struct inode *dir)
> +{
> +	struct dentry *alias, *dentry;
> +
> +	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
> +		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
> +			d_drop(dentry);
> +			dput(dentry);
> +		}
> +	}
> +}

That makes no sense whatsoever.
	1) directories can never have more than one alias
	2) what the hell are you doing to refcounts on those children?
Eric Biggers March 30, 2021, 1:48 a.m. UTC | #4
On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:
> For directories with negative dentries that are becoming case-insensitive
> dirs, we need to remove all those negative dentries, otherwise they will
> become dangling dentries. During the creation of a new file, if a d_hash
> collision happens and the names match in a case-insensitive way, the name
> of the file will be the name defined at the negative dentry, that may be
> different from the specified by the user. To prevent this from
> happening, we need to remove all dentries in a directory. Given that the
> directory must be empty before we call this function we are sure that
> all dentries there will be negative.
> 
> Create a function to remove all negative dentries from a directory, to
> be used as explained above by filesystems that support case-insensitive
> lookups.
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  fs/dcache.c            | 27 +++++++++++++++++++++++++++
>  include/linux/dcache.h |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7d24ff7eb206..fafb3016d6fd 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1723,6 +1723,33 @@ void d_invalidate(struct dentry *dentry)
>  }
>  EXPORT_SYMBOL(d_invalidate);
>  
> +/**
> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
> + * @dir: Directory to clear negative dentries
> + *
> + * For directories with negative dentries that are becoming case-insensitive
> + * dirs, we need to remove all those negative dentries, otherwise they will
> + * become dangling dentries. During the creation of a new file, if a d_hash
> + * collision happens and the names match in a case-insensitive, the name of
> + * the file will be the name defined at the negative dentry, that can be
> + * different from the specified by the user. To prevent this from happening, we
> + * need to remove all dentries in a directory. Given that the directory must be
> + * empty before we call this function we are sure that all dentries there will
> + * be negative.
> + */
> +void d_clear_dir_neg_dentries(struct inode *dir)
> +{
> +	struct dentry *alias, *dentry;
> +
> +	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
> +		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
> +			d_drop(dentry);
> +			dput(dentry);
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(d_clear_dir_neg_dentries);

As Al already pointed out, this doesn't work as intended, for a number of
different reasons.

Did you consider just using shrink_dcache_parent()?  That already does what you
are trying to do here, I think.

The harder part (which I don't think you've considered) is how to ensure that
all negative dentries really get invalidated even if there are lookups of them
happening concurrently.  Concurrent lookups can take temporary references to the
negative dentries, preventing them from being invalidated.

- Eric
André Almeida March 30, 2021, 12:54 p.m. UTC | #5
Hi Eric,

Às 22:48 de 29/03/21, Eric Biggers escreveu:
> On Sun, Mar 28, 2021 at 11:43:54AM -0300, André Almeida wrote:
>> For directories with negative dentries that are becoming case-insensitive
>> dirs, we need to remove all those negative dentries, otherwise they will
>> become dangling dentries. During the creation of a new file, if a d_hash
>> collision happens and the names match in a case-insensitive way, the name
>> of the file will be the name defined at the negative dentry, that may be
>> different from the specified by the user. To prevent this from
>> happening, we need to remove all dentries in a directory. Given that the
>> directory must be empty before we call this function we are sure that
>> all dentries there will be negative.
>>
>> Create a function to remove all negative dentries from a directory, to
>> be used as explained above by filesystems that support case-insensitive
>> lookups.
>>
>> Signed-off-by: André Almeida <andrealmeid@collabora.com>
>> ---
>>   fs/dcache.c            | 27 +++++++++++++++++++++++++++
>>   include/linux/dcache.h |  1 +
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 7d24ff7eb206..fafb3016d6fd 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1723,6 +1723,33 @@ void d_invalidate(struct dentry *dentry)
>>   }
>>   EXPORT_SYMBOL(d_invalidate);
>>   
>> +/**
>> + * d_clear_dir_neg_dentries - Remove negative dentries in an inode
>> + * @dir: Directory to clear negative dentries
>> + *
>> + * For directories with negative dentries that are becoming case-insensitive
>> + * dirs, we need to remove all those negative dentries, otherwise they will
>> + * become dangling dentries. During the creation of a new file, if a d_hash
>> + * collision happens and the names match in a case-insensitive, the name of
>> + * the file will be the name defined at the negative dentry, that can be
>> + * different from the specified by the user. To prevent this from happening, we
>> + * need to remove all dentries in a directory. Given that the directory must be
>> + * empty before we call this function we are sure that all dentries there will
>> + * be negative.
>> + */
>> +void d_clear_dir_neg_dentries(struct inode *dir)
>> +{
>> +	struct dentry *alias, *dentry;
>> +
>> +	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
>> +		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
>> +			d_drop(dentry);
>> +			dput(dentry);
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL(d_clear_dir_neg_dentries);
> 
> As Al already pointed out, this doesn't work as intended, for a number of
> different reasons.
> 
> Did you consider just using shrink_dcache_parent()?  That already does what you
> are trying to do here, I think.

When I wrote this patch, I didn't know it, but after Al Viro comments I 
get back to the code and found it, and it seems do do what I intend 
indeed, and my test is happy as well.

> 
> The harder part (which I don't think you've considered) is how to ensure that
> all negative dentries really get invalidated even if there are lookups of them
> happening concurrently.  Concurrent lookups can take temporary references to the
> negative dentries, preventing them from being invalidated.
> 

I didn't consider that, thanks for the feedback. So this means that 
those lookups will increase the refcount of the dentry, and it will only 
get really invalidated when refcount reaches 0? Or do would I need to 
call d_invalidate() again, until I succeed?

> - Eric
>

Patch
diff mbox series

diff --git a/fs/dcache.c b/fs/dcache.c
index 7d24ff7eb206..fafb3016d6fd 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1723,6 +1723,33 @@  void d_invalidate(struct dentry *dentry)
 }
 EXPORT_SYMBOL(d_invalidate);
 
+/**
+ * d_clear_dir_neg_dentries - Remove negative dentries in an inode
+ * @dir: Directory to clear negative dentries
+ *
+ * For directories with negative dentries that are becoming case-insensitive
+ * dirs, we need to remove all those negative dentries, otherwise they will
+ * become dangling dentries. During the creation of a new file, if a d_hash
+ * collision happens and the names match in a case-insensitive, the name of
+ * the file will be the name defined at the negative dentry, that can be
+ * different from the specified by the user. To prevent this from happening, we
+ * need to remove all dentries in a directory. Given that the directory must be
+ * empty before we call this function we are sure that all dentries there will
+ * be negative.
+ */
+void d_clear_dir_neg_dentries(struct inode *dir)
+{
+	struct dentry *alias, *dentry;
+
+	hlist_for_each_entry(alias, &dir->i_dentry, d_u.d_alias) {
+		list_for_each_entry(dentry, &alias->d_subdirs, d_child) {
+			d_drop(dentry);
+			dput(dentry);
+		}
+	}
+}
+EXPORT_SYMBOL(d_clear_dir_neg_dentries);
+
 /**
  * __d_alloc	-	allocate a dcache entry
  * @sb: filesystem it will belong to
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index c1e48014106f..c43cd0be077f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -250,6 +250,7 @@  extern void shrink_dcache_sb(struct super_block *);
 extern void shrink_dcache_parent(struct dentry *);
 extern void shrink_dcache_for_umount(struct super_block *);
 extern void d_invalidate(struct dentry *);
+extern void d_clear_dir_neg_dentries(struct inode *);
 
 /* only used at mount-time */
 extern struct dentry * d_make_root(struct inode *);