dcache: trivial comment fix
PATCH 1/1 dcache: trivial comment fix
As it stands this comment is confusing, and not quite grammatical.
--- fs/dcache.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
fs/dcache.c +2 -2
@@ -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. */ /*
Signature · 1 line
1.5.3
Quoted 7 lines · J. Bruce Fields
* 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.
Quoted 28 lines · On Monday September 10, [email protected] wrote:
Quoted 7 lines · J. Bruce Fields
* 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;
Quoted 1 line
... but
target->d_inamecould 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
Quoted 29 lines · Neil Brown
Quoted 28 lines · On Monday September 10, [email protected] wrote:
Quoted 7 lines · J. Bruce Fields
* 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;
Quoted 3 lines
Quoted 1 line
... but
target->d_inamecould 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.
Quoted 8 lines
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 simplememcpy()to copy over
* the new name before we switch.
Quoted 6 lines
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.