net: packet: option to only pass skb protocol

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly. However, currently doing this also requires supplying the interface index.

The following patch makes it possible to avoid supplying the interface index by interpreting index 0 as "use device this socket is bound to".

Quoted 8 lines · Michael S. Tsirkin

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly. However, currently doing this also requires supplying the interface index.

The following patch makes it possible to avoid supplying the interface index by interpreting index 0 as "use device this socket is bound to".

Patch is correct, but I dont understand why zero initialization by caller is any better then supplying ifindex (known when socket was bound to device ?)

To avoid one syscall at socket setup (to get ifindex from dev name), you prefer to add a test/branch at each send() syscall...

Am I missing something ?

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

binding socket to device might be done by a separate process from the one doing sendmsg, and IMO the device socket is bound to might change at any time.

So the sending process would need to get socket name before each sendmsg.

Makes sense?

Quoted 3 lines

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

What about automatically detecting the protocol from the data being sent to avoid the necessity of specifying it in the first place?

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 15:28:22 -0600

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

Quoted 2 lines

What about automatically detecting the protocol from the data being sent to avoid the necessity of specifying it in the first place?

This limits packet socket usage to only protocols the kernel is aware of, defeating part of the usefulness of the packet socket facility.

From: "Michael S. Tsirkin" <[email protected]>
Date: Tue, 5 Jan 2010 22:50:40 +0200

Quoted 7 lines

binding socket to device might be done by a separate process from the one doing sendmsg, and IMO the device socket is bound to might change at any time.

So the sending process would need to get socket name before each sendmsg.

Quoted 1 line

Makes sense?

Not really, when it's at the expense of everyone else.

If you can pass the FD around, you can pass around auxiliary information as well.

Make sense? :-)

Quoted 11 lines · David Miller

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 15:28:22 -0600

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

What about automatically detecting the protocol from the data being sent to avoid the necessity of specifying it in the first place?

Quoted 2 lines

This limits packet socket usage to only protocols the kernel is aware of, defeating part of the usefulness of the packet socket facility.

We could do this if the protocol is ETH_P_ALL - skbs end up with this protocol currently when sendmsg does not have msgname and when socket is set up to listen for all packets. It's not a valid protocol value, is it?

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 15:28:22 -0600

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

Quoted 2 lines

What about automatically detecting the protocol from the data being sent to avoid the necessity of specifying it in the first place?

This limits packet socket usage to only protocols the kernel is aware of, defeating part of the usefulness of the packet socket facility.

Quoted 8 lines · David Miller

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 15:28:22 -0600

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

When sending packets with a packet socket it is often necessary to set protocol in msg_name: otherwise the protocol field in the skb will not be set correctly.

Quoted 5 lines

Quoted 2 lines

What about automatically detecting the protocol from the data being sent to avoid the necessity of specifying it in the first place?

This limits packet socket usage to only protocols the kernel is aware of, defeating part of the usefulness of the packet socket facility.

I don't follow.

If SOCK_RAW packets are being sent, the protocol number is embedded in the packet data and the kernel should be able to extract it regardless of whether the kernel actually supports it or not. I see that Michael just posted a patch for this.

If SOCK_DGRAM packets are being sent, then I agree that the app needs to pass it down at send time or at bind() time to support protocols of which the kernel is not aware.

While looking at the code I noticed that while the protocol number is validated at socket creation time it does not appear to be validated for calls to bind() for packet sockets. Is this intentional?

As a further question, does it actually make sense to check the protocol number at packet socket creation? It seems like we should be able to allow a packet socket to specify arbitrary protocol numbers since the kernel doesn't really need to do anything with them.

Chris

If SOCK_RAW packets are being sent, the protocol number is embedded in the packet data

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 16:13:29 -0600

Quoted 2 lines

If SOCK_RAW packets are being sent, the protocol number is embedded in the packet data

Where exactly is that protocol value? Not every link level protocol is ethernet.

We support FDDI, HIPPI, wireless, VLAN, PPP, and a host of others.

So you can't know for sure unless you assume ethernet, which you can't do.

Quoted 6 lines · David Miller

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 16:13:29 -0600

Quoted 2 lines

If SOCK_RAW packets are being sent, the protocol number is embedded in the packet data

Quoted 7 lines

Where exactly is that protocol value? Not every link level protocol is ethernet.

