Patchwork vfs: avoid hang caused by attempting to rmdir an invalid file system

login
register
mail settings
Submitter Theodore Ts'o
Date May 28, 2012, 5:33 p.m.
Message ID <1338226422-8845-1-git-send-email-tytso@mit.edu>
Download mbox | patch
Permalink /patch/161631/
State Not Applicable
Headers show

Comments

Theodore Ts'o - May 28, 2012, 5:33 p.m.
If we rmdir a directory which is a hard link to '.', we will deadlock
trying to grab the directory's i_mutex.  Check for this condition and
return EINVAL, which is what we return if the user attempts to rmdir
"/foo/bar/."

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/namei.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
Greg KH - May 29, 2012, 8:21 a.m.
On Mon, May 28, 2012 at 01:33:42PM -0400, Theodore Ts'o wrote:
> If we rmdir a directory which is a hard link to '.', we will deadlock
> trying to grab the directory's i_mutex.  Check for this condition and
> return EINVAL, which is what we return if the user attempts to rmdir
> "/foo/bar/."
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/namei.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Any reason why you didn't also tag this for the stable kernel releases?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bernd Schubert - May 29, 2012, 11:25 a.m.
On 05/28/2012 07:33 PM, Theodore Ts'o wrote:
> If we rmdir a directory which is a hard link to '.', we will deadlock
> trying to grab the directory's i_mutex.  Check for this condition and
> return EINVAL, which is what we return if the user attempts to rmdir
> "/foo/bar/."
>
> Signed-off-by: "Theodore Ts'o"<tytso@mit.edu>
> ---
>   fs/namei.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0062dd1..081f872 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2774,6 +2774,17 @@ static long do_rmdir(int dfd, const char __user *pathname)
>   		error = -ENOENT;
>   		goto exit3;
>   	}
> +	if (nd.path.dentry->d_inode == dentry->d_inode) {

Shouldn't this be tagged as unlikely()?

> +		/*
> +		 * Corrupt file system where there is a symlink to
> +		 * '.'; treat it as if we are trying to rmdir '.'
> +		 *
> +		 * XXX Should we call into the low-level file system
> +		 * to request that the file system be marked corrupt?
> +		 */
> +		error = -EINVAL;
> +		goto exit3;
> +	}
>   	error = mnt_want_write(nd.path.mnt);
>   	if (error)
>   		goto exit3;

Thanks,
Bernd
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 29, 2012, 12:18 p.m.
On Tue, May 29, 2012 at 05:21:44PM +0900, Greg KH wrote:
> On Mon, May 28, 2012 at 01:33:42PM -0400, Theodore Ts'o wrote:
> > If we rmdir a directory which is a hard link to '.', we will deadlock
> > trying to grab the directory's i_mutex.  Check for this condition and
> > return EINVAL, which is what we return if the user attempts to rmdir
> > "/foo/bar/."
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> > ---
> >  fs/namei.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> Any reason why you didn't also tag this for the stable kernel releases?

Because I was sending it out for comment; I wanted to get Al's opinion
how to handle whether we needed to inform the low-level file system
about the corruption.  I should have added an RFC to the subject line,
sorry.

Andrea's solution of detecting the corruption is lookup seems like the
right answer, so at this point, I'll remove the XXX comment, and add
the unlikely() annotation to the if statement, and resubmit with a Cc:
stable.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 0062dd1..081f872 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2774,6 +2774,17 @@  static long do_rmdir(int dfd, const char __user *pathname)
 		error = -ENOENT;
 		goto exit3;
 	}
+	if (nd.path.dentry->d_inode == dentry->d_inode) {
+		/*
+		 * Corrupt file system where there is a symlink to
+		 * '.'; treat it as if we are trying to rmdir '.'
+		 *
+		 * XXX Should we call into the low-level file system
+		 * to request that the file system be marked corrupt?
+		 */
+		error = -EINVAL;
+		goto exit3;
+	}
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto exit3;