Hiya.
I use smbfs on my x86 Linux 2.6.3 machine to mount filesystems from a
(Debian) Samba 3.0.0beta2 server. This has worked fine with both this
kernel and previous ones for the last few months, but I've just had an
Oops message while trying to open a directory with ROX-Filer. The
filesystem in question is automounted (using autofs4), and this would
have been the first operation upon it after being mounted.
My kernel config is at <http://offog.org/stuff/config-2.6.3>. Here's the
ksymoops output:
ksymoops 2.4.9 on i686 2.6.3. Options used
-v /src/linux-2.6.3/vmlinux (specified)
-k /proc/ksyms (default)
-l /proc/modules (default)
-o /lib/modules/2.6.3/ (default)
-m /src/linux-2.6.3/System.map (specified)
Error (regular_file): read_ksyms stat /proc/ksyms failed
No modules in ksyms, skipping objects
No ksyms, skipping lsmod
Unable to handle kernel NULL pointer dereference at virtual address 00000000
00000000
*pde = 00000000
Oops: 0000 [#1]
CPU: 0
EIP: 0060:[<00000000>] Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00210246
eax: ee433c40 ebx: 00000002 ecx: c5107840 edx: cf84bfa0
esi: c0165560 edi: c1750080 ebp: cf84bf68 esp: cf84bef8
ds: 007b es: 007b ss: 0068
Stack: c01dbbfb c5107840 cf84bfa0 c0165560 cf84bf2c 00000000 00000002 00000004
d4ea7ee4 00000000 eecd0000 c40520c0 d0a0c980 00000000 5e50af11 00000000
00000000 00000000 eecd0000 00000002 00000000 00000000 00000001 00000004
Call Trace:
[<c01dbbfb>] smb_readdir+0x4fb/0x6e0
[<c0165560>] filldir64+0x0/0x130
[<c016524a>] vfs_readdir+0x8a/0x90
[<c0165560>] filldir64+0x0/0x130
[<c01656fd>] sys_getdents64+0x6d/0xa6
[<c0165560>] filldir64+0x0/0x130
[<c010adff>] syscall_call+0x7/0xb
Code: Bad EIP value.
>>EIP; 00000000 Before first symbol
>>eax; ee433c40 <_end+2dc7cad8/3f846e98>
>>ecx; c5107840 <_end+49506d8/3f846e98>
>>edx; cf84bfa0 <_end+f094e38/3f846e98>
>>esi; c0165560 <filldir64+0/130>
>>edi; c1750080 <_end+f98f18/3f846e98>
>>ebp; cf84bf68 <_end+f094e00/3f846e98>
>>esp; cf84bef8 <_end+f094d90/3f846e98>
Trace; c01dbbfb <smb_readdir+4fb/6e0>
Trace; c0165560 <filldir64+0/130>
Trace; c016524a <vfs_readdir+8a/90>
Trace; c0165560 <filldir64+0/130>
Trace; c01656fd <sys_getdents64+6d/a6>
Trace; c0165560 <filldir64+0/130>
Trace; c010adff <syscall_call+7/b>
1 error issued. Results may not be reliable.
And here's the raw Oops:
Unable to handle kernel NULL pointer dereference at virtual address 00000000
printing eip:
00000000
*pde = 00000000
Oops: 0000 [#1]
CPU: 0
EIP: 0060:[<00000000>] Not tainted
EFLAGS: 00210246
EIP is at 0x0
eax: ee433c40 ebx: 00000002 ecx: c5107840 edx: cf84bfa0
esi: c0165560 edi: c1750080 ebp: cf84bf68 esp: cf84bef8
ds: 007b es: 007b ss: 0068
Process ROX-Filer (pid: 19894, threadinfo=cf84a000 task=d0b93320)
Stack: c01dbbfb c5107840 cf84bfa0 c0165560 cf84bf2c 00000000 00000002 00000004
d4ea7ee4 00000000 eecd0000 c40520c0 d0a0c980 00000000 5e50af11 00000000
00000000 00000000 eecd0000 00000002 00000000 00000000 00000001 00000004
Call Trace:
[<c01dbbfb>] smb_readdir+0x4fb/0x6e0
[<c0165560>] filldir64+0x0/0x130
[<c016524a>] vfs_readdir+0x8a/0x90
[<c0165560>] filldir64+0x0/0x130
[<c01656fd>] sys_getdents64+0x6d/0xa6
[<c0165560>] filldir64+0x0/0x130
[<c010adff>] syscall_call+0x7/0xb
Code: Bad EIP value.
If there's any other information that'd be useful, please let me know.
Thanks,
--
Adam Sampson <[email protected]> <http://offog.org/>
On Wed, 10 Mar 2004, Adam Sampson wrote:
>
> I use smbfs on my x86 Linux 2.6.3 machine to mount filesystems from a
> (Debian) Samba 3.0.0beta2 server. This has worked fine with both this
> kernel and previous ones for the last few months, but I've just had an
> Oops message while trying to open a directory with ROX-Filer. The
> filesystem in question is automounted (using autofs4), and this would
> have been the first operation upon it after being mounted.
Hmm.. Looks like an indirect call that jumped through a NULL pointer.
There's a few different indirect calls in "smb_readdir()", so it's a bit
hard to guess which one. It's at the end of the function, but gcc
re-orders code so much (and usually wrong, I have to say), that it's hard
to guess which one it would be.
Three out of four calls are to "readdir()", which is an argument to the
function and should not really reasonably be NULL unless there is a
compiler bug or some _major_ stack corruption going on. So I consider
those to be the less likely causes.
The last one is to "server->ops->readdir()", and it's entirely possible
that that might be NULL. That's reinforced by the data on the stack
which would be the arguments to that function:
c5107840 cf84bfa0 c0165560 cf84bf2c
which makes some amount of sense (arg 2 and 4 are both pointing to the
call-stack, which I think is correct for those arguments if it is that
"->readdir()" call). In contrast, the above would _not_ make sense as the
four first arguments to "filldir" (the third one should be a name length).
So I think you had a NULL pointer for "server->ops->readdir".
As to how something like that could happen, I have absolutely no clue. The
"smb_install_null_ops()" would seem to cause that, but that's all I can
say.
Maybe the "smp_ops_null" thing should be filled in with stuff that always
returns EINVAL or something? Rather than actual NULL pointers that will
oops if they are ever used?
(That structure is actually from Andrew's patch from Zwane Mwaikambo
as a workaround for smb_proc_getattr oops from last summer)
Urban? Andrew? Zwane? I can decode the oopses, but when it comes to smbfs
I'm a retard.
Linus
On Tue, 9 Mar 2004, Linus Torvalds wrote:
> As to how something like that could happen, I have absolutely no clue. The
> "smb_install_null_ops()" would seem to cause that, but that's all I can
> say.
>
> Maybe the "smp_ops_null" thing should be filled in with stuff that always
> returns EINVAL or something? Rather than actual NULL pointers that will
> oops if they are ever used?
The getattr problem was that after the mount smbfs isn't ready to do
anything until it gets a connection from smbmount. Filling smp_ops_null
would give the user an error on whatever it is that it does immediately
after mounting.
smp_ops_null should really make all functions block and wait for the
connection to be created and given to us from smbmount. That would make it
behave more like smbfs in 2.4 does.
I am thinking of something like this for each entry in smp_ops_null.
int whatever_it_is_that_I_am(args)
{
timeleft = wait_event_interruptible_timeout(...)
if (!timeleft || signal_pending(current))
return -EIO;
if (!server->ops->whatever_it_is_that_I_am)
return -EIO;
return server->ops->whatever_it_is_that_I_am(args)
}
I know bugme has bug1671 with a similar oops. If no one else does, I'll
make a patch for Adam and #1671 to test.
I have not been able to reproduce this before, but if it is that readdir
is used before it is ready then some extra delays in smbmount might help.
/Urban
On Wed, 10 Mar 2004, Urban Widmark wrote:
> On Tue, 9 Mar 2004, Linus Torvalds wrote:
>
> > As to how something like that could happen, I have absolutely no clue. The
> > "smb_install_null_ops()" would seem to cause that, but that's all I can
> > say.
> >
> > Maybe the "smp_ops_null" thing should be filled in with stuff that always
> > returns EINVAL or something? Rather than actual NULL pointers that will
> > oops if they are ever used?
I originally didn't fill them all in intentionally, doing so may be best.
> smp_ops_null should really make all functions block and wait for the
> connection to be created and given to us from smbmount. That would make it
> behave more like smbfs in 2.4 does.
>
> I am thinking of something like this for each entry in smp_ops_null.
>
> int whatever_it_is_that_I_am(args)
> {
> timeleft = wait_event_interruptible_timeout(...)
> if (!timeleft || signal_pending(current))
> return -EIO;
> if (!server->ops->whatever_it_is_that_I_am)
> return -EIO;
> return server->ops->whatever_it_is_that_I_am(args)
> }
Thanks Urban, i have posted the following on bugzilla
(http://bugzilla.kernel.org/show_bug.cgi?id=1671) for testing. But,
it appears racy wrt getattr and win9x servers.
Index: linux-2.6.4-rc3/fs/smbfs/proc.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.4-rc3/fs/smbfs/proc.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 proc.c
--- linux-2.6.4-rc3/fs/smbfs/proc.c 10 Mar 2004 01:05:32 -0000 1.1.1.1
+++ linux-2.6.4-rc3/fs/smbfs/proc.c 10 Mar 2004 17:16:22 -0000
@@ -56,6 +56,7 @@ static struct smb_ops smb_ops_os2;
static struct smb_ops smb_ops_win95;
static struct smb_ops smb_ops_winNT;
static struct smb_ops smb_ops_unix;
+static struct smb_ops smb_ops_null;
static void
smb_init_dirent(struct smb_sb_info *server, struct smb_fattr *fattr);
@@ -2794,10 +2795,46 @@ out:
}
static int
+smb_proc_ops_wait(struct smb_sb_info *server)
+{
+ int result;
+ DECLARE_WAIT_QUEUE_HEAD(wq);
+
+ result = wait_event_interruptible_timeout(wq,
+ server->ops != &smb_ops_null, 5*HZ);
+
+ if (!result || signal_pending(current))
+ return -EIO;
+
+ return 0;
+}
+
+static int
smb_proc_getattr_null(struct smb_sb_info *server, struct dentry *dir,
- struct smb_fattr *attr)
+ struct smb_fattr *fattr)
{
- return -EIO;
+ int result;
+
+ if (smb_proc_ops_wait(server) < 0)
+ return -EIO;
+
+ smb_init_dirent(server, fattr);
+ result = server->ops->getattr(server, dir, fattr);
+ smb_finish_dirent(server, fattr);
+
+ return result;
+}
+
+static int
+smb_proc_readdir_null(struct file *filp, void *dirent, filldir_t filldir,
+ struct smb_cache_control *ctl)
+{
+ struct smb_sb_info *server = server_from_dentry(filp->f_dentry);
+
+ if (smb_proc_ops_wait(server) < 0)
+ return -EIO;
+
+ return server->ops->readdir(filp, dirent, filldir, ctl);
}
int
@@ -3431,6 +3467,7 @@ static struct smb_ops smb_ops_unix =
/* Place holder until real ops are in place */
static struct smb_ops smb_ops_null =
{
+ .readdir = smb_proc_readdir_null,
.getattr = smb_proc_getattr_null,
};
On Wed, 10 Mar 2004, Zwane Mwaikambo wrote:
> Thanks Urban, i have posted the following on bugzilla
> (http://bugzilla.kernel.org/show_bug.cgi?id=1671) for testing. But,
> it appears racy wrt getattr and win9x servers.
How about the following to synchronize with smb_newconn()
smb_lock_server(server);
smb_unlock_server(server);
I've already uploaded the new patch on Bugzilla, but i also came across a
smb_dir_cache related oops whilst testing, which i'm debugging.
On Wed, 10 Mar 2004, Zwane Mwaikambo wrote:
> On Wed, 10 Mar 2004, Zwane Mwaikambo wrote:
>
> > Thanks Urban, i have posted the following on bugzilla
> > (http://bugzilla.kernel.org/show_bug.cgi?id=1671) for testing. But,
> > it appears racy wrt getattr and win9x servers.
The 5 second timeout is probably too short. Some bad configs can use a
long time to connect, possibly more. 30?
> How about the following to synchronize with smb_newconn()
>
> smb_lock_server(server);
> smb_unlock_server(server);
Shouldn't "wq" be accessible to both smb_newconn and smb_proc_ops_wait?
I'd put it in the "server" struct and then have smb_newconn() do this
when it is done:
wake_up_interruptible_all(&server->ops_wq);
I don't know enough about wait_queue's to understand why it would work
otherwise. The only thing I can think of is that the condition is true
before it actually waits on anything.
Since install_ops isn't the last thing done in smb_newconn perhaps a
different variable should be used to signal that a new connection is
there. I would suggest using "server->state == CONN_VALID" and then move
that assignment to the end of smb_newconn.
I'm guessing read/write/truncate can't be called before smb_newconn since
they all require a file to be opened, and open needs getattr (or?). But
just to be safe how about adding the code below?
static int
smb_proc_ops_bug(void)
{
BUG();
}
static struct smb_ops smb_ops_null =
{
.readdir = smb_proc_readdir_null,
.getattr = smb_proc_getattr_null,
.read = (void *) smb_proc_ops_bug,
.write = (void *) smb_proc_ops_bug,
.truncate = (void *) smb_proc_ops_bug,
};
If the void* can be avoided by something clever then that is what I really
meant :)
> I've already uploaded the new patch on Bugzilla, but i also came across a
> smb_dir_cache related oops whilst testing, which i'm debugging.
If you are in cleanup mode the following changes should probably be made:
server->rcls replaced by req->rq_rcls
server->err replaced by req->rq_err
and remove the server->{rcls,err} fields.
/Urban
On Wed, 10 Mar 2004, Urban Widmark wrote:
> Shouldn't "wq" be accessible to both smb_newconn and smb_proc_ops_wait?
> I'd put it in the "server" struct and then have smb_newconn() do this
> when it is done:
> wake_up_interruptible_all(&server->ops_wq);
Oops, yes my code was horribly broken, the previous patch will only avoid
the oops since readdir won't be NULL, but is still fundamentally wrong.
> I don't know enough about wait_queue's to understand why it would work
> otherwise. The only thing I can think of is that the condition is true
> before it actually waits on anything.
>
> Since install_ops isn't the last thing done in smb_newconn perhaps a
> different variable should be used to signal that a new connection is
> there. I would suggest using "server->state == CONN_VALID" and then move
> that assignment to the end of smb_newconn.
>
> If you are in cleanup mode the following changes should probably be made:
>
> server->rcls replaced by req->rq_rcls
> server->err replaced by req->rq_err
Sure thing, i'll fix it up.
Thanks,
Zwane
Two patches applied in this order;
patch-smbfs-readdir-oops
fs/smbfs/inode.c | 1 +
fs/smbfs/proc.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/smb_fs_sb.h | 2 +-
3 files changed, 44 insertions(+), 4 deletions(-)
patch-smbfs-rcls-cleanup
fs/smbfs/proc.c | 12 ++++++------
include/linux/smb_fs_sb.h | 3 ---
2 files changed, 6 insertions(+), 9 deletions(-)
I avoided adding the BUG() placeholders, since we will PF# when we hit the
NULL pointer and the call trace should give us a decent idea of where it
is.
Thanks,
Zwane
On Thu, 11 Mar 2004, Zwane Mwaikambo wrote:
> Two patches applied in this order;
>
> patch-smbfs-readdir-oops
> fs/smbfs/inode.c | 1 +
> fs/smbfs/proc.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> include/linux/smb_fs_sb.h | 2 +-
> 3 files changed, 44 insertions(+), 4 deletions(-)
>
> patch-smbfs-rcls-cleanup
> fs/smbfs/proc.c | 12 ++++++------
> include/linux/smb_fs_sb.h | 3 ---
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> I avoided adding the BUG() placeholders, since we will PF# when we hit the
> NULL pointer and the call trace should give us a decent idea of where it
> is.
Not quite there yet.
I have added a sleep(10); in send_fs_socket (smbmount) to give me time to
send other requests to the fs. Here is what I get when I run the mount and
two 'ls' in parallel:
1. smbmount mounts and goes to sleep
2. ls 1 ends up in smb_proc_ops_wait
3. ls 2 ends up in smb_proc_ops_wait
4. smbmount wakes up and passes the socket to smbfs
5a. ls 1 & 2 are woken and lists files
5b. Everyone is waiting for ls 1 (or 2 I don't know) to timeout.
6b. ls 1 lists nothing because of the timeout
7b. ls 2 lists normally
The difference between a and b is that in the first case I wait a few
seconds between the two ls' but in the second I run the within one second
of each other.
Some debug printks give:
a:
smb_proc_getattr_null:
smb_proc_ops_wait: state: 1
smb_proc_getattr_null:
smb_proc_ops_wait: state: 1
(here it waits for smbmount to do the ioctl)
smb_proc_ops_wait: result: 26192
smb_proc_ops_wait: result: 23014
b:
smb_proc_readdir_null:
smb_proc_ops_wait: state: 1
smb_proc_getattr_null:
smb_proc_ops_wait: state: 1
(here it waits for smbmount to do the ioctl)
smb_proc_ops_wait: result: 0
smb_proc_ops_wait: result: 682
The difference must be that in a the inode data for the root inode is not
considered current when the second ls runs, but I don't understand why the
readdir is printed before the getattr.
/Urban
On Fri, 12 Mar 2004, Urban Widmark wrote:
> Not quite there yet.
>
> I have added a sleep(10); in send_fs_socket (smbmount) to give me time to
> send other requests to the fs. Here is what I get when I run the mount and
> two 'ls' in parallel:
Thanks for testing it, i'll have another go at it.
> The difference must be that in a the inode data for the root inode is not
> considered current when the second ls runs, but I don't understand why the
> readdir is printed before the getattr.
I don't understand why to expect the getattr before the readdir, perhaps
you can elaborate for me?
Thanks,
Zwane
On Fri, 12 Mar 2004, Zwane Mwaikambo wrote:
> > The difference must be that in a the inode data for the root inode is not
> > considered current when the second ls runs, but I don't understand why the
> > readdir is printed before the getattr.
>
> I don't understand why to expect the getattr before the readdir, perhaps
> you can elaborate for me?
smb_readdir
smb_revalidate_inode
smb_refresh_inode
smb_proc_getattr
server->ops->getattr
server->ops->readdir
The first ls should find the inode out-of-date (smb_readdir probably isn't
the first call, but that doesn't matter) because it is the first user.
The second ls is run shortly after and should find the inode to be
up-to-date.
I'm not sure it is important at all, it just wasn't what I expected.
/Urban