We support FDDI, HIPPI, wireless, VLAN, PPP, and a host of others.

So you can't know for sure unless you assume ethernet, which you can't do.

You are right. This is TX though so the socket is bound to a specific device, and devices know which link protocol they use. So we could add a get_protocol operation to each device type, and call it if the protocol passed is ETH_P_ALL to get the real one.

Like this: except I only added this for ethernet, and I would have to add this for the rest of device types. But I would like to hear what others think about this before I do the rest of the work for the rest of device types.

Does the below look sane?

include/linux/netdevice.h +1 -0

index a3fccc8..99d7801 100644
@@ -319,6 +319,7 @@ struct header_ops {
 	void	(*cache_update)(struct hh_cache *hh,
 				const struct net_device *dev,
 				const unsigned char *haddr);
+	__be16	(*get_protocol)(struct sk_buff *skb, struct net_device *dev)
 };
 
 /* These flag bits are private to the generic network queueing

net/ethernet/eth.c +12 -0

index 205a1c1..3e7761d 100644
@@ -231,6 +231,17 @@ int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr)
 EXPORT_SYMBOL(eth_header_parse);
 
 /**
+ * eth_get_protocol - extract protocol value from packet
+ * @skb: packet to extract protocol from
+ * @dev: source device
+ */
+__be16 eth_get_protocol(const struct sk_buff *skb, struct net_device *dev)
+{
+	const struct ethhdr *eth = eth_hdr(skb);
+	return eth->h_proto
+}
+
+/**
  * eth_header_cache - fill cache entry from neighbour
  * @neigh: source neighbour
  * @hh: destination cache entry

@@ -324,6 +335,7 @@ EXPORT_SYMBOL(eth_validate_addr);
 const struct header_ops eth_header_ops ____cacheline_aligned = {
 	.create		= eth_header,
 	.parse		= eth_header_parse,
+	.get_protocol   = eth_get_protocol,
 	.rebuild	= eth_rebuild_header,
 	.cache		= eth_header_cache,
 	.cache_update	= eth_header_cache_update,

net/packet/af_packet.c +15 -2

index a81dae8..492e580 100644
@@ -845,7 +845,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 
 	ph.raw = frame;
 
-	skb->protocol = proto;
 	skb->dev = dev;
 	skb->priority = po->sk.sk_priority;
 	skb->mark = po->sk.sk_mark;

@@ -924,6 +923,14 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		len = ((to_write > len_max) ? len_max : to_write);
 	}
 
+	if (proto == htons(ETH_P_ALL) && dev->header_ops->get_protocol) {
+		skb->protocol = dev->header_ops->get_protocol(dev, skb);
+		if (unlikely(skb->protocol == htons(ETH_P_ALL)))
+			return -EINVAL;
+	} else {
+		skb->protocol = proto;
+	}
+
 	return tp_len;
 }
 

@@ -1113,7 +1120,13 @@ static int packet_snd(struct socket *sock,
 	if (err)
 		goto out_free;
 
-	skb->protocol = proto;
+	if (proto == htons(ETH_P_ALL) && dev->header_ops->get_protocol) {
+		skb->protocol = dev->header_ops->get_protocol(dev, skb);
+		if (unlikely(skb->protocol == htons(ETH_P_ALL)))
+			return -EINVAL;
+	} else {
+		skb->protocol = proto;
+	}
 	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;

Signature · 2 lines

-- 
MST

Quoted 9 lines

From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 16:13:29 -0600

Quoted 2 lines

If SOCK_RAW packets are being sent, the protocol number is embedded in the packet data

Where exactly is that protocol value?  Not every link level protocol is ethernet.
We support FDDI, HIPPI, wireless, VLAN, PPP, and a host of others. So you can't know for sure unless you assume ethernet, which you can't do.

You are right.  This is TX though so the socket is bound to a specific device, and devices know which link protocol they use.  So we could add a get_protocol operation to each device type, and call it if the protocol passed is ETH_P_ALL to get the real one.

Like this: except I only added this for ethernet, and I would have to add this for the rest of device types. But I would like to hear what others think about this before I do the rest of the work for the rest of device types.

Does the below look sane?

net/ethernet/eth.c +0 -0

index 205a1c1..3e7761d 100644
@@ -231,6 +231,17 @@ int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr)
  EXPORT_SYMBOL(eth_header_parse);