Missing patch from stable [3/7]

Missing patch from stable [3/7]

[Willy Tarreau]

Hi,

this patch from mainline seems suitable for -stable, but was not proposed
for inclusion. I think we should include it for next review unless the
author disagrees.

Thanks,
Willy
--

>From 8dc4e37362a5dc910d704d52ac6542bfd49ddc2f Mon Sep 17 00:00:00 2001
From: Miklos Szeredi <mszeredi@suse.cz>
Date: Mon, 12 May 2008 14:02:04 -0700
Subject: ecryptfs: clean up (un)lock_parent

dget(dentry->d_parent) --> dget_parent(dentry)

unlock_parent() is racy and unnecessary.  Replace single caller with
unlock_dir().

There are several other suspect uses of ->d_parent in ecryptfs...

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Michael Halcrow <mhalcrow@us.ibm.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/ecryptfs/inode.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 0a13973..c92cc1c 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -37,17 +37,11 @@ static struct dentry *lock_parent(struct dentry *dentry)
 {
 	struct dentry *dir;
 
-	dir = dget(dentry->d_parent);
+	dir = dget_parent(dentry);
 	mutex_lock_nested(&(dir->d_inode->i_mutex), I_MUTEX_PARENT);
 	return dir;
 }
 
-static void unlock_parent(struct dentry *dentry)
-{
-	mutex_unlock(&(dentry->d_parent->d_inode->i_mutex));
-	dput(dentry->d_parent);
-}
-
 static void unlock_dir(struct dentry *dir)
 {
 	mutex_unlock(&dir->d_inode->i_mutex);
@@ -426,8 +420,9 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
 	int rc = 0;
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);
+	struct dentry *lower_dir_dentry;
 
-	lock_parent(lower_dentry);
+	lower_dir_dentry = lock_parent(lower_dentry);
 	rc = vfs_unlink(lower_dir_inode, lower_dentry);
 	if (rc) {
 		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
@@ -439,7 +434,7 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
 	dentry->d_inode->i_ctime = dir->i_ctime;
 	d_drop(dentry);
 out_unlock:
-	unlock_parent(lower_dentry);
+	unlock_dir(lower_dir_dentry);
 	return rc;
 }
 
-- 
1.5.3.8

Re: Missing patch from stable [3/7]

[Miklos Szeredi]

On Sun, 2008-06-08 at 10:59 +0200, Willy Tarreau wrote:
> this patch from mainline seems suitable for -stable,

Willy,

Thanks for picking up these ecryptfs patches ...but they hardly meet
_any_ of the -stable rules.  In particular:


 - It must be obviously correct and tested.

It's obvious, but I don't know if it's been tested (or even looked at by
the maintainer).

 - It cannot be bigger than 100 lines, with context.

Check.

 - It must fix only one thing.

No, it's a small fix as well as a cleanup.

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

No, it doesn't seem to bother anybody.

 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.

Not critical at all.

 - No "theoretical race condition" issues, unless an explanation of how the
   race can be exploited is also provided.

It's theoretical, I have no idea how it's exploitable, if at all.

 - It cannot contain any "trivial" fixes in it (spelling changes,
   whitespace cleanups, etc).

Check.

 - It must follow the Documentation/SubmittingPatches rules.

Check.

 - It or an equivalent fix must already exist in Linus' tree.  Quote the
   respective commit ID in Linus' tree in your patch submission to -stable.

Check.


Total: 4/9, not a very convincing score :)

Thanks,
Miklos


> 
> Thanks,
> Willy
> --
> 
> From 8dc4e37362a5dc910d704d52ac6542bfd49ddc2f Mon Sep 17 00:00:00 2001
> From: Miklos Szeredi <mszeredi@suse.cz>
> Date: Mon, 12 May 2008 14:02:04 -0700
> Subject: ecryptfs: clean up (un)lock_parent
> 
> dget(dentry->d_parent) --> dget_parent(dentry)
> 
> unlock_parent() is racy and unnecessary.  Replace single caller with
> unlock_dir().
> 
> There are several other suspect uses of ->d_parent in ecryptfs...
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> Cc: Michael Halcrow <mhalcrow@us.ibm.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  fs/ecryptfs/inode.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 0a13973..c92cc1c 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -37,17 +37,11 @@ static struct dentry *lock_parent(struct dentry *dentry)
>  {
>  	struct dentry *dir;
>  
> -	dir = dget(dentry->d_parent);
> +	dir = dget_parent(dentry);
>  	mutex_lock_nested(&(dir->d_inode->i_mutex), I_MUTEX_PARENT);
>  	return dir;
>  }
>  
> -static void unlock_parent(struct dentry *dentry)
> -{
> -	mutex_unlock(&(dentry->d_parent->d_inode->i_mutex));
> -	dput(dentry->d_parent);
> -}
> -
>  static void unlock_dir(struct dentry *dir)
>  {
>  	mutex_unlock(&dir->d_inode->i_mutex);
> @@ -426,8 +420,9 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
>  	int rc = 0;
>  	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
>  	struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);
> +	struct dentry *lower_dir_dentry;
>  
> -	lock_parent(lower_dentry);
> +	lower_dir_dentry = lock_parent(lower_dentry);
>  	rc = vfs_unlink(lower_dir_inode, lower_dentry);
>  	if (rc) {
>  		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
> @@ -439,7 +434,7 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
>  	dentry->d_inode->i_ctime = dir->i_ctime;
>  	d_drop(dentry);
>  out_unlock:
> -	unlock_parent(lower_dentry);
> +	unlock_dir(lower_dir_dentry);
>  	return rc;
>  }
>  

