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".
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
net/packet/af_packet.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e0516a2..a81dae8 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -958,7 +958,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
+ offsetof(struct sockaddr_ll,
sll_addr)))
goto out;
- ifindex = saddr->sll_ifindex;
+ ifindex = saddr->sll_ifindex ? : po->ifindex;
proto = saddr->sll_protocol;
addr = saddr->sll_addr;
}
@@ -1074,7 +1074,7 @@ static int packet_snd(struct socket *sock,
goto out;
if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
goto out;
- ifindex = saddr->sll_ifindex;
+ ifindex = saddr->sll_ifindex ? : po->ifindex;
proto = saddr->sll_protocol;
addr = saddr->sll_addr;
}
--
1.6.6.rc1.43.gf55cc
Le 05/01/2010 19:57, Michael S. Tsirkin a ?crit :
> 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 ?
On Tue, Jan 05, 2010 at 08:27:21PM +0100, Eric Dumazet wrote:
> Le 05/01/2010 19:57, Michael S. Tsirkin a ?crit :
> > 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 ?
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?
--
MST
On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
> 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?
Chris
From: "Michael S. Tsirkin" <[email protected]>
Date: Tue, 5 Jan 2010 22:50:40 +0200
> 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?
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? :-)
From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 15:28:22 -0600
> On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
>> 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?
This limits packet socket usage to only protocols the kernel is aware
of, defeating part of the usefulness of the packet socket facility.
On Tue, Jan 05, 2010 at 01:42:18PM -0800, David Miller wrote:
> From: "Chris Friesen" <[email protected]>
> Date: Tue, 05 Jan 2010 15:28:22 -0600
>
> > On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
> >> 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?
>
> 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?
--
MST
On Tue, Jan 05, 2010 at 01:40:38PM -0800, David Miller wrote:
> From: "Michael S. Tsirkin" <[email protected]>
> Date: Tue, 5 Jan 2010 22:50:40 +0200
>
> > 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?
>
> 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? :-)
At some level, of course I can. But I would have to do this
communication each time socket is bound to another device, as opposed to
passing the fd once. At least for me, option to autodetect protocol
would work even better though - it's what I do in the application
anyway.
--
MST
On Tue, Jan 05, 2010 at 11:45:24PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 05, 2010 at 01:42:18PM -0800, David Miller wrote:
> > From: "Chris Friesen" <[email protected]>
> > Date: Tue, 05 Jan 2010 15:28:22 -0600
> >
> > > On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
> > >> 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?
> >
> > 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?
Something like this (if we want to support this for all device types,
we would probably need to add get_proto callback to net_device_ops),
and call it instead of eth_hdr(skb)->h_proto.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a81dae8..a23d4b2 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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,11 @@ 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->type == ARPHRD_ETHER)
+ skb->protocol = eth_hdr(skb)->h_proto;
+ else
+ skb->protocol = proto;
+
return tp_len;
}
@@ -1113,7 +1117,10 @@ static int packet_snd(struct socket *sock,
if (err)
goto out_free;
- skb->protocol = proto;
+ if (proto == htons(ETH_P_ALL) && dev->type == ARPHRD_ETHER)
+ skb->protocol = eth_hdr(skb)->h_proto;
+ else
+ skb->protocol = proto;
skb->dev = dev;
skb->priority = sk->sk_priority;
skb->mark = sk->sk_mark;
On 01/05/2010 03:42 PM, David Miller wrote:
> From: "Chris Friesen" <[email protected]>
> Date: Tue, 05 Jan 2010 15:28:22 -0600
>
>> On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
>>> 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?
>
> 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
On Tue, Jan 05, 2010 at 04:13:29PM -0600, Chris Friesen wrote:
> On 01/05/2010 03:42 PM, David Miller wrote:
> > From: "Chris Friesen" <[email protected]>
> > Date: Tue, 05 Jan 2010 15:28:22 -0600
> >
> >> On 01/05/2010 12:57 PM, Michael S. Tsirkin wrote:
> >>> 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?
> >
> > 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
I wouldn't say kernel doesn't really need to do anything with them.
In fact I think currently protocol number affects whether other packet
sockets see the packet in question. There may be other cases, e.g. if
the device in question is a bridge, the packet gets reinjected in host
stack, so using a silly value for protocol field might interfere with
GRO etc.
--
MST
From: "Chris Friesen" <[email protected]>
Date: Tue, 05 Jan 2010 16:13:29 -0600
> 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.
On Tue, Jan 05, 2010 at 03:51:50PM -0800, David Miller wrote:
> From: "Chris Friesen" <[email protected]>
> Date: Tue, 05 Jan 2010 16:13:29 -0600
>
> > 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?
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a3fccc8..99d7801 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 205a1c1..3e7761d 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -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,
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a81dae8..492e580 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;
--
MST
On Wed, Jan 06, 2010 at 11:12:57AM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 05, 2010 at 03:51:50PM -0800, David Miller wrote:
> > From: "Chris Friesen" <[email protected]>
> > Date: Tue, 05 Jan 2010 16:13:29 -0600
> >
> > > 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?
I mean, besides the missing semicolons etc :)
Also, maybe it's a good idea to add a socket option that would trigger
this behaviour, just to make sure existing behavior stays unchanged
even if it's broken?
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a3fccc8..99d7801 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -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
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 205a1c1..3e7761d 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -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,
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a81dae8..492e580 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -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;
>
> --
> MST
2010/1/6 Michael S. Tsirkin <[email protected]>:
> On Tue, Jan 05, 2010 at 03:51:50PM -0800, David Miller wrote:
>> From: "Chris Friesen" <[email protected]>
>> Date: Tue, 05 Jan 2010 16:13:29 -0600
>> > 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?
>
[...]
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 205a1c1..3e7761d 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -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
> +}
[...]
You might want taking fragment of eth_type_trans() for this function
so that 802.2/IPX would be detected too.
Best Regards,
Micha? Miros?aw