All of lore.kernel.org
 help / color / mirror / Atom feed
* packet_mmap, 802.1Q and bridges
@ 2013-07-29 16:53 ` Phil Sutter
  0 siblings, 0 replies; 15+ messages in thread
From: Phil Sutter @ 2013-07-29 16:53 UTC (permalink / raw)
  To: netdev; +Cc: Ben Greear, Stephen Hemminger, bridge, David S. Miller

Hello everyone,

I have found a situation where sending of VLAN tagged packets via
packet_mmap over a bridge creates MTU problems.

The setup is as follows:
- physical interface eth0 (mv643xx)
- bridge interface br0 contains just eth0
- packet_mmap sender bound to br0, socket() called with SOCK_RAW and
  ETH_P_ALL as arguments, PACKET_VERSION set to TPACKET_V2
- MTU is 1500 on all involved interfaces

When userspace sends a full frame, it will pass tpacket_snd which will
reject it unless there is a small patch of mine applied (for patch and
related discussion, see: http://patchwork.ozlabs.org/patch/67422/).
Tpacket_snd creates an SKB around the passed data, with 'protocol' field
set to whatever was specified at socket() stage (in this case,
ETH_P_ALL). Eventually, dev_queue_xmit() is called.

The SKB created in af_packet.c is passed to the bridge code, while the
interesting part is the function br_dev_queue_push_xmit() in
net/bridge/br_forward.c. This function checks the frame's length against
the device's MTU, therefore calling packet_length() which in turn does
look at the SKB's protocol field for whether to account for an
additional VLAN header or not. In this case, br_dev_queue_push_xmit()
will drop the frame since ETH_P_ALL != ETH_P_8021Q.

Using tcpdump, I could observe the packet occurs on br0, but not eth0.
Changing packet_length() to also subtract VLAN_HLEN if skb->protocol is
htons(ETH_P_ALL) fixed the problem.

While preparing this email, I read again the discussion referenced above
and found it surprisingly adequate for the problem described here. If I
am not mistaken, the consensus was whether we should parse the given
buffer in order to extract the real ethernet protocol or just account
for four extra bytes, regardless of the actual packet.

IMHO the problem described above is a strong argument for the first
option. There is code which depends on skb->protocol's content to be
correct, so af_packet should stick to it.

Comments are highly appreciated. In the meantime, I will have a look at
how to parse the packet_mmap buffer. If you got anything in that
direction already, please feel free to come forward.

Best wishes, Phil

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Bridge] packet_mmap, 802.1Q and bridges
@ 2013-07-29 16:53 ` Phil Sutter
  0 siblings, 0 replies; 15+ messages in thread
From: Phil Sutter @ 2013-07-29 16:53 UTC (permalink / raw)
  To: netdev; +Cc: Ben Greear, bridge, Stephen Hemminger, David S. Miller

Hello everyone,

I have found a situation where sending of VLAN tagged packets via
packet_mmap over a bridge creates MTU problems.

The setup is as follows:
- physical interface eth0 (mv643xx)
- bridge interface br0 contains just eth0
- packet_mmap sender bound to br0, socket() called with SOCK_RAW and
  ETH_P_ALL as arguments, PACKET_VERSION set to TPACKET_V2
- MTU is 1500 on all involved interfaces

