| Message ID | 20100105185732.GA30346@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Michael S. Tsirkin" <mst@redhat.com> 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? :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Chris Friesen" <cfriesen@nortel.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 05, 2010 at 01:42:18PM -0800, David Miller wrote: > From: "Chris Friesen" <cfriesen@nortel.com> > 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?
On Tue, Jan 05, 2010 at 01:40:38PM -0800, David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > 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.
On 01/05/2010 03:42 PM, David Miller wrote: > From: "Chris Friesen" <cfriesen@nortel.com> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" <cfriesen@nortel.com> > > 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.
From: "Chris Friesen" <cfriesen@nortel.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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 <mst@redhat.com> --- net/packet/af_packet.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)