From owner-freebsd-net Tue Oct 10 16: 7:12 2000 Delivered-To: freebsd-net@freebsd.org Received: from falla.videotron.net (falla.videotron.net [205.151.222.106]) by hub.freebsd.org (Postfix) with ESMTP id 68CB837B502 for ; Tue, 10 Oct 2000 16:07:09 -0700 (PDT) Received: from modemcable213.3-201-24.mtl.mc.videotron.ca ([24.201.3.213]) by falla.videotron.net (Sun Internet Mail Server sims.3.5.1999.12.14.10.29.p8) with ESMTP id <0G2800M9LLJOMZ@falla.videotron.net> for freebsd-net@freebsd.org; Tue, 10 Oct 2000 19:07:01 -0400 (EDT) Date: Tue, 10 Oct 2000 19:11:05 -0400 (EDT) From: Bosko Milekic Subject: Re: ip_input.c patch In-reply-to: <200010102202.e9AM2L538821@bubba.whistle.com> X-Sender: bmilekic@jehovah.technokratis.com To: Archie Cobbs Cc: freebsd-net@freebsd.org Message-id: MIME-version: 1.0 Content-type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Hi Archie [and others?], Some few comments follow... On Tue, 10 Oct 2000, Archie Cobbs wrote: > Bosko (and anyone else..), > > Does this patch look appropriate to you? > > Thanks, > -Archie > > ___________________________________________________________________________ > Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com > > Index: ip_input.c > =================================================================== > RCS file: /home/ncvs/src/sys/netinet/ip_input.c,v > retrieving revision 1.141 > diff -u -r1.141 ip_input.c > --- ip_input.c 2000/09/14 21:06:48 1.141 > +++ ip_input.c 2000/10/10 21:58:46 > @@ -338,15 +338,23 @@ > goto bad; > } > > +#if BYTE_ORDER != BIG_ENDIAN > /* > - * Convert fields to host representation. > + * Convert fields to host representation. But first make > + * sure we don't write into a multiply-referenced mbuf. > */ > + if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m) > + && (m = m_pullup(m, sizeof(*ip))) == NULL) { Assuming that you only want to attempt to pullup into a "multiply" referenced mbuf, this check is OK. > + ipstat.ips_badhlen++; > + return; > + } > NTOHS(ip->ip_len); > + NTOHS(ip->ip_off); > +#endif /* !BIG_ENDIAN */ > if (ip->ip_len < hlen) { > ipstat.ips_badlen++; > goto bad; > } > - NTOHS(ip->ip_off); > > /* > * Check that the amount of data in the buffers > @@ -599,7 +607,7 @@ > * Reassembly should be able to treat a mbuf cluster, for later > * operation of contiguous protocol headers on the cluster. (KAME) > */ > - if (m->m_flags & M_EXT) { /* XXX */ > + if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)) { > if ((m = m_pullup(m, hlen)) == 0) { > ipstat.ips_toosmall++; How about collapsing that m_pullup into the same if() statement, to remain consistent with the above? The reason I'm suggesting you be picky about this is that those relatively repetetive extensive checks on the "readability" of the mbuf will likely soon be replaced ... as soon as I merge a few diffs ( :-) ) and it will be simpler to search and replace this way. > #ifdef IPFIREWALL_FORWARD > @@ -688,6 +696,14 @@ > #ifdef IPDIVERT > /* Restore original checksum before diverting packet */ > if (divert_info != 0) { > + /* Don't overwrite multiply-referenced mbuf */ > + if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m) > + && (m = m_pullup(m, sizeof(*ip))) == NULL) { > +#ifdef IPFIREWALL_FORWARD > + ip_fw_fwd_addr = NULL; > +#endif > + return; > + } > ip->ip_len += hlen; > HTONS(ip->ip_len); > HTONS(ip->ip_off); > @@ -717,6 +733,15 @@ > /* Clone packet if we're doing a 'tee' */ > if ((divert_info & IP_FW_PORT_TEE_FLAG) != 0) > clone = m_dup(m, M_DONTWAIT); > + > + /* Don't overwrite multiply-referenced mbuf */ > + if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m) > + && (m = m_pullup(m, sizeof(*ip))) == NULL) { > +#ifdef IPFIREWALL_FORWARD > + ip_fw_fwd_addr = NULL; > +#endif > + return; > + } > > /* Restore packet header fields to original values */ > ip->ip_len += hlen; Overall, looks fine to me. Cheers, Bosko Milekic bmilekic@technokratis.com To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message