Hi David,
romfs_lookup worked in 2.6.24.2, but always fails in
linux-2.6.25-rc3-git3. fs/romfs/inode.c is the same in at least
2.6.25-rc3 through 2.6.25-rc3-git4 and the latest sources from git, so
these versions almost certainly have the same problem.
The bug appears to be from a well meaning but botched attempt
to eliminate a goto from romfs_lookup. Previously, a goto statement
was used to skip over "inode = NULL;" when the lookup succeeded. In
the 2.6.25-rc3 version, inode is set to NULL even when an inode was
found, so the result is the the lookup always appears to fail.
The attached patch fixes the problem while still eliminating
the goto. The patch adds one line and replaces one line. It only
looks big because I've set the number of context lines to 10 for
better readability. I have tested it in on a romfs initial ramdisk
which on which I had experienced the problem.
If this patch looks OK to you, can you please submit it
upstream?
P.S. romfs_lookup casts a valid pointer to an int and then back again
with res = PTR_ERR(inode);...return ERR_PTR(res). This may break on
arhictectures where sizeof(int) < sizeof(pointer). If I want to submit
a subsequent fix for this, are you the person I should the patch to?
Adam Richter
On Mon, 3 Mar 2008 09:36:20 -0800 "Adam J. Richter" <[email protected]> wrote:
> Hi David,
>
> romfs_lookup worked in 2.6.24.2, but always fails in
> linux-2.6.25-rc3-git3. fs/romfs/inode.c is the same in at least
> 2.6.25-rc3 through 2.6.25-rc3-git4 and the latest sources from git, so
> these versions almost certainly have the same problem.
>
> The bug appears to be from a well meaning but botched attempt
> to eliminate a goto from romfs_lookup. Previously, a goto statement
> was used to skip over "inode = NULL;" when the lookup succeeded. In
> the 2.6.25-rc3 version, inode is set to NULL even when an inode was
> found, so the result is the the lookup always appears to fail.
>
> The attached patch fixes the problem while still eliminating
> the goto. The patch adds one line and replaces one line. It only
> looks big because I've set the number of context lines to 10 for
> better readability. I have tested it in on a romfs initial ramdisk
> which on which I had experienced the problem.
>
> If this patch looks OK to you, can you please submit it
> upstream?
Thanks.
> P.S. romfs_lookup casts a valid pointer to an int and then back again
> with res = PTR_ERR(inode);...return ERR_PTR(res). This may break on
> arhictectures where sizeof(int) < sizeof(pointer). If I want to submit
> a subsequent fix for this, are you the person I should the patch to?
I can take care of it.
Please remember the signed-off-by: red tape.
Adam J. Richter <[email protected]> wrote:
> romfs_lookup worked in 2.6.24.2, but always fails in
> linux-2.6.25-rc3-git3. fs/romfs/inode.c is the same in at least
> 2.6.25-rc3 through 2.6.25-rc3-git4 and the latest sources from git, so
> these versions almost certainly have the same problem.
>
> The bug appears to be from a well meaning but botched attempt
> to eliminate a goto from romfs_lookup. Previously, a goto statement
> was used to skip over "inode = NULL;" when the lookup succeeded. In
> the 2.6.25-rc3 version, inode is set to NULL even when an inode was
> found, so the result is the the lookup always appears to fail.
No, the problem is that I've changed the sense of the condition of the
if-statement, but failed to notice:-(
> The attached patch fixes the problem while still eliminating
> the goto. The patch adds one line and replaces one line. It only
> looks big because I've set the number of context lines to 10 for
> better readability. I have tested it in on a romfs initial ramdisk
> which on which I had experienced the problem.
Looking at the code, the negative/error case is in the main flow, and was
jumped around by the positive case. Your patch sets up the negative case in
advance as the default, thus meaning the positive case handles the negative
case too. I like it.
> If this patch looks OK to you, can you please submit it
> upstream?
Perhaps. Can you look over the attached patch as an alternative, please?
> P.S. romfs_lookup casts a valid pointer to an int and then back again
> with res = PTR_ERR(inode);...return ERR_PTR(res). This may break on
> arhictectures where sizeof(int) < sizeof(pointer).
This is not true. Note the if-statement in the following:
inode = romfs_iget(dir->i_sb, offset);
if (IS_ERR(inode)) {
res = PTR_ERR(inode);
goto out;
}
Inside the if-statement, the pointer 'inode' is actually an integer in the
range -4095 to -1, and, as such, actually represents an error code. Casting it
to an int and back will be fine. A cleaner way to do it would be to avoid the
cast entirely, especially as it may avoid a couple of instructions on a 64-bit
platform (32/64-bit conversion).
David
---
ROMFS: Fix up an error in iget removal
From: David Howells <[email protected]>
Fix up an error in iget removal in which romfs_lookup() making a successful
call to romfs_iget() continues through the negative/error handling (previously
the successful case jumped around the negative/error handling case):
(1) inode is initialised to NULL at the top of the function, eliminating the
need for specific negative-inode handling. This means the positive
success handling now flows straight through.
(2) Rename the labels to be clearer about what they mean.
Also make romfs_lookup()'s result variable of type long so as to avoid
32-bit/64-bit conversions with PTR_ERR() and friends.
Signed-off-by: David Howells <[email protected]>
---
fs/romfs/inode.c | 30 +++++++++++-------------------
1 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/fs/romfs/inode.c b/fs/romfs/inode.c
index 00b6f0a..3f13d49 100644
--- a/fs/romfs/inode.c
+++ b/fs/romfs/inode.c
@@ -340,8 +340,9 @@ static struct dentry *
romfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
{
unsigned long offset, maxoff;
- int fslen, res;
- struct inode *inode;
+ long res;
+ int fslen;
+ struct inode *inode = NULL;
char fsname[ROMFS_MAXFN]; /* XXX dynamic? */
struct romfs_inode ri;
const char *name; /* got from dentry */
@@ -351,7 +352,7 @@ romfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
offset = dir->i_ino & ROMFH_MASK;
lock_kernel();
if (romfs_copyfrom(dir, &ri, offset, ROMFH_SIZE) <= 0)
- goto out;
+ goto error;
maxoff = romfs_maxsize(dir->i_sb);
offset = be32_to_cpu(ri.spec) & ROMFH_MASK;
@@ -364,9 +365,9 @@ romfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
for(;;) {
if (!offset || offset >= maxoff)
- goto out0;
+ goto success; /* negative success */
if (romfs_copyfrom(dir, &ri, offset, ROMFH_SIZE) <= 0)
- goto out;
+ goto error;
/* try to match the first 16 bytes of name */
fslen = romfs_strnlen(dir, offset+ROMFH_SIZE, ROMFH_SIZE);
@@ -397,23 +398,14 @@ romfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
inode = romfs_iget(dir->i_sb, offset);
if (IS_ERR(inode)) {
res = PTR_ERR(inode);
- goto out;
+ goto error;
}
- /*
- * it's a bit funky, _lookup needs to return an error code
- * (negative) or a NULL, both as a dentry. ENOENT should not
- * be returned, instead we need to create a negative dentry by
- * d_add(dentry, NULL); and return 0 as no error.
- * (Although as I see, it only matters on writable file
- * systems).
- */
-
-out0: inode = NULL;
+success:
+ d_add(dentry, inode);
res = 0;
- d_add (dentry, inode);
-
-out: unlock_kernel();
+error:
+ unlock_kernel();
return ERR_PTR(res);
}
On Tue, Mar 04, 2008 at 01:26:21AM -0800, Andrew Morton wrote:
> On Mon, 3 Mar 2008 09:36:20 -0800 "Adam J. Richter" <[email protected]> wrote:
[...]
> > P.S. romfs_lookup casts a valid pointer to an int and then back again
> > with res = PTR_ERR(inode);...return ERR_PTR(res). This may break on
> > arhictectures where sizeof(int) < sizeof(pointer). If I want to submit
> > a subsequent fix for this, are you the person I should the patch to?
Oops, please ignore my incorrect "P.S.". As David Howells has
explained, the PTR_ERR(inode) call is only done when inode is known to
contain an error code, so it should be safe on all architectures.
Sorry for the false alarm about that part.
Adam
On Tue, Mar 04, 2008 at 02:32:43PM +0000, David Howells wrote:
> [...] Can you look over the attached patch as an alternative, please?
Your new goto labels "error" and "success" (instead of "out"
and "out0") clarify the code enough to justify your removal of the
comment block, so that's all good. My only question is why you've
changed res from int to long, given that you've corrected my misguided
"P.S.":
> Adam J. Richter <[email protected]> wrote:
[...]
> > P.S. romfs_lookup casts a valid pointer to an int and then back again
> > with res = PTR_ERR(inode);...return ERR_PTR(res). This may break on
> > arhictectures where sizeof(int) < sizeof(pointer).
>
> This is not true. [...]
You're right. My mistake. Sorry for causing confusion. But,
based on what you've just said, it doesn't sound like you need to
change res from int to long. Is there some other issue I'm missing?
Anyhow, if you want to do these changes together in a single
commit or separately, I'm fine with it either way.
Adam Richter
Adam J. Richter <[email protected]> wrote:
> My only question is why you've changed res from int to long, given that
> you've corrected my misguided "P.S.":
Because if you compile for a 64-bit system, you may find extra instructions
emitted, at the very least in ERR_PTR(res) where it turns a 32-bit int into a
64-bit pointer.
David