Opened 10 years ago

Closed 10 years ago

#3124 closed bug (fixed)

ls /dev causes a page fault

Reported by: apprentice Owned by: axeld
Priority: normal Milestone: R1
Component: System/Kernel Version: R1/pre-alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: x86

Description

When reading the contents of /dev in any way the kernel crashes. I attached a (poor quality) image of the stack crawl to this ticket.

Attachments (34)

stackcrawl1.jpg (468.5 KB) - added by apprentice 10 years ago.
stack crawl
syslog (197.2 KB) - added by apprentice 10 years ago.
listimage (21.4 KB) - added by apprentice 10 years ago.
syslog.2 (423.2 KB) - added by apprentice 10 years ago.
syslog-with-sanity-check
sys-tail.jpg (129.2 KB) - added by apprentice 10 years ago.
sys | tail -5
fix_dirent-line-numbers.diff (2.3 KB) - added by Adek336 10 years ago.
line numbers tracing in fix_dirent()
fix_dirent-diff.jpg (191.9 KB) - added by apprentice 10 years ago.
d_reclen.diff (2.4 KB) - added by Adek336 10 years ago.
trace file names; sanity check against buffer overflow
d_reclen-diff.jpg (364.0 KB) - added by apprentice 10 years ago.
devfs_create_vnode.diff (3.2 KB) - added by Adek336 10 years ago.
trace vnodes created in devfs. print name of offending direntry
fix_dirent-trac.jpg (291.3 KB) - added by apprentice 10 years ago.
devfs_create_vnode1.jpg (367.7 KB) - added by apprentice 10 years ago.
devfs_create_vnode2.jpg (352.0 KB) - added by apprentice 10 years ago.
devfs_create_vnode3.jpg (343.0 KB) - added by apprentice 10 years ago.
panic_at_vnode_name.diff (4.2 KB) - added by Adek336 10 years ago.
bt.jpg (343.5 KB) - added by apprentice 10 years ago.
trac-republish1.jpg (410.6 KB) - added by apprentice 10 years ago.
trac-republish2.jpg (418.9 KB) - added by apprentice 10 years ago.
trac-republish3.jpg (467.1 KB) - added by apprentice 10 years ago.
trac0-90.jpg (365.8 KB) - added by apprentice 10 years ago.
trac0-90-2.jpg (347.0 KB) - added by apprentice 10 years ago.
fbsd_compat.diff (5.1 KB) - added by Adek336 10 years ago.
sc-fbsd_compat.jpg (343.1 KB) - added by apprentice 10 years ago.
trac-fbsd_compat.jpg (434.0 KB) - added by apprentice 10 years ago.
fbsd_compat.2.diff (11.6 KB) - added by Adek336 10 years ago.
fbsd_compat-2-trac.jpg (329.3 KB) - added by apprentice 10 years ago.
reproduce.diff (1.8 KB) - added by Adek336 10 years ago.
patch.1.diff (1.4 KB) - added by Adek336 10 years ago.
reproduce-diff.jpg (386.2 KB) - added by apprentice 10 years ago.
patch.2.diff (1.4 KB) - added by Adek336 10 years ago.
patch.3.vfs.diff (578 bytes) - added by Adek336 10 years ago.
patch.3.vfs.2.diff (578 bytes) - added by Adek336 10 years ago.
patch.3.fbdscompat.diff (682 bytes) - added by Adek336 10 years ago.
patch.4.fbsdcompat.diff (2.8 KB) - added by Adek336 10 years ago.

Change History (103)

Changed 10 years ago by apprentice

Attachment: stackcrawl1.jpg added

stack crawl

comment:1 Changed 10 years ago by Adek336

What does listdev say ?

comment:2 Changed 10 years ago by apprentice

->listdev

device Multimedia controller (Multimedia audio controller) [4|1|0]

vendor 1106: VIA Technologies, Inc. device 3059: VT8233/A/8235/8237 AC97 Audio Controller

