[RFC, PATCH]: Pass link level header from/to PPP interface

[RFC, PATCH]: Pass link level header from/to PPP interface

[Urs Thuermann]

The PPP interface strips off the PPP header from the packet before
passing it up with netif_rx().  On the xmit path, it has to add the
PPP header itself because dev->header_ops is NULL.

This means that a PF_PACKET, SOCK_RAW socket can't get the link level
header and has to use the sll_protocol field of the sockaddr_ll to
know what type of packet is received with recvfrom(2).  I consider
this a design flaw since with most (all?) other interfaces you only
need to know the sll_hatype to know what type of packet you get.

I have patched the PPP code to include the PPP header.  I tried with
IP and IPv6 over PPP and it works as expected.  This patch, however,
changes the interface to the user space in an incompatible way.  But
IMHO we could include it, since

* PF_PACKET is Linux specific and not portable anyway.  Most apps use
  libpcap instead of opening PF_PACKET sockets themselves.

* Using strace on tcpdump, it seems that libpcap on Linux uses
  PF_PACKET/SOCK_DGRAM for PPP interfaces and thus is not affected by
  my patch.

* Other apps using PF_PACKET/SOCK_RAW can easily be changed to
  PF_PACKET/SOCK_DGRAM if they don't want to see the link level
  header.  After all, this is what SOCK_DGRAM is for.

Currently SOCK_RAW and SOCK_DGRAM are the same although the packet(7)
man page states that with SOCK_RAW packets are passed without any
changes.  This makes having SOCK_RAW besides SOCK_DGRAM useless for
PPP.

So what is your opinion about this change?


Signed-off-by: Urs Thuermann <urs@isnogud.escape.de>

---
 drivers/net/ppp_generic.c |   41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

Index: net-2.6/drivers/net/ppp_generic.c
===================================================================
--- net-2.6.orig/drivers/net/ppp_generic.c	2008-02-08 11:09:03.000000000 +0100
+++ net-2.6/drivers/net/ppp_generic.c	2008-02-08 13:27:29.000000000 +0100
@@ -873,12 +873,32 @@
 /*
  * Network interface unit routines.
  */
+
+/* Put the 2-byte PPP protocol number on the front of skb */
+static int ppp_header(struct sk_buff *skb, struct net_device *dev,
+		      unsigned short type,
+		      const void *daddr, const void *saddr, unsigned len)
+{
+	unsigned char *pp;
+	int npi, proto;
+
+	npi = ethertype_to_npindex(ntohs(skb->protocol));
+	if (npi < 0)
+		return -dev->hard_header_len;
+
+	pp = skb_push(skb, 2);
+	proto = npindex_to_proto[npi];
+	pp[0] = proto >> 8;
+	pp[1] = proto;
+
+	return dev->hard_header_len;
+}
+
 static int
 ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ppp *ppp = (struct ppp *) dev->priv;
-	int npi, proto;
-	unsigned char *pp;
+	int npi;
 
 	npi = ethertype_to_npindex(ntohs(skb->protocol));
 	if (npi < 0)
@@ -897,16 +917,6 @@
 		goto outf;
 	}
 
-	/* Put the 2-byte PPP protocol number on the front,
-	   making sure there is room for the address and control fields. */
-	if (skb_cow_head(skb, PPP_HDRLEN))
-		goto outf;
-
-	pp = skb_push(skb, 2);
-	proto = npindex_to_proto[npi];
-	pp[0] = proto >> 8;
-	pp[1] = proto;
-
 	netif_stop_queue(dev);
 	skb_queue_tail(&ppp->file.xq, skb);
 	ppp_xmit_process(ppp);
@@ -969,9 +979,14 @@
 	return err;
 }
 
+static const struct header_ops ppp_header_ops ____cacheline_aligned = {
+	.create		= ppp_header,
+};
+
 static void ppp_setup(struct net_device *dev)
 {
 	dev->hard_header_len = PPP_HDRLEN;
+	dev->header_ops = &ppp_header_ops;
 	dev->mtu = PPP_MTU;
 	dev->addr_len = 0;
 	dev->tx_queue_len = 3;
@@ -1677,10 +1692,10 @@
 			kfree_skb(skb);
 		} else {
 			/* chop off protocol */
+			skb_reset_mac_header(skb);
 			skb_pull_rcsum(skb, 2);
 			skb->dev = ppp->dev;
 			skb->protocol = htons(npindex_to_ethertype[npi]);
-			skb_reset_mac_header(skb);
 			netif_rx(skb);
 			ppp->dev->last_rx = jiffies;
 		}

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[David Miller]

From: Urs Thuermann <urs@isnogud.escape.de>
Date: 10 Feb 2008 10:48:51 +0100

> So what is your opinion about this change?

No general objections from me.

But if libpcap and tcpdump can already identify PPP packets
then, besides "consistency", what does this buy us other
than potential breakage?

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[Urs Thuermann]

Hello Dave,

> But if libpcap and tcpdump can already identify PPP packets
> then, besides "consistency", what does this buy us other
> than potential breakage?

My reason for the patch is mostly cleanliness and to make the kernel
match what is described in packet(7).

If you open a PF_PACKET/SOCK_RAW socket and bind it to an interface
you get a fixed type of frame/packet depending on the type of
interface, including the LL header.  Also if you bind to all
interfaces you can look at the interface type to know what type of
packet you get.  But not for PPP interfaces.

