[PATCH] dcache: trivial comment fix

[PATCH] dcache: trivial comment fix

[J. Bruce Fields]

As it stands this comment is confusing, and not quite grammatical.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/dcache.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 678d39d..2bacdf6 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1514,8 +1514,8 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
  * This forceful removal will result in ugly /proc output if
  * somebody holds a file open that got deleted due to a rename.
  * We could be nicer about the deleted file, and let it show
- * up under the name it got deleted rather than the name that
- * deleted it.
+ * up under the name it had before it was deleted rather than
+ * under the original name of the file that was moved on top of it.
  */
  
 /*
-- 
1.5.3

Re: [PATCH] dcache: trivial comment fix

[J. Bruce Fields]

On Mon, Sep 10, 2007 at 02:46:32PM -0400, J. Bruce Fields wrote:
>   * This forceful removal will result in ugly /proc output if
>   * somebody holds a file open that got deleted due to a rename.
>   * We could be nicer about the deleted file, and let it show
> - * up under the name it got deleted rather than the name that
> - * deleted it.
> + * up under the name it had before it was deleted rather than
> + * under the original name of the file that was moved on top of it.

By the way, on further examination of the code it doesn't actually do
what's described in the case where the target name is large and the
moved-from name is small.  Instead, it reports random garbage (usually
part of a name left over from some other dentry?) as far as I can tell:

from switch_names():


	if (dname_external(target)) {
                if (dname_external(dentry)) {
			...
                } else {
                        /*
                         * dentry:internal, target:external.  Steal target's
                         * storage and make target internal.
                         */
                        dentry->d_name.name = target->d_name.name;
                        target->d_name.name = target->d_iname;

... but target->d_iname could have anything in it, right?

--b.

Re: [PATCH] dcache: trivial comment fix

[Neil Brown]

