[ovs-dev] [PATCH] datapath: Ensure that skb->protocol is set in internal_dev_xmit().

Ben Pfaff blp at nicira.com
Mon Jan 2 17:52:26 UTC 2012


On Fri, Dec 30, 2011 at 12:52:14AM -0500, Jesse Gross wrote:
> On Wed, Dec 28, 2011 at 7:20 PM, Ben Pfaff <blp at 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.



More information about the dev mailing list