device Mass storage controller (IDE interface) [1|1|8a]

vendor 1106: VIA Technologies, Inc. device 0571: VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE

device Bridge (ISA bridge) [6|1|0]

vendor 1106: VIA Technologies, Inc. device 3177: VT8235 ISA Bridge

device Serial bus controller (USB Controller, EHCI) [c|3|20]

vendor 1106: VIA Technologies, Inc. device 3104: USB 2.0

device Serial bus controller (USB Controller, UHCI) [c|3|0]

vendor 1106: VIA Technologies, Inc. device 3038: VT82xxxxx UHCI USB 1.1 Controller

device Serial bus controller (USB Controller, UHCI) [c|3|0]

vendor 1106: VIA Technologies, Inc. device 3038: VT82xxxxx UHCI USB 1.1 Controller

device Serial bus controller (USB Controller, UHCI) [c|3|0]

vendor 1106: VIA Technologies, Inc. device 3038: VT82xxxxx UHCI USB 1.1 Controller

device Network controller (Ethernet controller) [2|0|0]

vendor 10b7: 3Com Corporation device 9001: 3c900 10Mbps Combo [Boomerang]

device Display controller (VGA compatible controller, VGA controller) [3|0|0]

vendor 10de: nVidia Corporation device 0322: NV34 [GeForce FX 5200]

device Bridge (PCI bridge, Normal decode) [6|4|0]

vendor 1106: VIA Technologies, Inc. device b099: VT8366/A/7 [Apollo KT266/A/333 AGP]

device Bridge (Host bridge) [6|0|0]

vendor 1106: VIA Technologies, Inc. device 3099: VT8366/A/7 [Apollo KT266/A/333]


Seems that according to listdev the device is loaded and working. If I have time tomorrow I'll remove the card from the mainboard and try to read the contents of the /dev directory again. Can anyone tell me that when the hardware is found but the device driver fails to initialize itself, does it still create the device node. It shouldn't because the device isn't working anyway (Duh!). Maybe it just thinks the device is live and tries to use some structure uninitialized resulting into a null pointer exception that crashes the system. Just poking thin ice, since I don't really know what the kernel does when one tries to access the /dev filesystem...

comment:3 Changed 10 years ago by umccullough

Actually, listdev just queries the pci bus to get that info. It doesn't suggest that a driver has been loaded for the devices yet.

comment:4 Changed 10 years ago by Adek336

Could you attach the syslog, please? Does ls /dev/net, ls /dev/audio, ls /dev/disk work? Does dd if=/dev/zero of=/dev/null work?

If you start up with the Safe Mode Console (press space at boot time, choose Safe Mode IIRC), can you do ls /dev?

Changed 10 years ago by apprentice

Attachment: syslog added

comment:5 Changed 10 years ago by apprentice

I Attached the syslog, but haven't had the time to test any commands. Somebody suggested that the bug might be in Haiku's fbsd layer and said that my 3com boomerang nic that doesn't initialize the hardware properly might be related(I posted a separate ticket on this but don't remember the ticket number and this is a duplicate syslog) to the kernel crash.

comment:6 Changed 10 years ago by apprentice

I booted into safe mode as suggested and disabled the bios part also. ls /dev returned the contents of the /dev directory and I could cd to every directory without a hitch, so far so good. For a final stress test I ran 'find .' under /dev. Kernel crashed again and the stack trace looked the same as the earlier (beefdead -- read error). None of the commands worked without safe mode. Maybe the problem is in my mainboard's bios (msi kt3 ultra2).

comment:7 Changed 10 years ago by anevilyak

The BIOS shouldn't actually be involved when looking at /dev, it's most probably a buggy driver or something along those lines. When you ran 'find .', was that in safe mode or normal? What might be interesting is the list of kernel images when you're not in safe mode (listimage on the kernel's team ID will give you this).

comment:8 Changed 10 years ago by apprentice

