| Message ID | 1286803522-16478-1-git-send-email-phil@nwl.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 11 octobre 2010 à 15:25 +0200, Phil Sutter a écrit : > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > net/packet/af_packet.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 9a17f28..ad37754 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -56,6 +56,7 @@ > #include <linux/in.h> > #include <linux/inet.h> > #include <linux/netdevice.h> > +#include <linux/if_arp.h> > #include <linux/if_packet.h> > #include <linux/wireless.h> > #include <linux/kernel.h> > @@ -983,6 +984,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) > > reserve = dev->hard_header_len; > > + /* allow VLAN packets when device supports them */ > + if (likely(dev->type == ARPHRD_ETHER) && > + likely(!(dev->features & NETIF_F_VLAN_CHALLENGED))) > + reserve += VLAN_HLEN; > + > err = -ENETDOWN; > if (unlikely(!(dev->flags & IFF_UP))) > goto out_put; If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes frames for MTU=1500 Should we really care ? If not, just do reserve = dev->hard_header_len + VLAN_HLEN; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 11 Oct 2010 16:03:02 +0200 > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes > frames for MTU=1500 > > Should we really care ? > > If not, just do > > reserve = dev->hard_header_len + VLAN_HLEN; Thats a good point, I think we need to validate the SKB protocol field. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 11, 2010 at 09:01:53AM -0700, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 11 Oct 2010 16:03:02 +0200 > > > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes > > frames for MTU=1500 > > > > Should we really care ? > > > > If not, just do > > > > reserve = dev->hard_header_len + VLAN_HLEN; > > Thats a good point, I think we need to validate the SKB protocol > field. Which is set to the value of the passed struct sockaddr_ll field sll_protocol. At least in the two userspace code samples I have here, the later field is set to htons(ETH_P_ALL). So unless one changes the API, the only way to find out the packet type is to actually parse the given ethernet header. Since tpacket_rcv() just interprets the vlan_tci skb field, such detailed packet inspection is otherwise not done in af_packet.c. Greetings, Phil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 11, 2010 at 07:29:32PM +0200, Phil Sutter wrote: > On Mon, Oct 11, 2010 at 09:01:53AM -0700, David Miller wrote: > > From: Eric Dumazet <eric.dumazet@gmail.com> > > Date: Mon, 11 Oct 2010 16:03:02 +0200 > > > > > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes > > > frames for MTU=1500 > > > > > > Should we really care ? > > > > > > If not, just do > > > > > > reserve = dev->hard_header_len + VLAN_HLEN; > > > > Thats a good point, I think we need to validate the SKB protocol > > field. > > Which is set to the value of the passed struct sockaddr_ll field > sll_protocol. At least in the two userspace code samples I have here, > the later field is set to htons(ETH_P_ALL). So unless one changes the > API, the only way to find out the packet type is to actually parse the > given ethernet header. Yes, like eth_type_trans does I guess. I think we had a similar discussion already: http://lists.openwall.net/netdev/2010/01/06/38 Summary: if we want to make the protocol field have the correct value for this case we need to make it work for other transports not just for ethernet. > Since tpacket_rcv() just interprets the vlan_tci skb field, such > detailed packet inspection is otherwise not done in af_packet.c. > > Greetings, Phil > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Michael S. Tsirkin" <mst@redhat.com> Date: Tue, 12 Oct 2010 19:19:41 +0200 > Yes, like eth_type_trans does I guess. I think we had a similar > discussion already: > > http://lists.openwall.net/netdev/2010/01/06/38 > > Summary: if we want to make the protocol field have the correct > value for this case we need to make it work for other > transports not just for ethernet. Right, so for now we should just allow 4-byte larger than MTU TX packets, as long as the device is ethernet and can handle VLANs properly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 12, 2010 at 10:40:32AM -0700, David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Tue, 12 Oct 2010 19:19:41 +0200 > > > Yes, like eth_type_trans does I guess. I think we had a similar > > discussion already: > > > > http://lists.openwall.net/netdev/2010/01/06/38 > > > > Summary: if we want to make the protocol field have the correct > > value for this case we need to make it work for other > > transports not just for ethernet. > > Right, so for now we should just allow 4-byte larger > than MTU TX packets, as long as the device is ethernet > and can handle VLANs properly. Incidently, I believe that this problem will only become more acute and complex if support for 802.1ad (Provider Bridges, aka Q-in-Q), 802.1ah (Provider Backbone Bridges, aka MAC-in-MAC) or other standards which further extend the maximum frame size. Dave, you were mentioning to me the other day that the kernel already supports some notion of Q-in-Q (though its not 802.1ad). Does the current implementation allow for frames > 1504 bytes? Is that a complication to the change proposed here? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Simon Horman <horms@verge.net.au> Date: Fri, 22 Oct 2010 10:41:26 +0200 > Incidently, I believe that this problem will only become more acute > and complex if support for 802.1ad (Provider Bridges, aka Q-in-Q), > 802.1ah (Provider Backbone Bridges, aka MAC-in-MAC) or other standards > which further extend the maximum frame size. No doubt. > Dave, you were mentioning to me the other day that the kernel > already supports some notion of Q-in-Q (though its not 802.1ad). > Does the current implementation allow for frames > 1504 bytes? It's only going to hardware offload and allow the extra space for the outer-most VLAN tag. Everthing inside of the outer tag will be handled in software as far as Linux is concerned. > Is that a complication to the change proposed here? For now, I don't think so. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 9a17f28..ad37754 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -56,6 +56,7 @@ #include <linux/in.h> #include <linux/inet.h> #include <linux/netdevice.h> +#include <linux/if_arp.h> #include <linux/if_packet.h> #include <linux/wireless.h> #include <linux/kernel.h> @@ -983,6 +984,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) reserve = dev->hard_header_len; + /* allow VLAN packets when device supports them */ + if (likely(dev->type == ARPHRD_ETHER) && + likely(!(dev->features & NETIF_F_VLAN_CHALLENGED))) + reserve += VLAN_HLEN; + err = -ENETDOWN; if (unlikely(!(dev->flags & IFF_UP))) goto out_put;
Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/packet/af_packet.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)