Re: Missing patch from stable [3/7]

[Willy Tarreau]

On Sun, Jun 08, 2008 at 01:11:02PM +0200, Miklos Szeredi wrote:
> 
> On Sun, 2008-06-08 at 10:59 +0200, Willy Tarreau wrote:
> > this patch from mainline seems suitable for -stable,
> 
> Willy,
> 
> Thanks for picking up these ecryptfs patches ...but they hardly meet
> _any_ of the -stable rules.  In particular:
> 
> 
>  - It must be obviously correct and tested.
> 
> It's obvious, but I don't know if it's been tested (or even looked at by
> the maintainer).

well, some of them (eg: Cyrill's fix for missed mutex_unlock) are obviously
needed.

>  - It cannot be bigger than 100 lines, with context.
> 
> Check.
> 
>  - It must fix only one thing.
> 
> No, it's a small fix as well as a cleanup.
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> No, it doesn't seem to bother anybody.

Generally speaking, everything which relates to locking is hard to diagnose.
I've been hit for years by locking bugs in the NFS client on old 2.4, and
they hit me at most once in a week.

>  - It must fix a problem that causes a build error (but not for things
>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>    security issue, or some "oh, that's not good" issue.  In short, something
>    critical.
> 
> Not critical at all.

OK

>  - No "theoretical race condition" issues, unless an explanation of how the
>    race can be exploited is also provided.
> 
> It's theoretical, I have no idea how it's exploitable, if at all.

OK, but being exploitable is one thing, randomly failing is another one. If
you think it cannot cause trouble, then OK.

>  - It cannot contain any "trivial" fixes in it (spelling changes,
>    whitespace cleanups, etc).
> 
> Check.
> 
>  - It must follow the Documentation/SubmittingPatches rules.
> 
> Check.
> 
>  - It or an equivalent fix must already exist in Linus' tree.  Quote the
>    respective commit ID in Linus' tree in your patch submission to -stable.
> 
> Check.
> 
> 
> Total: 4/9, not a very convincing score :)

Well, you know the implications of leaving these known bugs open more than
me. If you think some of them are really not needed, I'm fine with this,
but some definitely fix real bugs (judging by the code and the comments).

Thanks,
Willy

Re: Missing patch from stable [3/7]

[Miklos Szeredi]

On Sun, 2008-06-08 at 14:29 +0200, Willy Tarreau wrote:

> Well, you know the implications of leaving these known bugs open more than
> me. If you think some of them are really not needed, I'm fine with this,
> but some definitely fix real bugs (judging by the code and the comments).

Yeah.  Perhaps there should also be a rule, that -stable patches for
EXPERIMENTAL stuff (like ecryptfs) are automatically rejected.  And then
they wouldn't increase the workload for the people collecting/reviewing
the stable series.

Thanks,
Miklos

Re: Missing patch from stable [3/7]

[Willy Tarreau]

On Sun, Jun 08, 2008 at 04:50:22PM +0200, Miklos Szeredi wrote:
> On Sun, 2008-06-08 at 14:29 +0200, Willy Tarreau wrote:
> 
> > Well, you know the implications of leaving these known bugs open more than
> > me. If you think some of them are really not needed, I'm fine with this,
> > but some definitely fix real bugs (judging by the code and the comments).
> 
> Yeah.  Perhaps there should also be a rule, that -stable patches for
> EXPERIMENTAL stuff (like ecryptfs) are automatically rejected.  And then
> they wouldn't increase the workload for the people collecting/reviewing
> the stable series.

That would indeed help. However, half of the kernel is tagged EXPERIMENTAL.
I don't even know if there is one distro which ships a working kernel with
experimental turned off :-/

Willy

Re: [stable] Missing patch from stable [3/7]

[Chris Wright]

* Miklos Szeredi (mszeredi@suse.cz) wrote:
> On Sun, 2008-06-08 at 14:29 +0200, Willy Tarreau wrote:
> > Well, you know the implications of leaving these known bugs open more than
> > me. If you think some of them are really not needed, I'm fine with this,
> > but some definitely fix real bugs (judging by the code and the comments).
> 
> Yeah.  Perhaps there should also be a rule, that -stable patches for
> EXPERIMENTAL stuff (like ecryptfs) are automatically rejected.  And then
> they wouldn't increase the workload for the people collecting/reviewing
> the stable series.

Both users and distros enable code marked EXPERIMENTAL.  It's not
that uncommon for code to get stuck in EXPERIMENTAL long after it's in
general use.

thanks,
-chris

Re: [stable] Missing patch from stable [3/7]

[Miklos Szeredi]

On Mon, 2008-06-09 at 10:12 -0700, Chris Wright wrote:
> * Miklos Szeredi (mszeredi@suse.cz) wrote:
> > On Sun, 2008-06-08 at 14:29 +0200, Willy Tarreau wrote:
> > > Well, you know the implications of leaving these known bugs open more than
> > > me. If you think some of them are really not needed, I'm fine with this,
> > > but some definitely fix real bugs (judging by the code and the comments).
> > 
> > Yeah.  Perhaps there should also be a rule, that -stable patches for
> > EXPERIMENTAL stuff (like ecryptfs) are automatically rejected.  And then
> > they wouldn't increase the workload for the people collecting/reviewing
> > the stable series.
> 
> Both users and distros enable code marked EXPERIMENTAL.  It's not
> that uncommon for code to get stuck in EXPERIMENTAL long after it's in
> general use.

OK, it's been voted down.

While I do think that -stable updates for experimental stuff are not as
important as for mainstream stuff, it's also true that it does little
harm to include these fixes.

The only harm is that lots of these non-critical patches drown out the
really important ones, which get less attention this way.

Oh, well...

Miklos

Re: Missing patch from stable [3/7]

[Michael Halcrow]

On Sun, Jun 08, 2008 at 01:11:02PM +0200, Miklos Szeredi wrote:
> On Sun, 2008-06-08 at 10:59 +0200, Willy Tarreau wrote:
> Thanks for picking up these ecryptfs patches ...but they hardly meet
> _any_ of the -stable rules.  In particular:
> 
>  - It must be obviously correct and tested.
> 
> It's obvious, but I don't know if it's been tested (or even looked
> at by the maintainer).

