Date: Tue, 14 Nov 2000 18:19:22 -0500 (EST) From: Bosko Milekic <bmilekic@dsuper.net> To: Thomas Moestl <tmoestl@gmx.net> Cc: freebsd-net@FreeBSD.ORG Subject: Re: bug in bridging/dummynet code - PR kern/19551 Message-ID: <Pine.BSF.4.21.0011141807180.6305-100000@jehovah.technokratis.com> In-Reply-To: <20001114220117.A732@forge.local>
next in thread | previous in thread | raw e-mail | index | archive | help
That _DEFFINATELY_ looks EVIL.
I'd like to see some debugging info, at the least a stack trace, so
that we can see whether everybody "reporting" this problem is referring
to a side effect of this.
Hopefully I'll have this appended to the PR or in my INBOX by this
weekend so that we can look into fixing this.
On Tue, 14 Nov 2000, Thomas Moestl wrote:
> Hi,
>
> I think I have spotted a bug in the bridge/dummynet code that might be
> responsible for some panics people have reported recently - see e.g.
> PR kern/19551.
> PR kern/21534 seems related and are probably about the same thing,
> PR kern/19488 goes in the same direction.
>
> Bosko, I'm CCing this to you because the PR is currently assigned to
> you.
>
> Here is the relevant section of code from netinet/ip_dummynet.c:402:
>
> #ifdef BRIDGE
> case DN_TO_BDG_FWD : {
> struct mbuf *m = (struct mbuf *)pkt ;
> struct ether_header hdr;
>
> if (m->m_len < ETHER_HDR_LEN
> && (m = m_pullup(m, ETHER_HDR_LEN)) == NULL) {
> m_freem(m);
> break;
> }
> bcopy(mtod(m, struct ether_header *), &hdr, ETHER_HDR_LEN);
> m_adj(m, ETHER_HDR_LEN);
> bdg_forward(&m, &hdr, pkt->ifp);
> if (m)
> m_freem(m);
> }
> break ;
> #endif
>
> Now, pkt is a malloc()ed structure, not an mbuf! Calling m_pullup() on it
> seems defective, at least because m_free may be called in m_pullup(),
> which leaks kernel memory if the freed structure is not an mbuf.
> And of course, the ethernet header should be in the mbuf in pkt->dn_m.
> Should it be:
>
> #ifdef BRIDGE
> case DN_TO_BDG_FWD : {
> struct mbuf *m = (struct mbuf *)pkt ;
> struct ether_header hdr;
>
> if (pkt->dn_m->m_len < ETHER_HDR_LEN
> && (pkt->dn_m = m_pullup(pkt->dn_m, ETHER_HDR_LEN)) == NULL) {
> m_freem(pkt->dn_m);
> break;
> }
> bcopy(mtod(pkt->dn_m, struct ether_header *), &hdr, ETHER_HDR_LEN);
> m_adj(pkt->dn_m, ETHER_HDR_LEN);
> bdg_forward(&m, &hdr, pkt->ifp);
> if (m)
> /* bdg_format will put pkt->dn_m into mbuf into m in our case */
> m_freem(m);
> }
> break ;
> #endif
>
> Hmm, maybe I'm wrong here, but that seems odd to me. Please enlighten
> me! Unfortunetly, I have no machine I could use to test it at the moment.
>
> I just wanted to ask before I add this to the PR.
>
> Sorry if I was wrong,
> - Thomas
Regards,
Bosko Milekic
bmilekic@technokratis.com
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0011141807180.6305-100000>