Date: Tue, 10 Oct 2000 19:11:05 -0400 (EDT) From: Bosko Milekic <bmilekic@dsuper.net> To: Archie Cobbs <archie@whistle.com> Cc: freebsd-net@freebsd.org Subject: Re: ip_input.c patch Message-ID: <Pine.BSF.4.21.0010101850530.8597-100000@jehovah.technokratis.com> In-Reply-To: <200010102202.e9AM2L538821@bubba.whistle.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0010101850530.8597-100000>