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    [W:0.026 / U:30.028 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site