Bug 18471

Summary:

mbuf and mbuf clusters can be freed multiple times

Product:

Base System

Reporter:

dwmalone <dwmalone>

Component:

kern

Assignee:

freebsd-bugs (Nobody) <bugs>

Status:

Closed FIXED

Severity:

Affects Only Me

Priority:

Normal

Version:

3.4-STABLE

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
file.diff	none

Description dwmalone 2000-05-09 19:20:00 UTC

The code for freeing mbuf clusters and mbufs doesn't check if the
object is already free before freeing it. While this shouldn't
happen it makes debugging difficult when it does, as we found while
trying to debug some problems with the netatalk code.

It would be better if the kernel paniced at the time of the second
free, as opposed to some time later when the entry which has been
freed twice gets reused while still in use!

Fix: I've been running a machine tracking current at home with the
following KASSERTs added and INVARIENTS on. I've seen no problems
with them.
How-To-Repeat: 
Write code which doesn't track it's mbufs carefully enough, and try
to debug.

Comment 1 Jin Guojun 2000-05-09 19:29:55 UTC

> It would be better if the kernel paniced at the time of the second
> free, as opposed to some time later when the entry which has been
> freed twice gets reused while still in use!

I disagree to panic at this point. The better fixing is just printing
out some error message and do nothing for refreeing code. Whoever writes
such driver code will know what happens.

	-Jin

Comment 2 dwmalone 2000-05-10 07:54:54 UTC

> > It would be better if the kernel paniced at the time of the second
> > free, as opposed to some time later when the entry which has been
> > freed twice gets reused while still in use!

> I disagree to panic at this point. The better fixing is just printing
> out some error message and do nothing for refreeing code. Whoever writes
> such driver code will know what happens.

That wouldn't really be consistant with the other reference counters
in the kernel (vnode reference counters would be the main example
in my mind). At the stage when this happens the kernel has definitely
done something wrong - possibly having corrupted in data. So a
panic seems apropriate.

It isn't clear to me what useful message you could print to help
diganose the problem. Neither the address, nor the contents of the
mbuf would be that useful. A stack trace would probably be useful
- but a kernel dump would definitely be. Maybe I've missed something,
but...

	David.

Comment 3 bmilekic@dsuper.net 2000-05-16 18:15:03 UTC

	Indeed, David is correct when he argues that this should be a panic,
  especially since it's a KASSERT().
	Without even considering the other troubles the [now confused]
  calling code will run into, just look at what actually happens at the
  mcluster free routine: the "freed" cluster is attached to the mclfree
  list, which is essentially singly linked. When you free a cluster already
  in the free list, you'll basically "lose" all the clusters sitting after
  that one cluster on the list. This then becomes virtually a "leak" and
  the system is royally borked.
  	Somebody please commit this code.

  --Bosko

--
 Bosko Milekic * pages.infinit.net/bmilekic/index.html * www.technokratis.com
 bmilekic@dsuper.net * bmilekic@technokratis.com * b.milekic@marianopolis.edu

 "Give a man a fish and he will eat for a day. Teach him how
  to fish, and he will sit in a boat and drink beer all day."

Comment 4 jlemon freebsd_committer

2000-06-11 17:42:38 UTC

State Changed
From-To: open->closed

Patch applied, thanks!