On Fri, Dec 30, 2011 at 12:52:14AM -0500, Jesse Gross wrote:
> On Wed, Dec 28, 2011 at 7:20 PM, Ben Pfaff <b...@nicira.com> wrote:
> > diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> > index c56f3b2..31d55fc 100644
> > --- a/datapath/vport-internal_dev.c
> > +++ b/datapath/vport-internal_dev.c
> > @@ -94,11 +94,21 @@ static int internal_dev_mac_addr(struct net_device
> > *dev, void *p)
> > ??/* Called with rcu_read_lock_bh. */
> > ??static int internal_dev_xmit(struct sk_buff *skb, struct net_device
> > *netdev)
> > ??{
> > + ?? ?? ?? struct ethhdr *eth;
> > +
> > ?? ?? ?? ??if (unlikely(compute_ip_summed(skb, true))) {
> > ?? ?? ?? ?? ?? ?? ?? ??kfree_skb(skb);
> > ?? ?? ?? ?? ?? ?? ?? ??return 0;
> > ?? ?? ?? ??}
> >
> > + ?? ?? ?? /* Set skb->protocol since some sources (e.g. AF_PACKET) don't.
> > */
> > + ?? ?? ?? skb_reset_mac_header(skb);
> > + ?? ?? ?? eth = eth_hdr(skb);
> > + ?? ?? ?? if (ntohs(eth->h_proto) >= 1536)
> > + ?? ?? ?? ?? ?? ?? ?? skb->protocol = eth->h_proto;
> > + ?? ?? ?? else
> > + ?? ?? ?? ?? ?? ?? ?? skb->protocol = htons(ETH_P_802_2);
>
> I see where the problem comes from (it looks like it actually is
> possible to set skb->protocol from AF_PACKET, I guess that scapy
> doesn't though?).
Checking around, scapy, libpcap, tcpreplay, and lib/netdev-linux.c
don't. (I didn't actually check scapy but the bug report implies that
it doesn't.) It's not too much of a surprise that they don't, because
packet(7) says that they should not:
Address Types
The sockaddr_ll is a device independent physical layer address.
struct sockaddr_ll {
unsigned short sll_family; /* Always AF_PACKET */
unsigned short sll_protocol; /* Physical layer protocol */
int sll_ifindex; /* Interface number */
unsigned short sll_hatype; /* Header type */
unsigned char sll_pkttype; /* Packet type */
unsigned char sll_halen; /* Length of address */
unsigned char sll_addr[8]; /* Physical layer address */
};
...
When you send packets it is enough to specify sll_family, sll_addr,
sll_halen, sll_ifindex. The other fields should be 0. ...
> It seems like a bigger problem that this field isn't initialized than
> just for OVS. Should we be proposing to fix AF_PACKET upstream?
I don't know. I don't know what the use case is for setting the
protocol explicitly. Maybe it is useful for non-Ethernet media?
Maybe it is intended only for use with non-"raw" sockets?
I don't know what such a fix would look like. AF_PACKET is supposed
to work with various media, but there's no header_op for parsing a
packet's protocol, so we'd have to figure out what the right thing to
do is there. Would we need a new header_op?
This property of AF_PACKET looks unchanged from 2.6.12-rc2 to me, by
the way. Seems odd that no one would have noticed before.
In any case, we'll need a backportable fix.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