It crashed in safe mode when running find. I will check the listimage later. I haven't yet tried the dd command in safe mode either. It is also interesting that I can cd into any directory that /dev contains, I just can't read the contents of the directory. ls /dev/s[TAB] for example crashes the kernel immediately. Also I can mount my linux partitions from the gui just fine.

comment:9 Changed 10 years ago by apprentice

I just booted into haiku in normal mode. dd if=/dev/zero of=/dev/null worked! ls /dev/zero and ls /dev/null took ages to complete but they worked also. Again ls /dev crashed. It must be some specific device file in /dev that cannot be stat()d. Can someone tell me what devices are under /dev when haiku boots normally, but not in safe mode, since in safe mode ls /dev works. I could then try to ls every file there and maybe find what device node is buggy.

Changed 10 years ago by apprentice

Attachment: listimage added

comment:10 Changed 10 years ago by Adek336

ls /dev is the same under safe and normal mode

~> ls /dev
audio  console  dprintf  graphics  keyboard  misc  null  ptmx    tt   urandom
bus    disk     dvb      input     midi      net   pt    random  tty  zero

but some are files (as opposed to directories):

console, dprintf, keyboard, null, ptmx, random, tty, random, zero

perhaps you can ls or stat them. If that works, these are more files for ls / stat

/dev/pt/p0
/dev/misc/test/1
/dev/misc/poke
/dev/graphics/vesa

Perhaps deleting /boot/beos/add-ons/kernel/drivers/bin/3com would help?

comment:11 Changed 10 years ago by apprentice

Thanks, I'll try that

comment:12 Changed 10 years ago by apprentice

Getting nowhere...

I did a separate ls on every file/directory you listed and they all worked. The ls /dev/dir returned the directory contents of every directory. So the /dev works but I just have to know exactly what device file I'm using since ls /dev always crashes.

BTW, ran into another bug (maybe this is a known issue already):

When running in safe mode and continuosly booting and crashing the system the filesystem got corrupted. The process using all of the cpu time was the syslog daemon. so I tried to cd to /var/log, but couldn't. ls shows that /var/log exists. but it isn't really there since I can't cd to it (I thought that journaling filesystems don't have these kind of problems). Well I've heard of stories of Linux crashing due to power failure, after which XFS and ReiserFS filesystems were so corrupted that they couldn't be recovered and mkfs was the only solution. So is there a fsck.befs anywhere?

I also deleted the 3com driver, but it didn't help. I just don't know how to debug this problem any further. I've tried everything, but it just keeps crashing.

comment:13 Changed 10 years ago by Adek336

Thanks for the testing!

If you'd care for a little coding:

fix_dirent() is static in src/system/kernel/fs/vfs.cpp and invoked only from dir_read(), so, out of the top of my head, one would like to add a sanity check like

in dir_read:
+                       if ( ((unsigned)vnode) && 0xffff0000 == 0xbeaf0000)
+                               panic("dir_read: vnode is beafdead");
			error = fix_dirent(vnode, buffer, ioContext, &length);

but a stack corruption might be happening after entering fix_dirent so another thing useful'd be

in dir_read:
+                       dprintf("dir_read: enter fix_dirent, ioContext=%p, "
+                               "vnode=%p, cookie=%p, buffer=%p, bufferSize=0x%x, _count=%p, i=0x%x\n",
+                                   ioContext, vnode, cookie, buffer, (unsigned)bufferSize, _count, i);
+                       if ( ((unsigned)vnode) && 0xffff0000 == 0xbeaf0000)
+                               panic("dir_read: vnode is beafdead");
			error = fix_dirent(vnode, buffer, ioContext, &length);
+                       dprintf("dir_read: leave fix_dirent\n");

Changed 10 years ago by apprentice

Attachment: syslog.2 added

syslog-with-sanity-check

comment:14 Changed 10 years ago by apprentice

