Date: Wed, 21 Feb 2001 18:59:16 +0200 From: Ruslan Ermilov <ru@FreeBSD.org> To: idobarnea@NewMail.Net, bmilekic@FreeBSD.org Cc: mouss <usebsd@free.fr>, hackers@FreeBSD.org Subject: Re: Bug in creating ICMP error messages in FreeBSD4.2 Message-ID: <20010221185915.A80894@sunbay.com> In-Reply-To: <3a938f82.19b.0@NewMail.Net>; from idobarnea@NewMail.Net on Wed, Feb 21, 2001 at 09:50:58AM %2B0000 References: <3a938f82.19b.0@NewMail.Net>
next in thread | previous in thread | raw e-mail | index | archive | help
[redirected to -net]
On Wed, Feb 21, 2001 at 09:50:58AM +0000, idobarnea@NewMail.Net wrote:
> >On Mon, Feb 19, 2001 at 08:26:56PM +0100, mouss wrote:
> > > At 14:25 19/02/01 +0200, idobarnea@NewMail.Net wrote:
> > > > Hi,
> > > > I encountered the following problem in the 4.2 version.
> > > > In ip_forward, the following lines intend to save the mbuf in
> > > > case we want to send ICMP error later:
> > > > mcopy = m_copy(m, 0, imin((int)ip->ip_len, 64));
> > > > if (mcopy && (mcopy->m_flags & M_EXT))
> > > > m_copydata(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
> > > >
> > > > Later on, before sending the ICMP packet we do:
> > > > if (mcopy->m_flags & M_EXT)
> > > > m_copyback(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
> > > >
> > > > The problem as I understand it is that the m_copydata and m_copyback,
> > > > actually do nothing (It just copies from mcopy to itself).
> > >
> > > I'm speaking from memory, so don't take this for more than it is:)
> > >
> > > As far as I understand:
> > > m_copym copies the mbuf, but if there is external storage, only
> > > pointers are copied. so you get two mbuf chains with a common
> > > external storage.
> > > m_copydata will copy the external storage.
> > > that's why there are both m_copym and m_copydata. so while
> > > m_copydata(mcopy, .... (mcopy...))
> > > is surprising, it's not nothing. it copies the data pointed to
> > > in mcopy.
> > >
> > I wrote this code, and the above said is right.
>
> I understand that this is what you meant it to be.
> But look again at m_copydata.
> This is the relevant line:
> bcopy(mtod(m, caddr_t) + off, cp, count);
> If cp is mtod(m, caddr_t) and off is 0, this command has no effect.
>
> Anyway, even if I am wrong about this, the facts are that if you
> take FreeBSD 4.2-RELEASE machine, put net.inet.ip.forwarding to 1,
> and then blast the kernel with ip packets with len 256 and with
> destination to which it has a direct network route (on its local
> lan), but it can't resolve the arp entry the kernel crashes
> after a while. You can try this yourself. I can explain some more,
> and give exact conf' if anybody wants it.
>
> Now if you stop with a debugger in icmp_error you see that
> oip->ip_len equals 1, then you see the other stuff I talked about,
> and then you get a kernel panic.
>
> After you make the correction I suggested, and do the same thing,
> you see that oip->ip_len equals 256 (the right value), and you
> never get a kernel panic.
>
> I'll be glad to here other explanations to this kernel crash.
>
You are right, the code does nothing, though it was supposed to
preserve the byte order of IP header fields in case ip_output()
modifies them. The idea was to save the standard IP header in
some area in mbuf that is unused for M_EXT mbufs, but obviously
mtod() place is wrong, it points to the external data for M_EXT
mbuf. Could you please try this patch:
Index: ip_input.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.130.2.12
diff -u -p -r1.130.2.12 ip_input.c
--- ip_input.c 2001/02/01 20:25:09 1.130.2.12
+++ ip_input.c 2001/02/21 16:48:10
@@ -1513,7 +1513,8 @@ ip_forward(m, srcrt)
*/
mcopy = m_copy(m, 0, imin((int)ip->ip_len, 64));
if (mcopy && (mcopy->m_flags & M_EXT))
- m_copydata(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
+ m_copydata(mcopy, 0, sizeof(struct ip),
+ (caddr_t)(&mcopy->m_ext + 1));
#ifdef IPSTEALTH
if (!ipstealth) {
@@ -1661,7 +1662,8 @@ ip_forward(m, srcrt)
return;
}
if (mcopy->m_flags & M_EXT)
- m_copyback(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
+ m_copyback(mcopy, 0, sizeof(struct ip),
+ (caddr_t)(&mcopy->m_ext + 1));
icmp_error(mcopy, type, code, dest, destifp);
}
And if that does not work, this (more clear but slower) one:
Index: ip_input.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.130.2.13
diff -u -p -r1.130.2.13 ip_input.c
--- ip_input.c 2001/02/07 01:03:13 1.130.2.13
+++ ip_input.c 2001/02/21 16:10:35
@@ -1510,9 +1510,17 @@ ip_forward(m, srcrt)
* we need to generate an ICMP message to the src.
*/
mcopy = m_copy(m, 0, imin((int)ip->ip_len, 64));
- if (mcopy && (mcopy->m_flags & M_EXT))
- m_copydata(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
+ /*
+ * Make sure the copy is read-write.
+ */
+ if (mcopy && (mcopy->m_flags & M_EXT)) {
+ struct mbuf *mtemp = mcopy;
+
+ mcopy = m_dup(mtemp, M_DONTWAIT);
+ m_freem(mtemp);
+ }
+
#ifdef IPSTEALTH
if (!ipstealth) {
#endif
@@ -1658,8 +1666,6 @@ ip_forward(m, srcrt)
m_freem(mcopy);
return;
}
- if (mcopy->m_flags & M_EXT)
- m_copyback(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
icmp_error(mcopy, type, code, dest, destifp);
}
Cheers,
--
Ruslan Ermilov Oracle Developer/DBA,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010221185915.A80894>