| Message ID | 1324474811.2728.61.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet wrote: > Le mercredi 21 décembre 2011 à 14:28 +0100, Michal Simek a écrit : > >> ok. Can you provide me any background why size should be setup by >> size = SKB_WITH_OVERHEAD(ksize(data)); >> and not to use size which is passed to kmalloc in __alloc_skb. > > Its all about memory accounting (based on skb->truesize) > > Prior to the patch, we could fool memory accounting because skbs claimed > to use less memory than what they really used. > > And crash machines eventually. > > Now memory accouting is fixed, we probably need to change some points in > the kernel, where we previously accepted a small skb, but not a very > large one. > > Since "ping" probably uses SOCK_RAW sockets, I'll try this one : > > (We dont care of _this_ skb truesize, only on the count of previously > queued packets) > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 0da505c..a809a48 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > if (snaplen > res) > snaplen = res; > > - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= > - (unsigned)sk->sk_rcvbuf) > + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) > goto drop_n_acct; > > if (skb_shared(skb)) { > @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, > if (po->tp_version <= TPACKET_V2) { > if (macoff + snaplen > po->rx_ring.frame_size) { > if (po->copy_thresh && > - atomic_read(&sk->sk_rmem_alloc) + skb->truesize > + atomic_read(&sk->sk_rmem_alloc) > < (unsigned)sk->sk_rcvbuf) { > if (skb_shared(skb)) { > copy_skb = skb_clone(skb, GFP_ATOMIC); > > > > It doesn't work too. It is possible to see this behavior on qemu. What about if I prepare you package with cross toolchain, rootfs and you can add debug message where you want? I have also tried ll_temac driver with ppc440 and behavior is the same. Max FRAME_SIZE pass to netdev_alloc_skb_ip_align is 7966. For this value ping works. (For ll_temac driver it is #define XTE_JUMBO_MTU 7948 from ll_temac.h) Thanks, Michal
Le mercredi 21 décembre 2011 à 15:24 +0100, Michal Simek a écrit : > Eric Dumazet wrote: > > Le mercredi 21 décembre 2011 à 14:28 +0100, Michal Simek a écrit : > > > >> ok. Can you provide me any background why size should be setup by > >> size = SKB_WITH_OVERHEAD(ksize(data)); > >> and not to use size which is passed to kmalloc in __alloc_skb. > > > > Its all about memory accounting (based on skb->truesize) > > > > Prior to the patch, we could fool memory accounting because skbs claimed > > to use less memory than what they really used. > > > > And crash machines eventually. > > > > Now memory accouting is fixed, we probably need to change some points in > > the kernel, where we previously accepted a small skb, but not a very > > large one. > > > > Since "ping" probably uses SOCK_RAW sockets, I'll try this one : > > > > (We dont care of _this_ skb truesize, only on the count of previously > > queued packets) > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 0da505c..a809a48 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > > if (snaplen > res) > > snaplen = res; > > > > - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= > > - (unsigned)sk->sk_rcvbuf) > > + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) > > goto drop_n_acct; > > > > if (skb_shared(skb)) { > > @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, > > if (po->tp_version <= TPACKET_V2) { > > if (macoff + snaplen > po->rx_ring.frame_size) { > > if (po->copy_thresh && > > - atomic_read(&sk->sk_rmem_alloc) + skb->truesize > > + atomic_read(&sk->sk_rmem_alloc) > > < (unsigned)sk->sk_rcvbuf) { > > if (skb_shared(skb)) { > > copy_skb = skb_clone(skb, GFP_ATOMIC); > > > > > > > > > > It doesn't work too. > It is possible to see this behavior on qemu. What about if I prepare you package with cross toolchain, rootfs > and you can add debug message where you want? > > I have also tried ll_temac driver with ppc440 and behavior is the same. > > Max FRAME_SIZE pass to netdev_alloc_skb_ip_align is 7966. For this value ping works. > (For ll_temac driver it is #define XTE_JUMBO_MTU 7948 from ll_temac.h) I did several tests with MTU 9000 on my machines and it works well. It seems my pings (iputils-sss20071127 or iputils-sss20101006) uses a big enough RCVBUF setsockopt(3, SOL_SOCKET, SO_RCVBUF, [65536], 4) = 0 getsockopt(3, SOL_SOCKET, SO_RCVBUF, [131072], [4]) = 0 Could you check with "strace ping..." what is doing busybox ? -- 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
Le mercredi 21 décembre 2011 à 16:30 +0100, Eric Dumazet a écrit : > Le mercredi 21 décembre 2011 à 15:24 +0100, Michal Simek a écrit : > > Eric Dumazet wrote: > > > Le mercredi 21 décembre 2011 à 14:28 +0100, Michal Simek a écrit : > > > > > >> ok. Can you provide me any background why size should be setup by > > >> size = SKB_WITH_OVERHEAD(ksize(data)); > > >> and not to use size which is passed to kmalloc in __alloc_skb. > > > > > > Its all about memory accounting (based on skb->truesize) > > > > > > Prior to the patch, we could fool memory accounting because skbs claimed > > > to use less memory than what they really used. > > > > > > And crash machines eventually. > > > > > > Now memory accouting is fixed, we probably need to change some points in > > > the kernel, where we previously accepted a small skb, but not a very > > > large one. > > > > > > Since "ping" probably uses SOCK_RAW sockets, I'll try this one : > > > > > > (We dont care of _this_ skb truesize, only on the count of previously > > > queued packets) > > > > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > > index 0da505c..a809a48 100644 > > > --- a/net/packet/af_packet.c > > > +++ b/net/packet/af_packet.c > > > @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > > > if (snaplen > res) > > > snaplen = res; > > > > > > - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= > > > - (unsigned)sk->sk_rcvbuf) > > > + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) > > > goto drop_n_acct; > > > > > > if (skb_shared(skb)) { > > > @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, > > > if (po->tp_version <= TPACKET_V2) { > > > if (macoff + snaplen > po->rx_ring.frame_size) { > > > if (po->copy_thresh && > > > - atomic_read(&sk->sk_rmem_alloc) + skb->truesize > > > + atomic_read(&sk->sk_rmem_alloc) > > > < (unsigned)sk->sk_rcvbuf) { > > > if (skb_shared(skb)) { > > > copy_skb = skb_clone(skb, GFP_ATOMIC); > > > > > > > > > > > > > > > > It doesn't work too. > > It is possible to see this behavior on qemu. What about if I prepare you package with cross toolchain, rootfs > > and you can add debug message where you want? > > > > I have also tried ll_temac driver with ppc440 and behavior is the same. > > > > Max FRAME_SIZE pass to netdev_alloc_skb_ip_align is 7966. For this value ping works. > > (For ll_temac driver it is #define XTE_JUMBO_MTU 7948 from ll_temac.h) > > I did several tests with MTU 9000 on my machines and it works well. > > It seems my pings (iputils-sss20071127 or iputils-sss20101006) uses a > big enough RCVBUF > > setsockopt(3, SOL_SOCKET, SO_RCVBUF, [65536], 4) = 0 > getsockopt(3, SOL_SOCKET, SO_RCVBUF, [131072], [4]) = 0 > > Could you check with "strace ping..." what is doing busybox ? I found it : Its too small for jumbo frames. setsockopt(3, SOL_SOCKET, SO_RCVBUF, [7280], 4) = 0 networking/ping.c /* set recv buf (needed if we can get lots of responses: flood ping, * broadcast ping etc) */ sockopt = (datalen * 2) + 7 * 1024; /* giving it a bit of extra room */ setsockopt(pingsock, SOL_SOCKET, SO_RCVBUF, &sockopt, sizeof(sockopt)); -- 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
Le mercredi 21 décembre 2011 à 15:24 +0100, Michal Simek a écrit : > Eric Dumazet wrote: > > Le mercredi 21 décembre 2011 à 14:28 +0100, Michal Simek a écrit : > > > >> ok. Can you provide me any background why size should be setup by > >> size = SKB_WITH_OVERHEAD(ksize(data)); > >> and not to use size which is passed to kmalloc in __alloc_skb. > > > > Its all about memory accounting (based on skb->truesize) > > > > Prior to the patch, we could fool memory accounting because skbs claimed > > to use less memory than what they really used. > > > > And crash machines eventually. > > > > Now memory accouting is fixed, we probably need to change some points in > > the kernel, where we previously accepted a small skb, but not a very > > large one. > > > > Since "ping" probably uses SOCK_RAW sockets, I'll try this one : > > > > (We dont care of _this_ skb truesize, only on the count of previously > > queued packets) > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 0da505c..a809a48 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > > if (snaplen > res) > > snaplen = res; > > > > - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= > > - (unsigned)sk->sk_rcvbuf) > > + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) > > goto drop_n_acct; > > > > if (skb_shared(skb)) { > > @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, > > if (po->tp_version <= TPACKET_V2) { > > if (macoff + snaplen > po->rx_ring.frame_size) { > > if (po->copy_thresh && > > - atomic_read(&sk->sk_rmem_alloc) + skb->truesize > > + atomic_read(&sk->sk_rmem_alloc) > > < (unsigned)sk->sk_rcvbuf) { > > if (skb_shared(skb)) { > > copy_skb = skb_clone(skb, GFP_ATOMIC); > > > > > > > > > > It doesn't work too. > It is possible to see this behavior on qemu. What about if I prepare you package with cross toolchain, rootfs > and you can add debug message where you want? > > I have also tried ll_temac driver with ppc440 and behavior is the same. > > Max FRAME_SIZE pass to netdev_alloc_skb_ip_align is 7966. For this value ping works. > (For ll_temac driver it is #define XTE_JUMBO_MTU 7948 from ll_temac.h) > I wonder if you applied/tested my patch correctly, since it really should had help in your case (allowing first received packet to be queued, even if very big) Given the way NIC drivers work (pre-allocating big buffers before hardware fill frames in them), SO_RCVBUF limits are difficult to respect. We should allow at least one frame, even with a very small SO_RCVBUF setting, or silently cap a minimum sane value. -- 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 Wed, 2011-12-21 at 16:44 +0100, Eric Dumazet wrote: > Le mercredi 21 décembre 2011 à 16:30 +0100, Eric Dumazet a écrit : > > Le mercredi 21 décembre 2011 à 15:24 +0100, Michal Simek a écrit : > > > Eric Dumazet wrote: > > > > Le mercredi 21 décembre 2011 à 14:28 +0100, Michal Simek a écrit : > > > > > > > >> ok. Can you provide me any background why size should be setup by > > > >> size = SKB_WITH_OVERHEAD(ksize(data)); > > > >> and not to use size which is passed to kmalloc in __alloc_skb. > > > > > > > > Its all about memory accounting (based on skb->truesize) > > > > > > > > Prior to the patch, we could fool memory accounting because skbs claimed > > > > to use less memory than what they really used. > > > > > > > > And crash machines eventually. > > > > > > > > Now memory accouting is fixed, we probably need to change some points in > > > > the kernel, where we previously accepted a small skb, but not a very > > > > large one. > > > > > > > > Since "ping" probably uses SOCK_RAW sockets, I'll try this one : > > > > > > > > (We dont care of _this_ skb truesize, only on the count of previously > > > > queued packets) > > > > > > > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > > > index 0da505c..a809a48 100644 > > > > --- a/net/packet/af_packet.c > > > > +++ b/net/packet/af_packet.c > > > > @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > > > > if (snaplen > res) > > > > snaplen = res; > > > > > > > > - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= > > > > - (unsigned)sk->sk_rcvbuf) > > > > + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) > > > > goto drop_n_acct; > > > > > > > > if (skb_shared(skb)) { > > > > @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, > > > > if (po->tp_version <= TPACKET_V2) { > > > > if (macoff + snaplen > po->rx_ring.frame_size) { > > > > if (po->copy_thresh && > > > > - atomic_read(&sk->sk_rmem_alloc) + skb->truesize > > > > + atomic_read(&sk->sk_rmem_alloc) > > > > < (unsigned)sk->sk_rcvbuf) { > > > > if (skb_shared(skb)) { > > > > copy_skb = skb_clone(skb, GFP_ATOMIC); > > > > > > > > > > > > > > > > > > > > > > It doesn't work too. > > > It is possible to see this behavior on qemu. What about if I prepare you package with cross toolchain, rootfs > > > and you can add debug message where you want? > > > > > > I have also tried ll_temac driver with ppc440 and behavior is the same. > > > > > > Max FRAME_SIZE pass to netdev_alloc_skb_ip_align is 7966. For this value ping works. > > > (For ll_temac driver it is #define XTE_JUMBO_MTU 7948 from ll_temac.h) > > > > I did several tests with MTU 9000 on my machines and it works well. > > > > It seems my pings (iputils-sss20071127 or iputils-sss20101006) uses a > > big enough RCVBUF > > > > setsockopt(3, SOL_SOCKET, SO_RCVBUF, [65536], 4) = 0 > > getsockopt(3, SOL_SOCKET, SO_RCVBUF, [131072], [4]) = 0 > > > > Could you check with "strace ping..." what is doing busybox ? > > I found it : Its too small for jumbo frames. > > setsockopt(3, SOL_SOCKET, SO_RCVBUF, [7280], 4) = 0 > > networking/ping.c > > > /* set recv buf (needed if we can get lots of responses: flood ping, > * broadcast ping etc) */ > sockopt = (datalen * 2) + 7 * 1024; /* giving it a bit of extra room */ > setsockopt(pingsock, SOL_SOCKET, SO_RCVBUF, &sockopt, sizeof(sockopt)); > > > > -- Why receive buffer size MUST bigger than the jumbo frames size even if the packet size is small? > 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 -- 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
Le jeudi 22 décembre 2011 à 00:01 +0800, Jun Zhao a écrit : > Why receive buffer size MUST bigger than the jumbo frames size even if > the packet size is small? Thats the way it is in linux. We count the memory size used by the packet. If not, a malicious attacker could send 1-byte frames and exhaust your kernel memory. If you want to reduce it, you might use copybreak : Some drivers copy the data into a small skb instead of providing a jumbo frame to upper stack. tg3 for example. -- 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 Wed, 2011-12-21 at 17:05 +0100, Eric Dumazet wrote: > Le jeudi 22 décembre 2011 à 00:01 +0800, Jun Zhao a écrit : > > > Why receive buffer size MUST bigger than the jumbo frames size even if > > the packet size is small? > > Thats the way it is in linux. > > We count the memory size used by the packet. > > If not, a malicious attacker could send 1-byte frames and exhaust your > kernel memory. > > If you want to reduce it, you might use copybreak : Some drivers copy > the data into a small skb instead of providing a jumbo frame to upper > stack. > > tg3 for example. > > > Eric Dumazet: I got it, thanks for you explanation. -- 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
> If not, a malicious attacker could send 1-byte frames and exhaust your > kernel memory. That used to happen in SVR4 - it was possible for single byte TCP (etc) data to be queued at a STREAM head (which counted actual data bytes) in a 2k message block. Actually we also managed to use all kernel memory queueing zero sized STREAMS messages. > If you want to reduce it, you might use copybreak : Some drivers copy > the data into a small skb instead of providing a jumbo frame to upper > stack. Many, many moons ago I wrote an ethernet driver that received into an array of 128 (mostly) 512byte buffers. Full sized frames would have multiple rx ring entries, but could almost always be copied into a correctly sized buffer with a single aligned copy (cache-line aligned if useful). This was significantly faster than other schemes - especially on systems where the iommu needed setting to allow the ethernet hardware to access memory. I don't know if the linux skb buffers would allow the ethernet driver to use (say) 1600 byte rx buffers, and link them together when a long frame arrives. Most ethernet HW I've seen will fragment long receives. The painful hardware is that which enforces a 4n byte alignment on the rx buffer! - on systens that can't do misaligned accesses. 2 bytes of junk would be fine! David -- 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
Le mercredi 21 décembre 2011 à 16:39 +0000, David Laight a écrit : > Many, many moons ago I wrote an ethernet driver that received into > an array of 128 (mostly) 512byte buffers. Full sized frames would have > multiple rx ring entries, but could almost always be copied into a > correctly > sized buffer with a single aligned copy (cache-line aligned if useful). > This was significantly faster than other schemes - especially > on systems where the iommu needed setting to allow the ethernet > hardware to access memory. > > I don't know if the linux skb buffers would allow the ethernet > driver to use (say) 1600 byte rx buffers, and link them together > when a long frame arrives. Most ethernet HW I've seen will > fragment long receives. > > The painful hardware is that which enforces a 4n byte alignment > on the rx buffer! - on systens that can't do misaligned accesses. > 2 bytes of junk would be fine! It all depends on hardware capabilities. Old hardware required a single area per frame. So driver had to talk to the hardware the same way. We now have hardware able to split data into several frags to reduce the overhead. (Even using different pools to get a low number of frags, and good filling ratio) Take a look at NIU for example. -- 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 0da505c..a809a48 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1631,8 +1631,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, if (snaplen > res) snaplen = res; - if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >= - (unsigned)sk->sk_rcvbuf) + if (atomic_read(&sk->sk_rmem_alloc) >= (unsigned)sk->sk_rcvbuf) goto drop_n_acct; if (skb_shared(skb)) { @@ -1763,7 +1762,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, if (po->tp_version <= TPACKET_V2) { if (macoff + snaplen > po->rx_ring.frame_size) { if (po->copy_thresh && - atomic_read(&sk->sk_rmem_alloc) + skb->truesize + atomic_read(&sk->sk_rmem_alloc) < (unsigned)sk->sk_rcvbuf) { if (skb_shared(skb)) { copy_skb = skb_clone(skb, GFP_ATOMIC);