cvs commit: src/sys/net if_vlan.c

[Sam Leffler]

Yar Tikhiy wrote:
> 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.

Correct, but with the short bcopy this is no longer needed.  proto0 is
left intact and only dst+src are copied up.

> 
>>  			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.

I wrote the XXX comment.  The previous code allowed an array index w/ -1
when the panic was not present.  Rather than permit this incorrect code
to remain the proper cleanup was done instead.  I added a printf because
if that action ever happened it was clearly a mistake.  Qing Li wrapped
it under DEBUG since the statistic already accounts for it and leaving
it as a straight printf left open the possibility of spam'ing the console.

I do not understand your complaint.  Are you suggesting we leave panic's
in code paths that we can recover from?  Are you suggesting we not
comment places where alternative choices might be made?

> 
>> -			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!
> 

We obviously had no idea you were interested in this.  I was asked to
review the original change and noticed the bogus panic code and
suggested it be fixed at the same time.  I already pointed out to Qing
Li that the new code is equivalent to the old but he preferred it and
since it does nothing wrong I don't see the point in arguing about
it--work it out with him privately if you want it reverted.

	Sam