lkml.org 
[lkml]   [1999]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: select()/socket has problems under 2.2.x.
On Sat, 6 Mar 1999, Andi Kleen wrote:

>> - if (skb_peek(&tp->out_of_order_queue) == NULL) {
>> + if (!skb_queue_len(&tp->out_of_order_queue)) {
>
>Trivial micro optimization that is not suitable for a stable kernel patch.

??

>Or what exactly do you want to archive with this patch, other than to
>remove one jump? (which itself is fine, but for 2.3, not 2.2)

If the skbuff-queue code is buggy you would have just noticed it in many
other part of the network code, no? I really don't see how this can be not
completly safe.

The reason that it's been used skb_peek == NULL instead of !skb_queue_len
is only because who wrote it didn't thought that skb_peek does something
more than only returning you if the queue is empty or not.

>
>> }
>> } else if (TCP_SKB_CB(skb)->ack_seq == tp->snd_una &&
>> (sock_rspace(sk) ||
>> - (!skb_queue_len(&sk->receive_queue) &&
>> - !skb_queue_len(&tp->out_of_order_queue)))) {
>> + !skb_queue_len(&sk->receive_queue))) {
>> /* Bulk data transfer: receiver */
>> __skb_pull(skb,th->doff*4);
>
>Still the unnecessary function call?
>
>!sock_rspace(sk) is equivalent to atomic_read(&rmem_alloc) <= sk->rcvbuf,
>except that it is much slower.

It produced cleaner code (more easy to read) according to me. But OK I
agree with you.

>I also think the change is wrong: such a deadlock you're trying to avoid
>here is unusual. Unusual things are not handled in the fast path. If you

I could agree to go in the slow path but it looks like the right place to
put it according to me. Every time you see that the rcvbuf is empty you
should also check that you are not going to deadlock before reject new
incoming packets.

So from a design point of view I see the rmem check _wrong_ if done alone.

>> If rcvbuf < skb->truesize you won't have way to receive something without
>> my patch. My changes allow always a packet to be queued in the receiver
>> and so it avoids deadlocking in the receiver.
>
>Which allows a DOS - the receiver could overrun the socket buffer easily.

I really can't see how this can open a window for a DoS attack. Really
please can you write an exploit for it? Overrunning the rcvbuf if the
queue is empty it's exactly what I wanted to achieve.

And you was just used to overrun the rcvbuf for a short time also in
2.2.2.

/*
* This doesn't check if the socket has enough room for the
packet.
* Either process the packet _without_ queueing it and then free
it,
* or do the check later.
*/
skb_set_owner_r(skb, sk);
^^^^^^^^^^^^^^^^^^^^^^^^ this was just overrunning it
if (sk->state == TCP_ESTABLISHED) { /* Fast path */
if (tcp_rcv_established(sk, skb, skb->h.th, skb->len))
goto reset;
return 0;
}

And btw it looks like to me that the right place to fix the deadlock is
just to change skb_set_owner_? and make it returning a value that tells us
if the packet is been discareded or accepted (and making it behave like
sock_rmalloc(), and if you look at the code you'll see that sock_rmalloc
behave correctly, because it looks if there is space in the rcvbuf _only_
before increasing rmem_alloc, that's the right design choice according to
me and the TCP stack is instead doing the wrong thing). But that's a more
complicated patch than the trivial (and according to me safe) one I just
developed.

>Better make sure that rcvbuf is never < skb->truesize (which has the
>advantage that it doesn't cost anything in critical paths too). Best would
>be to make sure that the rcvbuf of every socket is always > maxmtu_of_devices
>+ skb header size, but that is impractical. I think 4096 is good minimum value
>and the ATM and HIPPIE people can tune it.

What if I'll change the MTU during a transer? I just thought at the way
you are proposing and it simply doesn't look like the right way to fix the
bug to me.

Andrea Arcangeli


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:50    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site