[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