From owner-cvs-all@FreeBSD.ORG Wed Aug 2 00:32:27 2006 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5009716A4E6; Wed, 2 Aug 2006 00:32:27 +0000 (UTC) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (comp.chem.msu.su [158.250.32.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8757043D46; Wed, 2 Aug 2006 00:32:26 +0000 (GMT) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.13.4/8.13.3) with ESMTP id k720WOlQ078510; Wed, 2 Aug 2006 04:32:24 +0400 (MSD) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.13.4/8.13.3/Submit) id k720WNLU078505; Wed, 2 Aug 2006 04:32:23 +0400 (MSD) (envelope-from yar) Date: Wed, 2 Aug 2006 04:32:23 +0400 From: Yar Tikhiy To: Qing Li Message-ID: <20060802003223.GA77391@comp.chem.msu.su> References: <200608011728.k71HSA9m019497@repoman.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200608011728.k71HSA9m019497@repoman.freebsd.org> User-Agent: Mutt/1.5.9i Cc: cvs-src@FreeBSD.org, sam@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/net if_vlan.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Aug 2006 00:32:27 -0000 On Tue, Aug 01, 2006 at 05:28:10PM +0000, Qing Li wrote: > qingli 2006-08-01 17:28:10 UTC > > FreeBSD src repository > > Modified files: > sys/net if_vlan.c > Log: > In vlan_input(), if the network interface does not perform h/w based > vlan tag processing, the code will use bcopy() to remove the vlan > tag field but the code copies 2 bytes too many, which essentially > overwrites the protocol type field. This had not been true until you removed the line of "dead" code. > Also, a tag value of -1 is generated for unrecognized interface type, > which would cause an invalid memory access in the vlans[] array. This is the only part of your change I like in theory, but its implementation is rather questionable. See below. > In addition, removed a line of dead code and its associated comments. The code was not dead, you just misunderstood it. See below. > Reviewed by: sam I think now I may commit some code Sam didn't dig a couple of years ago ;-) > Revision Changes Path > 1.107 +9 -15 src/sys/net/if_vlan.c > =================================================================== > RCS file: /usr/local/www/cvsroot/FreeBSD/src/sys/net/if_vlan.c,v > retrieving revision 1.106 > retrieving revision 1.107 > diff -u -p -r1.106 -r1.107 > --- src/sys/net/if_vlan.c 2006/07/09 06:04:00 1.106 > +++ src/sys/net/if_vlan.c 2006/08/01 17:28:10 1.107 > @@ -26,7 +26,7 @@ > * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > * SUCH DAMAGE. > * > - * $FreeBSD: /usr/local/www/cvsroot/FreeBSD/src/sys/net/if_vlan.c,v 1.106 2006/07/09 06:04:00 sam Exp $ > + * $FreeBSD: /usr/local/www/cvsroot/FreeBSD/src/sys/net/if_vlan.c,v 1.107 2006/08/01 17:28:10 qingli Exp $ > */ > > /* > @@ -917,21 +917,15 @@ vlan_input(struct ifnet *ifp, struct mbu > __func__, ntohs(evl->evl_encap_proto))); > > tag = EVL_VLANOFTAG(ntohs(evl->evl_tag)); > - > - /* > - * Restore the original ethertype. We'll remove > - * the encapsulation after we've found the vlan > - * interface corresponding to the tag. > - */ > - evl->evl_encap_proto = evl->evl_proto; This "dead" code copied the ethertype value saved in the dot1q mini-header to the outer Ethernet header's ethertype field. I.e.: +---------------+ V | +---------+---------+--------+-------+--------+----------- | dst | src | proto1 | tag | proto0 | payload... +---------+---------+--------+-------+--------+----------- Then the outer Ethernet header was as before VLAN encapsulation. That was why we could bcopy full ETHER_HDR_LEN bytes later. > break; > default: > - tag = (uint16_t) -1; > -#ifdef INVARIANTS > - panic("%s: unsupported if_type (%u)", > - __func__, ifp->if_type); > +#ifdef DEBUG > + /* XXX rate limit? */ > + if_printf(ifp, "unsupported if_type %u", ifp->if_type); > #endif Do you see this message on your console so often that you think it needs rate limiting? The former code paniced here because it could be reached only due to a programming error elsewhere. See ether_demux() in if_ethersubr.c and vlan_config() in this file. This default case can never be reached due to a bogus frame coming from the network. > - break; > + m_freem(m); > + ifp->if_noproto++; /* XXX? */ Nothing wrong with using if_noproto here IMHO, but this still "just can't happen". > + return; > } > } > > @@ -952,12 +946,12 @@ vlan_input(struct ifnet *ifp, struct mbu > if (mtag == NULL) { > /* > * Packet had an in-line encapsulation header; > - * remove it. The original header has already > - * been fixed up above. > + * remove it. Note that we leave the type field > + * unchanged; we only copy up the mac addresses. > */ > bcopy(mtod(m, caddr_t), > mtod(m, caddr_t) + ETHER_VLAN_ENCAP_LEN, > - ETHER_HDR_LEN); > + ETHER_HDR_LEN - ETHER_TYPE_LEN); > m_adj(m, ETHER_VLAN_ENCAP_LEN); > } Removing the "dead" assignment to evl->evl_encap_proto above and reducing the bcopy window by 2 bytes here is a nano-optimization. Which is worse, it smuggles hidden dot1q details in this otherwise encapsulation-neutral block of code. E.g., I could use the former code for both dot1q and ISL privately by just redefining ETHER_VLAN_ENCAP_LEN. Grand total: I would greately appreciate if you backed out this change and then fixed the tag value of -1 alone, but let me review that first. Thanks! -- Yar