Date: Wed, 2 Aug 2006 04:32:23 +0400 From: Yar Tikhiy <yar@FreeBSD.org> To: Qing Li <qingli@FreeBSD.org> 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 Message-ID: <20060802003223.GA77391@comp.chem.msu.su> In-Reply-To: <200608011728.k71HSA9m019497@repoman.freebsd.org> References: <200608011728.k71HSA9m019497@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060802003223.GA77391>