When userspace sends a full frame, it will pass tpacket_snd which will
reject it unless there is a small patch of mine applied (for patch and
related discussion, see: http://patchwork.ozlabs.org/patch/67422/).
Tpacket_snd creates an SKB around the passed data, with 'protocol' field
set to whatever was specified at socket() stage (in this case,
ETH_P_ALL). Eventually, dev_queue_xmit() is called.

The SKB created in af_packet.c is passed to the bridge code, while the
interesting part is the function br_dev_queue_push_xmit() in
net/bridge/br_forward.c. This function checks the frame's length against
the device's MTU, therefore calling packet_length() which in turn does
look at the SKB's protocol field for whether to account for an
additional VLAN header or not. In this case, br_dev_queue_push_xmit()
will drop the frame since ETH_P_ALL != ETH_P_8021Q.

Using tcpdump, I could observe the packet occurs on br0, but not eth0.
Changing packet_length() to also subtract VLAN_HLEN if skb->protocol is
htons(ETH_P_ALL) fixed the problem.

While preparing this email, I read again the discussion referenced above
and found it surprisingly adequate for the problem described here. If I
am not mistaken, the consensus was whether we should parse the given
buffer in order to extract the real ethernet protocol or just account
for four extra bytes, regardless of the actual packet.

IMHO the problem described above is a strong argument for the first
option. There is code which depends on skb->protocol's content to be
correct, so af_packet should stick to it.

Comments are highly appreciated. In the meantime, I will have a look at
how to parse the packet_mmap buffer. If you got anything in that
direction already, please feel free to come forward.

Best wishes, Phil

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Parse ethernet headers in af_packet.c
  2013-07-29 16:53 ` [Bridge] " Phil Sutter
  (?)
@ 2013-07-31 17:27 ` Phil Sutter
  2013-07-31 17:27   ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol Phil Sutter
  -1 siblings, 1 reply; 15+ messages in thread
From: Phil Sutter @ 2013-07-31 17:27 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

Hi,

This is an AF_PACKET specific solution to the problems discussed in [1]
and [2].

Basically this incorporates calling eth_type_trans() for packets to send
when the outgoing device's type is ARPHRD_ETHER.

In order to having a more generic solution which could be used in other
places (like e.g. tun.c), I second Michael's approach in [2], but would
extend it to a generic equivalent to eth_type_trans(). A device-type
specific layer-2 header parser if you will. Note that eth_type_trans()
also sets skbuff's dev, pkt_type and mac_header fields. A
dev->header_ops->type_trans() could do just that plus set skb->protocol
(maybe only if it's ETH_P_ALL).

[1] http://patchwork.ozlabs.org/patch/67422/
[2] http://lists.openwall.net/netdev/2010/01/06/38

Best wishes, Phil

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-07-31 17:27 ` Parse ethernet headers in af_packet.c Phil Sutter
@ 2013-07-31 17:27   ` Phil Sutter
  2013-07-31 17:27     ` [PATCH 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
  2013-08-01  0:30     ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
  0 siblings, 2 replies; 15+ messages in thread
From: Phil Sutter @ 2013-07-31 17:27 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

This may be necessary when the SKB is passed to other layers on the go,
which may check the protocol field on their own. An example is a VLAN
packet sent using AF_PACKET out on a bridge interface. The bridging code
does check the SKB size, accounting for any VLAN header only if the
protocol field is set accordingly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 20a1bd0..e549e17 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -88,6 +88,7 @@
 #include <linux/virtio_net.h>
 #include <linux/errqueue.h>
 #include <linux/net_tstamp.h>
+#include <linux/if_arp.h>
 
 #ifdef CONFIG_INET
 #include <net/inet_common.h>
@@ -2005,6 +2006,9 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err))
 			return err;
 
+		if (dev->type == ARPHRD_ETHER)
+			skb->protocol = eth_type_trans(skb, dev);
+
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
 	}
@@ -2324,6 +2328,13 @@ static int packet_snd(struct socket *sock,
 
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
+	if (dev->type == ARPHRD_ETHER) {
+		skb->protocol = eth_type_trans(skb, dev);
+	} else {
+		skb->protocol = proto;
+		skb->dev = dev;
+	}
+
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
 		/* Earlier code assumed this would be a VLAN pkt,
 		 * double-check this now that we have the actual
@@ -2338,8 +2349,6 @@ static int packet_snd(struct socket *sock,
 		}
 	}
 
-	skb->protocol = proto;
-	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
 
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] af_packet: fix for sending VLAN frames via packet_mmap
  2013-07-31 17:27   ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol Phil Sutter
@ 2013-07-31 17:27     ` Phil Sutter
  2013-07-31 17:27       ` [PATCH 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
  2013-08-01  0:30     ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
  1 sibling, 1 reply; 15+ messages in thread
From: Phil Sutter @ 2013-07-31 17:27 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

Since tpacket_fill_skb() parses the protocol field in ethernet frames'
headers, it's easy to see if any passed frame is a VLAN one and account
for the extended size.

But as the real protocol does not turn up before tpacket_fill_skb()
runs which in turn also checks the frame length, move the max frame
length calculation into the function.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e549e17..ce7b4f0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1924,7 +1924,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		__be16 proto, unsigned char *addr, int hlen)
 {
 	union tpacket_uhdr ph;
-	int to_write, offset, len, tp_len, nr_frags, len_max;
+	int to_write, offset, len, tp_len, nr_frags, len_max, max_frame_len;
 	struct socket *sock = po->sk.sk_socket;
 	struct page *page;
 	void *data;
@@ -1947,10 +1947,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		tp_len = ph.h1->tp_len;
 		break;
 	}
-	if (unlikely(tp_len > size_max)) {
-		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
-		return -EMSGSIZE;
-	}
 
 	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
@@ -2013,6 +2009,18 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		to_write -= dev->hard_header_len;
 	}
 
+	max_frame_len = dev->mtu + dev->hard_header_len;
+	if (skb->protocol == htons(ETH_P_8021Q))
+		max_frame_len += VLAN_HLEN;
+
+	if (size_max > max_frame_len)
+		size_max = max_frame_len;
+
+	if (unlikely(tp_len > size_max)) {
+		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
+		return -EMSGSIZE;
+	}
+
 	offset = offset_in_page(data);
 	len_max = PAGE_SIZE - offset;
 	len = ((to_write > len_max) ? len_max : to_write);
@@ -2051,7 +2059,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	struct net_device *dev;
 	__be16 proto;
 	bool need_rls_dev = false;
-	int err, reserve = 0;
+	int err;
 	void *ph;
 	struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
 	int tp_len, size_max;
@@ -2084,8 +2092,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	if (unlikely(dev == NULL))
 		goto out;
 
-	reserve = dev->hard_header_len;
-
 	err = -ENETDOWN;
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto out_put;
@@ -2093,9 +2099,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	size_max = po->tx_ring.frame_size
 		- (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
-	if (size_max > dev->mtu + reserve)
-		size_max = dev->mtu + reserve;
-
 	do {
 		ph = packet_current_frame(po, &po->tx_ring,
 				TP_STATUS_SEND_REQUEST);
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] af_packet: simplify VLAN frame check in packet_snd
  2013-07-31 17:27     ` [PATCH 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
@ 2013-07-31 17:27       ` Phil Sutter
  0 siblings, 0 replies; 15+ messages in thread
From: Phil Sutter @ 2013-07-31 17:27 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

For ethernet frames, eth_type_trans() already parses the header, so one
can skip this when checking the frame size.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ce7b4f0..bbe1ece 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2333,23 +2333,16 @@ static int packet_snd(struct socket *sock,
 
 	if (dev->type == ARPHRD_ETHER) {
 		skb->protocol = eth_type_trans(skb, dev);
+		if (skb->protocol == htons(ETH_P_8021Q))
+			reserve += VLAN_HLEN;
 	} else {
 		skb->protocol = proto;
 		skb->dev = dev;
 	}
 
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
-		/* Earlier code assumed this would be a VLAN pkt,
-		 * double-check this now that we have the actual
-		 * packet in hand.
-		 */
-		struct ethhdr *ehdr;
-		skb_reset_mac_header(skb);
-		ehdr = eth_hdr(skb);
-		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
-			err = -EMSGSIZE;
-			goto out_free;
-		}
+		err = -EMSGSIZE;
+		goto out_free;
 	}
 
 	skb->priority = sk->sk_priority;
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-07-31 17:27   ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol Phil Sutter
  2013-07-31 17:27     ` [PATCH 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
@ 2013-08-01  0:30     ` David Miller
  2013-08-01 10:35       ` Phil Sutter
  1 sibling, 1 reply; 15+ messages in thread
From: David Miller @ 2013-08-01  0:30 UTC (permalink / raw)
  To: phil; +Cc: netdev

From: Phil Sutter <phil@nwl.cc>
Date: Wed, 31 Jul 2013 19:27:45 +0200

> @@ -2324,6 +2328,13 @@ static int packet_snd(struct socket *sock,
>  
>  	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>  
> +	if (dev->type == ARPHRD_ETHER) {
> +		skb->protocol = eth_type_trans(skb, dev);
> +	} else {
> +		skb->protocol = proto;
> +		skb->dev = dev;
> +	}
> +
>  	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
>  		/* Earlier code assumed this would be a VLAN pkt,
>  		 * double-check this now that we have the actual
> @@ -2338,8 +2349,6 @@ static int packet_snd(struct socket *sock,
>  		}
>  	}
>  
> -	skb->protocol = proto;
> -	skb->dev = dev;
>  	skb->priority = sk->sk_priority;
>  	skb->mark = sk->sk_mark;

I don't see anything explaining why you are avoiding setting skb->dev when
you use eth_type_trans() to set the skb->protocol field.

Also, why isn't the user setting the protocol field correctly?  Isn't
that the _real_ source of these problems?

This patch series cannot be applied as is, it's either wrong, or
explanations are missing from the commit messages.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-08-01  0:30     ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
@ 2013-08-01 10:35       ` Phil Sutter
  2013-08-02  1:12         ` David Miller
  0 siblings, 1 reply; 15+ messages in thread
From: Phil Sutter @ 2013-08-01 10:35 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

Hi,

On Wed, Jul 31, 2013 at 05:30:17PM -0700, David Miller wrote:
> From: Phil Sutter <phil@nwl.cc>
> Date: Wed, 31 Jul 2013 19:27:45 +0200
> 
> > @@ -2324,6 +2328,13 @@ static int packet_snd(struct socket *sock,
> >  
> >  	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
> >  
> > +	if (dev->type == ARPHRD_ETHER) {
> > +		skb->protocol = eth_type_trans(skb, dev);
> > +	} else {
> > +		skb->protocol = proto;
> > +		skb->dev = dev;
> > +	}
> > +
> >  	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
> >  		/* Earlier code assumed this would be a VLAN pkt,
> >  		 * double-check this now that we have the actual
> > @@ -2338,8 +2349,6 @@ static int packet_snd(struct socket *sock,
> >  		}
> >  	}
> >  
> > -	skb->protocol = proto;
> > -	skb->dev = dev;
> >  	skb->priority = sk->sk_priority;
> >  	skb->mark = sk->sk_mark;
> 
> I don't see anything explaining why you are avoiding setting skb->dev when
> you use eth_type_trans() to set the skb->protocol field.

Simply because eth_type_trans() sets skb->dev already. You're right, I
probably should have mentioned this explicitly. At least my introductory
mail elaborated on this.

> Also, why isn't the user setting the protocol field correctly?  Isn't
> that the _real_ source of these problems?

Because the user specifies the value later used for skb->protocol at
either socket(), bind() or sendto() (i.e. flush) stage. Having both
"regular" and VLAN frames in the same TX ring is not possible at least.

> This patch series cannot be applied as is, it's either wrong, or
> explanations are missing from the commit messages.

Hmm. Is it OK to rely on eth_type_trans() setting skb->dev after all?
Depending on that, I would either add a comment or drop the change.

Best wishes, Phil

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-08-01 10:35       ` Phil Sutter
@ 2013-08-02  1:12         ` David Miller
  2013-08-02  9:37           ` [PATCH v2 " Phil Sutter
  0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2013-08-02  1:12 UTC (permalink / raw)
  To: phil; +Cc: netdev

From: Phil Sutter <phil@nwl.cc>
Date: Thu, 1 Aug 2013 12:35:09 +0200

> Is it OK to rely on eth_type_trans() setting skb->dev after all?

Yes, it is.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-08-02  1:12         ` David Miller
@ 2013-08-02  9:37           ` Phil Sutter
  2013-08-02  9:37             ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
  2013-08-02 21:58             ` [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
  0 siblings, 2 replies; 15+ messages in thread
From: Phil Sutter @ 2013-08-02  9:37 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

This may be necessary when the SKB is passed to other layers on the go,
which check the protocol field on their own. An example is a VLAN packet
sent out using AF_PACKET on a bridge interface. The bridging code checks
the SKB size, accounting for any VLAN header only if the protocol field
is set accordingly.

Note that eth_type_trans() sets skb->dev to the passed argument, so this
can be skipped in packet_snd() for ethernet frames, as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since V1:
- minor lingual review of the comment
- added note about setting skb->dev for non-ethernet frames only
---
 net/packet/af_packet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 20a1bd0..e549e17 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -88,6 +88,7 @@
 #include <linux/virtio_net.h>
 #include <linux/errqueue.h>
 #include <linux/net_tstamp.h>
+#include <linux/if_arp.h>
 
 #ifdef CONFIG_INET
 #include <net/inet_common.h>
@@ -2005,6 +2006,9 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err))
 			return err;
 
+		if (dev->type == ARPHRD_ETHER)
+			skb->protocol = eth_type_trans(skb, dev);
+
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
 	}
@@ -2324,6 +2328,13 @@ static int packet_snd(struct socket *sock,
 
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
+	if (dev->type == ARPHRD_ETHER) {
+		skb->protocol = eth_type_trans(skb, dev);
+	} else {
+		skb->protocol = proto;
+		skb->dev = dev;
+	}
+
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
 		/* Earlier code assumed this would be a VLAN pkt,
 		 * double-check this now that we have the actual
@@ -2338,8 +2349,6 @@ static int packet_snd(struct socket *sock,
 		}
 	}
 
-	skb->protocol = proto;
-	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
 
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap
  2013-08-02  9:37           ` [PATCH v2 " Phil Sutter
@ 2013-08-02  9:37             ` Phil Sutter
  2013-08-02  9:37               ` [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
  2013-08-02 21:58               ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap David Miller
  2013-08-02 21:58             ` [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
  1 sibling, 2 replies; 15+ messages in thread
From: Phil Sutter @ 2013-08-02  9:37 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

Since tpacket_fill_skb() parses the protocol field in ethernet frames'
headers, it's easy to see if any passed frame is a VLAN one and account
for the extended size.

But as the real protocol does not turn up before tpacket_fill_skb()
runs which in turn also checks the frame length, move the max frame
length calculation into the function.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e549e17..ce7b4f0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1924,7 +1924,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		__be16 proto, unsigned char *addr, int hlen)
 {
 	union tpacket_uhdr ph;
-	int to_write, offset, len, tp_len, nr_frags, len_max;
+	int to_write, offset, len, tp_len, nr_frags, len_max, max_frame_len;
 	struct socket *sock = po->sk.sk_socket;
 	struct page *page;
 	void *data;
@@ -1947,10 +1947,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		tp_len = ph.h1->tp_len;
 		break;
 	}
-	if (unlikely(tp_len > size_max)) {
-		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
-		return -EMSGSIZE;
-	}
 
 	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
@@ -2013,6 +2009,18 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		to_write -= dev->hard_header_len;
 	}
 
+	max_frame_len = dev->mtu + dev->hard_header_len;
+	if (skb->protocol == htons(ETH_P_8021Q))
+		max_frame_len += VLAN_HLEN;
+
+	if (size_max > max_frame_len)
+		size_max = max_frame_len;
+
+	if (unlikely(tp_len > size_max)) {
+		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
+		return -EMSGSIZE;
+	}
+
 	offset = offset_in_page(data);
 	len_max = PAGE_SIZE - offset;
 	len = ((to_write > len_max) ? len_max : to_write);
@@ -2051,7 +2059,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	struct net_device *dev;
 	__be16 proto;
 	bool need_rls_dev = false;
-	int err, reserve = 0;
+	int err;
 	void *ph;
 	struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
 	int tp_len, size_max;
@@ -2084,8 +2092,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	if (unlikely(dev == NULL))
 		goto out;
 
-	reserve = dev->hard_header_len;
-
 	err = -ENETDOWN;
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto out_put;
@@ -2093,9 +2099,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	size_max = po->tx_ring.frame_size
 		- (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
-	if (size_max > dev->mtu + reserve)
-		size_max = dev->mtu + reserve;
-
 	do {
 		ph = packet_current_frame(po, &po->tx_ring,
 				TP_STATUS_SEND_REQUEST);
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd
  2013-08-02  9:37             ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
@ 2013-08-02  9:37               ` Phil Sutter
  2013-08-02 21:59                 ` David Miller
  2013-08-02 21:58               ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap David Miller
  1 sibling, 1 reply; 15+ messages in thread
From: Phil Sutter @ 2013-08-02  9:37 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

For ethernet frames, eth_type_trans() already parses the header, so one
can skip this when checking the frame size.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/packet/af_packet.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ce7b4f0..bbe1ece 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2333,23 +2333,16 @@ static int packet_snd(struct socket *sock,
 
 	if (dev->type == ARPHRD_ETHER) {
 		skb->protocol = eth_type_trans(skb, dev);
+		if (skb->protocol == htons(ETH_P_8021Q))
+			reserve += VLAN_HLEN;
 	} else {
 		skb->protocol = proto;
 		skb->dev = dev;
 	}
 
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
-		/* Earlier code assumed this would be a VLAN pkt,
-		 * double-check this now that we have the actual
-		 * packet in hand.
-		 */
-		struct ethhdr *ehdr;
-		skb_reset_mac_header(skb);
-		ehdr = eth_hdr(skb);
-		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
-			err = -EMSGSIZE;
-			goto out_free;
-		}
+		err = -EMSGSIZE;
+		goto out_free;
 	}
 
 	skb->priority = sk->sk_priority;
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol
  2013-08-02  9:37           ` [PATCH v2 " Phil Sutter
  2013-08-02  9:37             ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
@ 2013-08-02 21:58             ` David Miller
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2013-08-02 21:58 UTC (permalink / raw)
  To: phil; +Cc: netdev

From: Phil Sutter <phil@nwl.cc>
Date: Fri,  2 Aug 2013 11:37:39 +0200

> This may be necessary when the SKB is passed to other layers on the go,
> which check the protocol field on their own. An example is a VLAN packet
> sent out using AF_PACKET on a bridge interface. The bridging code checks
> the SKB size, accounting for any VLAN header only if the protocol field
> is set accordingly.
> 
> Note that eth_type_trans() sets skb->dev to the passed argument, so this
> can be skipped in packet_snd() for ethernet frames, as well.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Applied.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap
  2013-08-02  9:37             ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
  2013-08-02  9:37               ` [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
@ 2013-08-02 21:58               ` David Miller
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2013-08-02 21:58 UTC (permalink / raw)
  To: phil; +Cc: netdev

From: Phil Sutter <phil@nwl.cc>
Date: Fri,  2 Aug 2013 11:37:40 +0200

> Since tpacket_fill_skb() parses the protocol field in ethernet frames'
> headers, it's easy to see if any passed frame is a VLAN one and account
> for the extended size.
> 
> But as the real protocol does not turn up before tpacket_fill_skb()
> runs which in turn also checks the frame length, move the max frame
> length calculation into the function.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Applied.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd
  2013-08-02  9:37               ` [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
@ 2013-08-02 21:59                 ` David Miller
  0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2013-08-02 21:59 UTC (permalink / raw)
  To: phil; +Cc: netdev

From: Phil Sutter <phil@nwl.cc>
Date: Fri,  2 Aug 2013 11:37:41 +0200

> For ethernet frames, eth_type_trans() already parses the header, so one
> can skip this when checking the frame size.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Applied.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-08-02 21:59 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-29 16:53 packet_mmap, 802.1Q and bridges Phil Sutter
2013-07-29 16:53 ` [Bridge] " Phil Sutter
2013-07-31 17:27 ` Parse ethernet headers in af_packet.c Phil Sutter
2013-07-31 17:27   ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol Phil Sutter
2013-07-31 17:27     ` [PATCH 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
2013-07-31 17:27       ` [PATCH 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
2013-08-01  0:30     ` [PATCH 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller
2013-08-01 10:35       ` Phil Sutter
2013-08-02  1:12         ` David Miller
2013-08-02  9:37           ` [PATCH v2 " Phil Sutter
2013-08-02  9:37             ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap Phil Sutter
2013-08-02  9:37               ` [PATCH v2 3/3] af_packet: simplify VLAN frame check in packet_snd Phil Sutter
2013-08-02 21:59                 ` David Miller
2013-08-02 21:58               ` [PATCH v2 2/3] af_packet: fix for sending VLAN frames via packet_mmap David Miller
2013-08-02 21:58             ` [PATCH v2 1/3] af_packet: when sending ethernet frames, parse header for skb->protocol David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.