netdev - Re: [PATCH 3/3] vlan: Simplify the code now that VLAN_FLAG_REORDER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 11 Jun 2011 23:17:10 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	David Miller <davem@...emloft.net>,
	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>, Changli Gao <xiaosuo@...il.com>,
	netdev@...r.kernel.org, shemminger@...ux-foundation.org,
	kaber@...sh.net, fubar@...ibm.com, eric.dumazet@...il.com,
	andy@...yhouse.net, Jesse Gross <jesse@...ira.com>
Subject: Re: [PATCH 3/3] vlan: Simplify the code now that VLAN_FLAG_REORDER_HDR is always set

Jiri Pirko <jpirko@...hat.com> writes:

> Sun, May 22, 2011 at 09:42:20PM CEST, ebiederm@...ssion.com wrote:
>>
>>Now that we no longer support clearing VLAN_FLAG_REORDER_HDR remove the
>>code that was needed to cope with the case when it was cleared.
>>
>>Signed-off-by: Eric W. Biederman <ebiederm@...ssion.com>
>>---
>> net/8021q/vlan_dev.c |   45 +++++----------------------------------------
>> 1 files changed, 5 insertions(+), 40 deletions(-)
>>
>>diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>>index 20629fe..2b3ca1e 100644
>>--- a/net/8021q/vlan_dev.c
>>+++ b/net/8021q/vlan_dev.c
>>@@ -96,63 +96,28 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>> 				const void *daddr, const void *saddr,
>> 				unsigned int len)
>> {
>>-	struct vlan_hdr *vhdr;
>>-	unsigned int vhdrlen = 0;
>>-	u16 vlan_tci = 0;
>> 	int rc;
>> 
>>-	if (!(vlan_dev_info(dev)->flags & VLAN_FLAG_REORDER_HDR)) {
>>-		vhdr = (struct vlan_hdr *) skb_push(skb, VLAN_HLEN);
>>-
>>-		vlan_tci = vlan_dev_info(dev)->vlan_id;
>>-		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
>>-		vhdr->h_vlan_TCI = htons(vlan_tci);
>>-
>>-		/*
>>-		 *  Set the protocol type. For a packet of type ETH_P_802_3/2 we
>>-		 *  put the length in here instead.
>>-		 */
>>-		if (type != ETH_P_802_3 && type != ETH_P_802_2)
>>-			vhdr->h_vlan_encapsulated_proto = htons(type);
>>-		else
>>-			vhdr->h_vlan_encapsulated_proto = htons(len);
>>-
>>-		skb->protocol = htons(ETH_P_8021Q);
>>-		type = ETH_P_8021Q;
>>-		vhdrlen = VLAN_HLEN;
>>-	}
>>-
>> 	/* Before delegating work to the lower layer, enter our MAC-address */
>> 	if (saddr == NULL)
>> 		saddr = dev->dev_addr;
>> 
>> 	/* Now make the underlying real hard header */
>> 	dev = vlan_dev_info(dev)->real_dev;
>>-	rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
>>-	if (rc > 0)
>>-		rc += vhdrlen;
>>+	rc = dev_hard_header(skb, dev, type, daddr, saddr, len);
>> 	return rc;
>> }
>> 
>> static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
>> 					    struct net_device *dev)
>> {
>>-	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
>> 	unsigned int len;
>>+	u16 vlan_tci;
>> 	int ret;
>> 
>>-	/* Handle non-VLAN frames if they are sent to us, for example by DHCP.
>>-	 *
>>-	 * NOTE: THIS ASSUMES DIX ETHERNET, SPECIFICALLY NOT SUPPORTING
>>-	 * OTHER THINGS LIKE FDDI/TokenRing/802.3 SNAPs...
>>-	 */
>>-	if (veth->h_vlan_proto != htons(ETH_P_8021Q) ||
> 	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 	    		this should stay here.
>

I admit this is a change in behavior from today so we need to be careful
here.

At least the comment needs to change.

When this code was written the assumption was that everything would come
in tagged and this code was a special exception for pf_packet sockets.

Now everything comes in untagged this code becomes a special exception
for pf_packet sockets of not putting on a vlan header.

To me this special exception pf_packet sockets looks like a bug, that
should have been conditionality on VLAN_FLAG_REORDER_HDR but was over
looked.

Now maybe we want to be bug compatible, or do this as a separate patch.

I would expect sending tagged packets out a vlan device would result
in double tagged packets but apparently not always.


Eric
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html