I've written a small packet dump util with a packet parser (somewhat
comparable to tcpdump but much smaller), which looks like this

        struct sockaddr_ll addr;
        ...
        s = open(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
        ...
        recvfrom(s, buffer, sizeof(buffer), 0, &addr, &size);
        switch (addr.sll_hatype) {
        case ARPHRD_ETHER:
        case ARPHRD_IEEE80211:
        case ARPHRD_LOOPBACK:
                parse_ether(buffer);
                break;
        case ARPHRD_TUNNEL:
                parse_ip(buffer);
                break;
        case ARPHRD_SIT:
                parse_ipv6(buffer);
                break;

and so on.  Only for PPP I need to switch on the addr.sll_protocol
since with PPP I don't get the PPP header:

        case ARPHRD_PPP:
                switch (ntohs(addr.sll_protocol))
		case ETH_P_IP:
		    parse_ip(buffer);
		    break;
		case ETH_P_IPV6:
		    parse_ipv6(buffer);
		    break;
                ...
		}
		break;

With my patch you would write

        case ARPHRD_PPP:
                parse_ppp(buffer);
                break;

and the parse function can show the complete PPP frame with protocol
field.  Without the patch there is no way to see the 2-byte PPP header
and, in contrast to packet(7), SOCK_DGRAM and SOCK_RAW are the same.

Do you consider this strong enough to justify this change?

urs

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[David Miller]

From: Urs Thuermann <urs@isnogud.escape.de>
Date: 13 Feb 2008 17:56:00 +0100

> Do you consider this strong enough to justify this change?

Can tcpdump and libpcap already handle PPP properly?

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[Urs Thuermann]

David Miller <davem@davemloft.net> writes:

> Can tcpdump and libpcap already handle PPP properly?

Yes, with the (admittedly non-serious) limitation, that both, tcpdump
and tcpdump -e don't show the PPP header.  I don't remember how
tcpdump behaved on BSD in this respect (so many years ago), but I
think libpcap will use the DLT_PPP link type on BSD, which has the PPP
header.

urs

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[David Miller]

From: Urs Thuermann <urs@isnogud.escape.de>
Date: 10 Feb 2008 10:48:51 +0100

> * Using strace on tcpdump, it seems that libpcap on Linux uses
>   PF_PACKET/SOCK_DGRAM for PPP interfaces and thus is not affected by
>   my patch.

When given a specific device, libpcap uses SOCK_RAW.

I spent some time seriously considering this change but I
can't because it really can potentially break so much stuff.

Sorry.

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[Urs Thuermann]

David Miller <davem@davemloft.net> writes:

> When given a specific device, libpcap uses SOCK_RAW.

I don't think this is completely true for PPP interfaces.  If you do a
pcap_open_live() on interface "any", libpcap opens a PF_PACKET/SOCK_DGRAM
socket.  Otherwise, on a specific device a SOCK_RAW is opened like you
write but libpcap asks with SIOCGIFHWADDR for the ARPHRD_* type of the
interface.  In case of PPP libpcap closes the socket and opens a new
socket with SOCK_DGRAM to capture packets on.  Here's a strace showing
this behavior:

    execve("./pcap", ["./pcap", "ppp0"], [/* 34 vars */]) = 0
    brk(0)                                  = 0x8068f5c
    ...
    socket(PF_PACKET, SOCK_RAW, 768)        = 3
    ioctl(3, SIOCGIFINDEX, {ifr_name="lo", ifr_index=1}) = 0
    ioctl(3, SIOCGIFHWADDR, {ifr_name="ppp0", ifr_hwaddr=00:00:00:00:00:00}) = 0
    close(3)                                = 0
    socket(PF_PACKET, SOCK_DGRAM, 768)      = 3
    ioctl(3, SIOCGIFINDEX, {ifr_name="ppp0", ifr_index=22}) = 0
    bind(3, {sa_family=AF_PACKET, proto=0x03, if22, pkttype=PACKET_HOST, addr(0)={0, }, 20) = 0
    ...

> I spent some time seriously considering this change but I
> can't because it really can potentially break so much stuff.

Thanks for your time on this.  But AFAICS, the patch doesn't break
libpcap.  I have it running here on 2.4 and 2.6 kernels for some weeks
now with PPPoE.  tcpdump and wireshark run without problems since they
use libpcap and for SOCK_DGRAM there is no difference.

My own simple packet sniffer is not libpcap based and takes advantage of
the patch.  Other programs using PF_PACKET I run here are dhcpd,
dhclient, rarpd, pppoe-server, and pppoe, but all of these use
SOCK_DGRAM or SOCK_PACKET and/or work on Ethernet interfaces.

I don't think the patch would break much stuff.

urs

Re: [RFC, PATCH]: Pass link level header from/to PPP interface

[David Miller]

From: Urs Thuermann <urs@isnogud.escape.de>
Date: 27 Feb 2008 22:08:06 +0100

> David Miller <davem@davemloft.net> writes:
> 
> > I spent some time seriously considering this change but I
> > can't because it really can potentially break so much stuff.
> 
> Thanks for your time on this.  But AFAICS, the patch doesn't break
> libpcap.

Not libpcap in this case, but potentially other programs
might be effected by it.

Sorry.