I see no obvious problems with these patches. I have tested with the
four eCryptfs patches in this thread applied to 2.6.25.5.

>  - It must fix a problem that causes a build error (but not for
>    things marked CONFIG_BROKEN), an oops, a hang, data corruption, a
>    real security issue, or some "oh, that's not good" issue.  In
>    short, something critical.
> 
> Not critical at all.

I am under the impression that bugs that could result in hangs or data
corruption are, by definition, critical.

>  - No "theoretical race condition" issues, unless an explanation of
>    how the race can be exploited is also provided.
> 
> It's theoretical, I have no idea how it's exploitable, if at all.

Exploits can be subtle, so I would be more comfortable with
eliminating known race conditions.

While I agree that "EXPERIMENTAL" features should be less likely to
receive updates in -stable, the experimental status of a feature
should not categorically exclude fixes from getting in. The
experimental status should just be one of the factors used in deciding
whether it is worth the effort.

Mike

> > --
> > 
> > From 8dc4e37362a5dc910d704d52ac6542bfd49ddc2f Mon Sep 17 00:00:00 2001
> > From: Miklos Szeredi <mszeredi@suse.cz>
> > Date: Mon, 12 May 2008 14:02:04 -0700
> > Subject: ecryptfs: clean up (un)lock_parent
> > 
> > dget(dentry->d_parent) --> dget_parent(dentry)
> > 
> > unlock_parent() is racy and unnecessary.  Replace single caller with
> > unlock_dir().
> > 
> > There are several other suspect uses of ->d_parent in ecryptfs...
> > 
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > Cc: Michael Halcrow <mhalcrow@us.ibm.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > ---
> >  fs/ecryptfs/inode.c |   13 ++++---------
> >  1 files changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> > index 0a13973..c92cc1c 100644
> > --- a/fs/ecryptfs/inode.c
> > +++ b/fs/ecryptfs/inode.c
> > @@ -37,17 +37,11 @@ static struct dentry *lock_parent(struct dentry *dentry)
> >  {
> >  	struct dentry *dir;
> >  
> > -	dir = dget(dentry->d_parent);
> > +	dir = dget_parent(dentry);
> >  	mutex_lock_nested(&(dir->d_inode->i_mutex), I_MUTEX_PARENT);
> >  	return dir;
> >  }
> >  
> > -static void unlock_parent(struct dentry *dentry)
> > -{
> > -	mutex_unlock(&(dentry->d_parent->d_inode->i_mutex));
> > -	dput(dentry->d_parent);
> > -}
> > -
> >  static void unlock_dir(struct dentry *dir)
> >  {
> >  	mutex_unlock(&dir->d_inode->i_mutex);
> > @@ -426,8 +420,9 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
> >  	int rc = 0;
> >  	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> >  	struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);
> > +	struct dentry *lower_dir_dentry;
> >  
> > -	lock_parent(lower_dentry);
> > +	lower_dir_dentry = lock_parent(lower_dentry);
> >  	rc = vfs_unlink(lower_dir_inode, lower_dentry);
> >  	if (rc) {
> >  		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
> > @@ -439,7 +434,7 @@ static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
> >  	dentry->d_inode->i_ctime = dir->i_ctime;
> >  	d_drop(dentry);
> >  out_unlock:
> > -	unlock_parent(lower_dentry);
> > +	unlock_dir(lower_dir_dentry);
> >  	return rc;
> >  }
> >  
>