All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: "André Almeida" <andrealmeid@collabora.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <chao@kernel.org>,
	krisman@collabora.com, kernel@collabora.com,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	Daniel Rosenberg <drosen@google.com>,
	Chao Yu <yuchao0@huawei.com>
Subject: Re: [PATCH 1/3] fs/dcache: Add d_clear_dir_neg_dentries()
Date: Sun, 28 Mar 2021 16:07:15 +0100	[thread overview]
Message-ID: <20210328150715.GA33249@casper.infradead.org> (raw)
In-Reply-To: <20210328144356.12866-2-andrealmeid@collabora.com>

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.

WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: "André Almeida" <andrealmeid@collabora.com>
Cc: linux-ext4@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
	Daniel Rosenberg <drosen@google.com>,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	kernel@collabora.com, krisman@collabora.com
Subject: Re: [f2fs-dev] [PATCH 1/3] fs/dcache: Add d_clear_dir_neg_dentries()
Date: Sun, 28 Mar 2021 16:07:15 +0100	[thread overview]
Message-ID: <20210328150715.GA33249@casper.infradead.org> (raw)
In-Reply-To: <20210328144356.12866-2-andrealmeid@collabora.com>

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.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

  reply	other threads:[~2021-03-28 15:09 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-28 14:43 [PATCH 0/3] fs: Fix dangling dentries on casefold directories André Almeida
2021-03-28 14:43 ` [f2fs-dev] " André Almeida
2021-03-28 14:43 ` [PATCH 1/3] fs/dcache: Add d_clear_dir_neg_dentries() André Almeida
2021-03-28 14:43   ` [f2fs-dev] " André Almeida
2021-03-28 15:07   ` Matthew Wilcox [this message]
2021-03-28 15:07     ` Matthew Wilcox
2021-03-28 15:49     ` André Almeida
2021-03-28 15:49       ` [f2fs-dev] " André Almeida
2021-03-28 17:39   ` Al Viro
2021-03-28 17:39     ` [f2fs-dev] " Al Viro
2021-03-30  1:48   ` Eric Biggers
2021-03-30  1:48     ` [f2fs-dev] " Eric Biggers
2021-03-30 12:54     ` André Almeida
2021-03-30 12:54       ` [f2fs-dev] " André Almeida
2021-03-28 14:43 ` [PATCH 2/3] ext4: Prevent dangling dentries on casefold directories André Almeida
2021-03-28 14:43   ` [f2fs-dev] " André Almeida
2021-03-28 14:43 ` [PATCH 3/3] f2fs: " André Almeida
2021-03-28 14:43   ` [f2fs-dev] " André Almeida

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210328150715.GA33249@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=andrealmeid@collabora.com \
    --cc=chao@kernel.org \
    --cc=drosen@google.com \
    --cc=jaegeuk@kernel.org \
    --cc=kernel@collabora.com \
    --cc=krisman@collabora.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yuchao0@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.