I applied your sanity check and added the syslog. This is strange. I booted into safe mode and run 'find /dev' and it started to flood my screen with the device files, but it didn't crash... it went into an infinite loop like there was a symlink from somewhere under /dev/tt back to /dev. 'cd /dev'; 'find .' again crashed in safe mode.

comment:15 Changed 10 years ago by apprentice

I forgot to mention the most important thing. The stack trace is exactly same as before, so when entering into fix_dirent() it seems that the vnode is valid because it passes the sanity check...

comment:16 Changed 10 years ago by Adek336

Actually the sanity check is a most basic one and very weak. The interesting part are the last messages written to the log; however, these don't have the chance to be written back to the disk, as the kernel crashes. The interesting part of the log is what you get when you type "syslog|tail 5" into the debugger.

Also, after you crash, can you continue through by entering "continue", or by entering "suspend" and then "continue" into the debugger? Perhaps by doing so a couple of times? Does that make the panic message change?

comment:17 Changed 10 years ago by apprentice

Yes and no. suspend/continue restores the system to a semi-usable state. I can use it like nothing happened as long as I don't touch the crashed "ls" thread. Kernel debugger says that it terminated the ls thread, but it doesn't since running threads still shows the offending process. Shutting the OS cleanly is through 'shutdown -r' that throws me in kernel debugging land from where I can run reboot. The stack trace doesn't seem to be altered in anyway by suspend/continue.

comment:18 Changed 10 years ago by Adek336

What does "sys|ta 5" tell you after a crash?

Changed 10 years ago by apprentice

Attachment: sys-tail.jpg added

sys | tail -5

comment:19 Changed 10 years ago by apprentice

Added the sys|tail -5

comment:20 Changed 10 years ago by Adek336

Thanks for the note!

It would be interesting to see how many lines of fix_dirent() are executed before the crash.

To avoid slowing down the system with the syslog being written to disk, it's better to use ktrace_printf.

It has to be enabled at build time: create a directory build/user_config_headers, copy the file build/config_headers/tracing_config.h to build/user_config_headers and change the line

#  define ENABLE_TRACING 0

to

#  define ENABLE_TRACING 1

. Then you can apply fix_dirent-line-numbers.diff (against src/system/kernel/fs/vfs.cpp) and build.

Let's also use tracing here:

in dir_read:
-                       dprintf("dir_read: enter fix_dirent, ioContext=%p, "
+                       ktrace_printf("dir_read: enter fix_dirent, ioContext=%p, "
                               "vnode=%p, cookie=%p, buffer=%p, bufferSize=0x%x, _count=%p, i=0x%x\n",
                                   ioContext, vnode, cookie, buffer, (unsigned)bufferSize, _count, i);
                       if ( ((unsigned)vnode) && 0xffff0000 == 0xbeaf0000)
                               panic("dir_read: vnode is beafdead");
			error = fix_dirent(vnode, buffer, ioContext, &length);
-                       dprintf("dir_read: leave fix_dirent\n");
+                       ktrace_printf("dir_read: leave fix_dirent\n");

Now with those changes, typing "trac 1" in the debugger gives the first 30 messages and typing "trac" gives the next sets of messages, "trac 0" gives the last 30 messages.

Could you try this changes and see what are the last trace messages? That may shed some light on the problem, thanks for the feedback!

Changed 10 years ago by Adek336

line numbers tracing in fix_dirent()

comment:21 Changed 10 years ago by apprentice

Thanks to you. You have devoted a lot of time in debugging a bug that no one else has been able to reproduce on their hardware.

I'm currently a bit short of time so it might take some time to apply the diff and test it, but rest assured I'll do it as soon as I get a chance.

comment:22 Changed 10 years ago by apprentice

I applied the patch and recompiled. Hopefully the image is clear enough.

Changed 10 years ago by apprentice

Attachment: fix_dirent-diff.jpg added

comment:23 Changed 10 years ago by apprentice

Does the trac output give any useful information on the crash whatsoever?

Changed 10 years ago by Adek336

Attachment: d_reclen.diff added

trace file names; sanity check against buffer overflow

comment:24 Changed 10 years ago by Adek336

The trac output gives a pretty clear view of what instructions cause the crash and suggest that "parent" is not a valid pointer at that point; also we see, that the value of parent which is fed into fix_dirent() seems reasonable (in the trac, vnode=0x...). That is a lot of valuable information! For once, it allows me to suspect what the direct cause of the crash is, a buffer overflow during user_memcpy() of d_name; if that is the case, with the new diff you will see a panic "d_reclen>sizeof(buffer)" which you can easily continue through; that would be quite a progress :-)