On Monday September 10, bfields@fieldses.org wrote:
> On Mon, Sep 10, 2007 at 02:46:32PM -0400, J. Bruce Fields wrote:
> >   * This forceful removal will result in ugly /proc output if
> >   * somebody holds a file open that got deleted due to a rename.
> >   * We could be nicer about the deleted file, and let it show
> > - * up under the name it got deleted rather than the name that
> > - * deleted it.
> > + * up under the name it had before it was deleted rather than
> > + * under the original name of the file that was moved on top of it.
> 
> By the way, on further examination of the code it doesn't actually do
> what's described in the case where the target name is large and the
> moved-from name is small.  Instead, it reports random garbage (usually
> part of a name left over from some other dentry?) as far as I can tell:
> 
> from switch_names():
> 
> 
> 	if (dname_external(target)) {
>                 if (dname_external(dentry)) {
> 			...
>                 } else {
>                         /*
>                          * dentry:internal, target:external.  Steal target's
>                          * storage and make target internal.
>                          */
>                         dentry->d_name.name = target->d_name.name;
>                         target->d_name.name = target->d_iname;
> 
> ... but target->d_iname could have anything in it, right?

Right, but not relevant.
The name "switch_names" is somewhat misleading.  It is really
"copyname" or similar.  From the comment at the top:

 * When switching names, the actual string doesn't strictly have to
 * be preserved in the target - because we're dropping the target
 * anyway. As such, we can just do a simple memcpy() to copy over
 * the new name before we switch.

so the apparent name of 'target' after the 'swap' is not important.

The purpose of the assignment
                         target->d_name.name = target->d_iname;
is to make "dname_external(target)" false, that making "target
internal" as the comment says.

static inline int dname_external(struct dentry *dentry)
{
	return dentry->d_name.name != dentry->d_iname;
}

This could possibly be made a little more clear....

NeilBrown

Re: [PATCH] dcache: trivial comment fix

[J. Bruce Fields]

On Tue, Sep 11, 2007 at 07:33:43PM +0200, Neil Brown wrote:
> On Monday September 10, bfields@fieldses.org wrote:
> > On Mon, Sep 10, 2007 at 02:46:32PM -0400, J. Bruce Fields wrote:
> > >   * This forceful removal will result in ugly /proc output if
> > >   * somebody holds a file open that got deleted due to a rename.
> > >   * We could be nicer about the deleted file, and let it show
> > > - * up under the name it got deleted rather than the name that
> > > - * deleted it.
> > > + * up under the name it had before it was deleted rather than
> > > + * under the original name of the file that was moved on top of it.
> > 
> > By the way, on further examination of the code it doesn't actually do
> > what's described in the case where the target name is large and the
> > moved-from name is small.  Instead, it reports random garbage (usually
> > part of a name left over from some other dentry?) as far as I can tell:
> > 
> > from switch_names():
> > 
> > 
> > 	if (dname_external(target)) {
> >                 if (dname_external(dentry)) {
> > 			...
> >                 } else {
> >                         /*
> >                          * dentry:internal, target:external.  Steal target's
> >                          * storage and make target internal.
> >                          */
> >                         dentry->d_name.name = target->d_name.name;
> >                         target->d_name.name = target->d_iname;
> > 
> > ... but target->d_iname could have anything in it, right?
> 
> Right, but not relevant.

The effect of it is that the name reported in /proc/<pid>/fd/<fd> is
random garbage if you're holding the target file open.  In quick tests,
I found that

	touch abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
	tail -f abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
	touch foo
	mv foo abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
	readlink /proc/<pid>/fd/<fd>

prints the initial portion of some other random name (often, not always,
"foo").

In theory I think that could disclose a little uninitialized kernel
memory, couldn't it?  I don't know if there's any practical way that
could be exploited.

> The name "switch_names" is somewhat misleading.  It is really
> "copyname" or similar.  From the comment at the top:
> 
>  * When switching names, the actual string doesn't strictly have to
>  * be preserved in the target - because we're dropping the target
>  * anyway. As such, we can just do a simple memcpy() to copy over
>  * the new name before we switch.
> 
> so the apparent name of 'target' after the 'swap' is not important.
> 
> The purpose of the assignment
>                          target->d_name.name = target->d_iname;
> is to make "dname_external(target)" false, that making "target
> internal" as the comment says.

Right.  But it looks like the contents of the buffer target->d_iname
also need to be initialized in this case--I suppose somebody just didn't
want to perform a memcpy they thought was pointless--so the name
reported in /proc is undefined.

--b.

[PATCH] dcache: trivial comment fix

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

As it stands this comment is confusing, and not quite grammatical.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/dcache.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 7da0cf5..5663a31 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1514,8 +1514,8 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
  * This forceful removal will result in ugly /proc output if
  * somebody holds a file open that got deleted due to a rename.
  * We could be nicer about the deleted file, and let it show
- * up under the name it got deleted rather than the name that
- * deleted it.
+ * up under the name it had before it was deleted rather than
+ * the original name of the file that was moved on top of it.
  */
  
 /*
-- 
1.5.3.4.208.gc990

[PATCH] dcache: don't expose uninitialized memory in /proc/<pid>/fd/<fd>

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

Well, it's not especially important that target->d_iname get the
contents of dentry->d_iname, but it's important that it get initialized
with *something*, otherwise we're just exposing some random piece of
memory to anyone who reads the link at /proc/<pid>/fd/<fd> for the
deleted file, when it's still held open by someone.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/dcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

(Am I missing something?  I've also run a test program that copies a
short (<36 character) name ontop of a long (>=36 character) name and see
that the first time I run it, without this patch, I get unpredicatable
results out of /proc/<pid>/fd/<fd>.)

diff --git a/fs/dcache.c b/fs/dcache.c
index 5663a31..24252fc 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1483,6 +1483,8 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
 			 * dentry:internal, target:external.  Steal target's
 			 * storage and make target internal.
 			 */
+			memcpy(target->d_iname, dentry->d_name.name,
+					dentry->d_name.len + 1);
 			dentry->d_name.name = target->d_name.name;
 			target->d_name.name = target->d_iname;
 		}
-- 
1.5.3.4.208.gc990

Re: [PATCH] dcache: don't expose uninitialized memory in /proc/<pid>/fd/<fd>

[Andrew Morton]

On Tue, 16 Oct 2007 15:35:57 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> Well, it's not especially important that target->d_iname get the
> contents of dentry->d_iname, but it's important that it get initialized
> with *something*, otherwise we're just exposing some random piece of
> memory to anyone who reads the link at /proc/<pid>/fd/<fd> for the
> deleted file, when it's still held open by someone.
> 

hm, that was tricky.

> ---
>  fs/dcache.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> (Am I missing something?  I've also run a test program that copies a
> short (<36 character) name ontop of a long (>=36 character) name and see
> that the first time I run it, without this patch, I get unpredicatable
> results out of /proc/<pid>/fd/<fd>.)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 5663a31..24252fc 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1483,6 +1483,8 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
>  			 * dentry:internal, target:external.  Steal target's
>  			 * storage and make target internal.
>  			 */
> +			memcpy(target->d_iname, dentry->d_name.name,
> +					dentry->d_name.len + 1);
>  			dentry->d_name.name = target->d_name.name;
>  			target->d_name.name = target->d_iname;
>  		}

Or we could just stick a \0 in there.

Or perhaps we should set it to "(deleted file)"?

Re: [PATCH] dcache: don't expose uninitialized memory in /proc/<pid>/fd/<fd>

[J. Bruce Fields]

Sorry for the delayed response:

On Wed, Oct 17, 2007 at 03:32:01PM -0700, Andrew Morton wrote:
> On Tue, 16 Oct 2007 15:35:57 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > Well, it's not especially important that target->d_iname get the
> > contents of dentry->d_iname, but it's important that it get initialized
> > with *something*, otherwise we're just exposing some random piece of
> > memory to anyone who reads the link at /proc/<pid>/fd/<fd> for the
> > deleted file, when it's still held open by someone.
> > 
> 
> hm, that was tricky.
> 
> > ---
> >  fs/dcache.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > (Am I missing something?  I've also run a test program that copies a
> > short (<36 character) name ontop of a long (>=36 character) name and see
> > that the first time I run it, without this patch, I get unpredicatable
> > results out of /proc/<pid>/fd/<fd>.)
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 5663a31..24252fc 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1483,6 +1483,8 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
> >  			 * dentry:internal, target:external.  Steal target's
> >  			 * storage and make target internal.
> >  			 */
> > +			memcpy(target->d_iname, dentry->d_name.name,
> > +					dentry->d_name.len + 1);
> >  			dentry->d_name.name = target->d_name.name;
> >  			target->d_name.name = target->d_iname;
> >  		}
> 
> Or we could just stick a \0 in there.

The memcpy() makes the behavior agree with the code comments, and with
what the kernel normally otherwise does.  But, yeah, just making it a
null string would probably be reasonable too.

> Or perhaps we should set it to "(deleted file)"?

Looks like __d_path already adds a (deleted) for us, so that'de be
redundant.

--b.