Message ID | 1503419968.2499.33.camel@edumazet-glaptop3.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2017-08-22 at 09:39 -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > When peeking, if a bad csum is discovered, the skb is unlinked from > the queue with __sk_queue_drop_skb and the peek operation restarted. > > __sk_queue_drop_skb only drops packets that match the queue head. > > This fails if the skb was found after the head, using SO_PEEK_OFF > socket option. This causes an infinite loop. > > We MUST drop this problematic skb, and we can simply check if skb was > already removed by another thread, by looking at skb->next : > > This pointer is set to NULL by the __skb_unlink() operation, that might > have happened only under the spinlock protection. > > Many thanks to syzkaller team (and particularly Dmitry Vyukov who > provided us nice C reproducers exhibiting the lockup) and Willem de > Bruijn who provided first version for this patch and a test program. > > Fixes: 627d2d6b5500 ("udp: enable MSG_PEEK at non-zero offset") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Willem de Bruijn <willemb@google.com> > --- > net/core/datagram.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index a21ca8dee5ea..8c2f4489ff8f 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -362,7 +362,7 @@ int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, > if (flags & MSG_PEEK) { > err = -ENOENT; > spin_lock_bh(&sk_queue->lock); > - if (skb == skb_peek(sk_queue)) { > + if (skb->next) { > __skb_unlink(skb, sk_queue); > refcount_dec(&skb->users); > if (destructor) > This version is really nice! Acked-by: Paolo Abeni <pabeni@redhat.com>
On Tue, Aug 22, 2017 at 12:47 PM, Paolo Abeni <pabeni@redhat.com> wrote: > On Tue, 2017-08-22 at 09:39 -0700, Eric Dumazet wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> When peeking, if a bad csum is discovered, the skb is unlinked from >> the queue with __sk_queue_drop_skb and the peek operation restarted. >> >> __sk_queue_drop_skb only drops packets that match the queue head. >> >> This fails if the skb was found after the head, using SO_PEEK_OFF >> socket option. This causes an infinite loop. >> >> We MUST drop this problematic skb, and we can simply check if skb was >> already removed by another thread, by looking at skb->next : >> >> This pointer is set to NULL by the __skb_unlink() operation, that might >> have happened only under the spinlock protection. >> >> Many thanks to syzkaller team (and particularly Dmitry Vyukov who >> provided us nice C reproducers exhibiting the lockup) and Willem de >> Bruijn who provided first version for this patch and a test program. >> >> Fixes: 627d2d6b5500 ("udp: enable MSG_PEEK at non-zero offset") >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Cc: Willem de Bruijn <willemb@google.com> >> --- >> net/core/datagram.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/datagram.c b/net/core/datagram.c >> index a21ca8dee5ea..8c2f4489ff8f 100644 >> --- a/net/core/datagram.c >> +++ b/net/core/datagram.c >> @@ -362,7 +362,7 @@ int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, >> if (flags & MSG_PEEK) { >> err = -ENOENT; >> spin_lock_bh(&sk_queue->lock); >> - if (skb == skb_peek(sk_queue)) { >> + if (skb->next) { >> __skb_unlink(skb, sk_queue); >> refcount_dec(&skb->users); >> if (destructor) >> > > This version is really nice! It is :) Thanks, Eric! Acked-by: Willem de Bruijn <willemb@google.com>
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 22 Aug 2017 09:39:28 -0700 > From: Eric Dumazet <edumazet@google.com> > > When peeking, if a bad csum is discovered, the skb is unlinked from > the queue with __sk_queue_drop_skb and the peek operation restarted. > > __sk_queue_drop_skb only drops packets that match the queue head. > > This fails if the skb was found after the head, using SO_PEEK_OFF > socket option. This causes an infinite loop. > > We MUST drop this problematic skb, and we can simply check if skb was > already removed by another thread, by looking at skb->next : > > This pointer is set to NULL by the __skb_unlink() operation, that might > have happened only under the spinlock protection. > > Many thanks to syzkaller team (and particularly Dmitry Vyukov who > provided us nice C reproducers exhibiting the lockup) and Willem de > Bruijn who provided first version for this patch and a test program. > > Fixes: 627d2d6b5500 ("udp: enable MSG_PEEK at non-zero offset") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> Applied and queued up for -stable.
diff --git a/net/core/datagram.c b/net/core/datagram.c index a21ca8dee5ea..8c2f4489ff8f 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -362,7 +362,7 @@ int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, if (flags & MSG_PEEK) { err = -ENOENT; spin_lock_bh(&sk_queue->lock); - if (skb == skb_peek(sk_queue)) { + if (skb->next) { __skb_unlink(skb, sk_queue); refcount_dec(&skb->users); if (destructor)