diff mbox

commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping

Message ID CACSApvb_CtFEenaSGcpUk6yb8O3X95MAULkJ-+T1hscCE_O_jg@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Soheil Hassas Yeganeh June 1, 2017, 4:42 p.m. UTC 
On Thu, Jun 1, 2017 at 11:36 AM, Cyril Hrubis <chrubis@suse.cz> wrote:
> It seems to repeatedly produce (until I plug the cable back):
>
> ee_errno = 113 ee_origin = 2 ee_type = 3 ee_code = 1 ee_info = 0 ee_data = 0
>
> So we get EHOSTUNREACH on SO_EE_ORIGIN_ICMP.

Thank you very much! I have a wild guess that, when we
have a train of skbs on the error queue starting from a local error,
we will see this issue.

Ping (without my patch) considers EAGAIN on a normal read as an
indication that there is nothing on the error queue, but that's a
flawed assumption.

Would you mind trying another shot in the darkness please? Thanks!

Comments

Cyril Hrubis June 1, 2017, 8:03 p.m. UTC | #1 
Hi!
> Thank you very much! I have a wild guess that, when we
> have a train of skbs on the error queue starting from a local error,
> we will see this issue.
> 
> Ping (without my patch) considers EAGAIN on a normal read as an
> indication that there is nothing on the error queue, but that's a
> flawed assumption.
> 
> Would you mind trying another shot in the darkness please? Thanks!

This patch seems to fix the issue, I've tried several times and poll()
just timeouts, haven't seen a single POLLERR in the ping strace.

You can add my Tested-by: for this patch as well :-).
diff mbox

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5a726161f4e4..097152a03c74 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3742,7 +3742,8 @@  EXPORT_SYMBOL(sock_queue_err_skb);
 static bool is_icmp_err_skb(const struct sk_buff *skb)
 {
        return skb && (SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
-                      SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP6);
+                      SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP6 ||
+                      SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_LOCAL);
 }

 struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
@@ -3751,14 +3752,19 @@  struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
        struct sk_buff *skb, *skb_next = NULL;
        bool icmp_next = false;
        unsigned long flags;
+       int err = 0;

        spin_lock_irqsave(&q->lock, flags);
        skb = __skb_dequeue(q);
-       if (skb && (skb_next = skb_peek(q)))
+       if (skb && (skb_next = skb_peek(q))) {
                icmp_next = is_icmp_err_skb(skb_next);
+               err = SKB_EXT_ERR(skb_next)->ee.ee_origin;
+       }
        spin_unlock_irqrestore(&q->lock, flags);

-       if (is_icmp_err_skb(skb) && !icmp_next)
+       if (icmp_next)
+               sk->sk_err = err;
+       else if (is_icmp_err_skb(skb) && !icmp_next)
                sk->sk_err = 0;

        if (skb_next)