The new diff also traces the names of files, so we will know the name of the /dev/* file which goes right before the offending file.

The diff is against the trunk version of vfs.cpp, so to revert to that version before applying the patch you may do

svn revert src/system/kernel/fs/vfs.cpp

comment:25 Changed 10 years ago by apprentice

Yup! It seems you nailed the bug. I'll run 'trac 0' later to get more informative ouput.

Changed 10 years ago by apprentice

Attachment: d_reclen-diff.jpg added

comment:26 Changed 10 years ago by Adek336

That's great !

Probably related to #1840.

comment:27 Changed 10 years ago by Adek336

Also note that #489 mentions both a 3com ethernet adapter and ls /dev/net problem.

Changed 10 years ago by Adek336

Attachment: devfs_create_vnode.diff added

trace vnodes created in devfs. print name of offending direntry

comment:28 in reply to:  25 Changed 10 years ago by Adek336

Replying to apprentice:

Yup! It seems you nailed the bug.

Not quite, we secured fix_dirent() against a stack overflow but the devfs is still erroneously passing an overgrown dir entry.

Could you send in the last trace messages ("trac 0") and all trace messages with the string devfs_create_vnode (all pages of "trac 1 300 -1 #devfs_create_vnode") after applying devfs_create_vnode.diff.

Changed 10 years ago by apprentice

Attachment: fix_dirent-trac.jpg added

comment:29 Changed 10 years ago by apprentice

I added the trac 0. I'll send the trace messages of the created device nodes as soon as possible. BTW, just out of curiosity, why can the system continue to run after a buffer overflow in kernel space. Doesn't it overwrite the adjacent memory locations or is this secured in the memcpy() implementation?

comment:30 Changed 10 years ago by Adek336

The last two patches avoid doing a memcpy when it would overflow the buffer.

Without them, you still can suspend and continue, because AFAIK the overflow damages the kernel stack allocated for the the ls /dev team. Because fix_dirent() dereferences a pointer from the stack right after memcpy is called and crashes, it doesn't have the opportunity to do much harm to other parts of the kernel which it would if it tried to, for example, update vnode lists with corrupt data from the stack.

comment:31 Changed 10 years ago by apprentice

So here is all the output from the latest diff. I hope it helps!

Changed 10 years ago by apprentice

Attachment: devfs_create_vnode1.jpg added

Changed 10 years ago by apprentice

Attachment: devfs_create_vnode2.jpg added

Changed 10 years ago by apprentice

Attachment: devfs_create_vnode3.jpg added

comment:32 Changed 10 years ago by Adek336

We're lucky!, because the triangles in devfs_create_vnode3.jpg rule out a bug in devfs. They probably come from a device driver. Also note that the triangle is ascii \314, just like in #1840.

Changed 10 years ago by Adek336

Attachment: panic_at_vnode_name.diff added

comment:33 Changed 10 years ago by Adek336

panic_at_vnode_name.diff: should panic at boot time when the corrupt file name is fed into devfs_create_vnode(); additional debug messages.

Particularly interesting are

bt

,

trac 1 300 -1 #republish

which could point at a faulty driver and

trac 0 90

.

comment:34 Changed 10 years ago by Adek336

Also to avoid recompiling a lot and reinstalling to the partition, you may do

jam kernel_x86

and copy the file generated/objects/haiku/x86/release/system/kernel/kernel_x86 to /boot/beos/system/, and after that reboot.

Changed 10 years ago by apprentice

Attachment: bt.jpg added

Changed 10 years ago by apprentice

Attachment: trac-republish1.jpg added

Changed 10 years ago by apprentice

Attachment: trac-republish2.jpg added

Changed 10 years ago by apprentice

Attachment: trac-republish3.jpg added

Changed 10 years ago by apprentice

Attachment: trac0-90.jpg added

Changed 10 years ago by apprentice

Attachment: trac0-90-2.jpg added

comment:35 Changed 10 years ago by apprentice

It's the 3com nic after all! The kernel shouldn't even create a device node for it since the driver initialization fails. So I guess the fbsd layer should be looked at very careful.

comment:36 Changed 10 years ago by Adek336

Could you see if deleting /boot/beos/system/add-ons/kernel/drivers/dev/net/3com helps ?

comment:37 Changed 10 years ago by apprentice

Yes. That does the trick. After deleting the driver and rebooting everything works. Except for the nic of course :-)

comment:38 Changed 10 years ago by apprentice

Strange. I have deleted the 3com driver before and it didn't help. And I guess it doesn't get loaded in safe mode and it crashed in safe mode when running find in /dev.

comment:39 Changed 10 years ago by Adek336

Are you still able to crash safe mode?

comment:40 Changed 10 years ago by apprentice

No. It doesn't crash in safe mode either.

comment:41 Changed 10 years ago by apprentice

This bug would have been easy to debug just by removing the nic but I was too short on time to devote enough effort on this. The driver is exactly the same as in FreeBSD and it works there. I wonder what makes it defunct in Haiku.

Changed 10 years ago by Adek336

Attachment: fbsd_compat.diff added

comment:42 Changed 10 years ago by Adek336

Yea, even more interestingly the driver works with my 3com 556b adapter.

Please apply fbsd_compat.diff against a trunk version of the sources, file system modules no longer emit debug messages, the freebsd compat layer now emit in various moments the contents of gDeviceNameList[0] to find out where it gets corrupt. Please also restore the 3com driver.

It should panic at boot, there should be only a handful of trace messages, these are of interest.

comment:43 Changed 10 years ago by apprentice

I'll apply your diff as soon as trac starts working again. I have been trying to download the diff for several hours now, but can't download anything. This is a known problem and has a ticket... Trac complains something about IOError: too many open files

comment:44 Changed 10 years ago by Adek336

Changed 10 years ago by apprentice

Attachment: sc-fbsd_compat.jpg added

Changed 10 years ago by apprentice

Attachment: trac-fbsd_compat.jpg added

comment:45 Changed 10 years ago by apprentice

So here are the trace messages after applying your diff.

Changed 10 years ago by Adek336

Attachment: fbsd_compat.2.diff added

comment:46 Changed 10 years ago by Adek336

Thanks !

I think the problem is as follows. gDeviceNameList[0] is a char pointer that points at a char array inside of ifp: ifp->device_name. ifp is freed in xl_detach after a failure in xl_attach. Thus, gDeviceNameList[0] points at unallocated memory.

We can verify this by looking at the traced contents after applying fbsd_compat.2.diff, backtrace not necessary.

Changed 10 years ago by apprentice

Attachment: fbsd_compat-2-trac.jpg added

comment:47 Changed 10 years ago by apprentice

trac from the latest diff.

comment:48 Changed 10 years ago by Adek336

Ok, thanks, so it was the problem I mentioned, I'll invent a fix sometime soon.

comment:49 Changed 10 years ago by apprentice

Thanks, acknowledged.

Changed 10 years ago by Adek336

Attachment: reproduce.diff added

comment:50 Changed 10 years ago by Adek336

reproduce.diff: exposes the bug in libfreebsd_network through the 3com driver (even if no 3com adapter is present).

comment:51 Changed 10 years ago by Adek336

proposed patch patch.1.diff

  • fix_dirent(): assert that the buffer is big enough for the file name; fix an off-by-1 error which would lead to string truncation;
  • if_free(): remove entries from gDevices and gDeviceNameList referenced by the ifp that is being freed.

if_index will be uninitialized, if if_initname has not been called, for example as a result of driver failure, so the patch checks for if_index initialization.

Avoid holes in the middle of gDeviceNameList as compat_open relies on that, relocate the last entry of gDeviceNameList in the place of the deleted entry.

_fbsd_uninit_driver will delete the devices which it finds in gDevices. I'm not sure if this is a problem, but if_free now removes the entry from gDevices so probably the device will never get deleted after calling if_free; on the other hand, if the driver doesn't call if_initname, no entry in gDevices is created so the device probably can't be deleted neither.

Changed 10 years ago by Adek336

Attachment: patch.1.diff added

Changed 10 years ago by apprentice

Attachment: reproduce-diff.jpg added

comment:52 Changed 10 years ago by apprentice

Here's the trace output from reproduce.diff

comment:53 Changed 10 years ago by apprentice

The patch did the trick. ls /dev doesn't crash anymore and ls /dev/net is empty as it should be. I think this ticket can be closed now.

bye, and thanks for all the fish.

comment:54 Changed 10 years ago by Adek336

Thanks for the note !

reproduce.diff is for the general public: it exposes the libfreebsd_network bug on any computer by simulating a failure in xl_attach which happens to only a few people, like you. Haiku with reproduce.diff crashes on the bug regardless if a 3com adapter is present and may be useful for local debugging.

patch.2.diff: "int clear" -> "u_short clear" as that is the type of struct ifnet::if_index.

Changed 10 years ago by Adek336

Attachment: patch.2.diff added

comment:55 in reply to:  51 Changed 10 years ago by Adek336

Replying to Adek336:

_fbsd_uninit_driver will delete the devices which it finds in gDevices. I'm not sure if this is a problem, but if_free now removes the entry from gDevices so probably the device will never get deleted after calling if_free; on the other hand, if the driver doesn't call if_initname, no entry in gDevices is created so the device probably can't be deleted neither.

Using ld to wrap around malloc,calloc,realloc and free in the driver binary I can see that there are no memory leaks with the 3com+reproduce.diff, that is, after going through probe / try attach / detach-after-failure-in-attach.

That is a rough indication, that my patch doesn't infere with deleting of the devices.

comment:56 Changed 10 years ago by apprentice

Should I test patch.2.diff? The former patch worked flawlessly.

comment:57 Changed 10 years ago by Adek336

No need to, it's only polished in one place.

comment:58 Changed 10 years ago by axeld

Now that has gone a long way :-) Thanks for your persistence and the patch, Adrian! There are some issues with it, though:

  • The changes in vfs.cpp are unrelated, and shouldn't be part of the same patch.
  • I'm not particularly sure if it's a good idea to change the index of an existing driver, although that pretty much looks like the only solution here. But since the initialization works sequentially, this case should actually never happen - if it does, it probably makes sense to panic.
  • Coding style issues: no space before '=' in 'clean=...', use NULL for pointers, not 0.
  • 'clear' should be defined inside the if-block, as it's not used outside of it.

comment:59 Changed 10 years ago by Adek336

It wouldn't be possible without Apprentice's patience, thanks a lot ! :-)

The detach function calls if_free. Is it possible that detach is called on a driver/device which is not the last on the list? Or is detach only called in reverse sequential order.

Changed 10 years ago by Adek336

Attachment: patch.3.vfs.diff added

Changed 10 years ago by Adek336

Attachment: patch.3.vfs.2.diff added

comment:60 Changed 10 years ago by Adek336

Patches no. 3, if_free() can wipe out only the last entry, hopefully if it breaks for someone it won't do much harm.

Changed 10 years ago by Adek336

Attachment: patch.3.fbdscompat.diff added

comment:61 Changed 10 years ago by axeld

Good question - it's probably wrong now. Maybe panicking isn't really a good idea; if this would be a USB driver, you cannot know in which order the devices are removed, anyway.

Maybe it's a better idea to review how the interface index is used, and if it could be changed on-the-fly safely (including potential threading problems), or if that's not possible, to rethink that interface a bit.

If you would like to look into that, great, but if not, just tell, no problem at all :-)

Changed 10 years ago by Adek336

Attachment: patch.4.fbsdcompat.diff added

comment:62 Changed 10 years ago by Adek336

  • the list returned by publish_devices, gDeviceNameList can have entries relocated and isn't indexed by if_index anymore;

  • gDevices list is indexed by if_index and won't have entries relocated, but can have empty holes;

  • like in FreeBSD, if_index is allocated in if_alloc instead of if_initname;

  • only compat_open seems to be relevant to change;

  • works both with my via_rhine and the pseudo-3com driver with reproducible.diff applied;

  • perhaps the looping code should be refactored?

  • in if.c:123, "name" is checked against NULL. Perhaps it should be checked against ""? Or does the device_manager check for such a filename?

comment:64 Changed 10 years ago by Adek336

If we decide to reuse if_indices we should at least leave a comment in the code saying that they are supposed to be not reused but are reused.

comment:65 in reply to:  62 Changed 10 years ago by Adek336

Replying to Adek336:

  • in if.c:123, "name" is checked against NULL. Perhaps it should be checked against ""? Or does the device_manager check for such a filename?

vfs.cpp seems to be checking against "". devfs.cpp does not check against "" and shouldn't because it looks like a "" has a special meaning during boot.
So if_initname should check, just like vfs.cpp does.

if.c:
void if_initname(...)
{
int i;

if (name == NULL || name[0] == '\0')
    panic("interface goes unnamed");

also fix typo ("unamed").

comment:66 Changed 10 years ago by Adek336

FreeBSD tries to get the lowest free if_index http://fxr.watson.org/fxr/source/net/if.c#L399 , NetBSD http://fxr.watson.org/fxr/source/net/if.c?v=NETBSD#L437 and Linux http://fxr.watson.org/fxr/source/net/core/dev.c?v=linux-2.6#L3608 first try to find an if_index bigger than the last allocated one and only fallback to reusing old if_indices.

I say, let's just make if_alloc try to find the lowest available if_index; that way one doesn't have to bother about resizing gDevices and gDeviceNameList. And also leave a comment about this behaviour in if_alloc.

I am not really sure if if_indices should be system unique or driver unique, it seems that FreeBSD and Haiku go for driver unique whereas NetBSD and Linux for system unique.

comment:67 Changed 10 years ago by Adek336

On a related note, in src/libs/compat/freebsd_network/device.c in compat_write()

        //if_printf(ifp, "compat_write(%lld, %p, [%lu])\n", position,
        //      buffer, *numBytes);

-       mb = m_getcl(0, MT_DATA, M_PKTHDR);
+       if (*numBytes > MHLEN)
+               mb = m_getcl(0, MT_DATA, M_PKTHDR);
+       else
+               mb = m_gethdr(0, MT_DATA);
+
        if (mb == NULL)
                return ENOBUFS;

This avoids allocating a mbuf with a cluster when a mbuf without a cluster suffices, this is how it's used in many places in fbsd_mbuf.c. Enjoy!

comment:68 Changed 10 years ago by Adek336

This probably affects all 3c900 [vendor 10b7, device 9001] users, so it's worth commiting before the alpha.

comment:69 Changed 10 years ago by axeld

Resolution: fixed
Status: newclosed

Thanks for the patch, applied in hrev29593! Please take a closer look to our coding style, though.

Note: See TracTickets for help on using tickets.