New: DEADLOOP in kernel network module

http://bugzilla.kernel.org/show_bug.cgi?id=11058

           Summary: DEADLOOP in kernel network module
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.16.1
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: high
          Priority: P1
         Component: Netfilter/Iptables
        AssignedTo: [email protected]
        ReportedBy: [email protected]

Latest working kernel version:2.6.16.1
Earliest failing kernel version:2.6.16.1
Distribution:FC3
Hardware Environment:
Software Environment:

Problem Description:

We have seen a deadloop in softirq and we got the cause by creating and analysising the core dump of the kernel.I dont know where to submit a patch , so I report it here,wish you can apply it to furthur version of linux kernel.

The Cause:

Both ctnetlink_del_conntrack() and the tcp_packet() run the code below:

    if (del_timer(&ct->timeout))                /*deactive the timer*/
       ct->timeout.function((unsigned long) ct);/*remove conntrack from

conntrack table*/

the ctnetlink_del_conntrack() context is:
................

        if (cda[CTA_ID-1]) {
                u_int32_t id = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_ID-1]));
                if (ct->id != id) {
                        ip_conntrack_put(ct);
                        return -ENOENT;
                }
        }       

* if (del_timer(&ct->timeout))
* ct->timeout.function((unsigned long)ct);

        ip_conntrack_put(ct);

...........

the tcp_packet() context is:
...........

        case TCP_CONNTRACK_SYN_SENT:
                if (old_state < TCP_CONNTRACK_TIME_WAIT)
                        break;
                if ((conntrack->proto.tcp.seen[dir].flags &
                         IP_CT_TCP_FLAG_CLOSE_INIT)
                    || after(ntohl(th->seq),
                             conntrack->proto.tcp.seen[dir].td_end)) {  
                        /* Attempt to reopen a closed connection.
                        * Delete this connection and look up again. */
                        write_unlock_bh(&tcp_lock);
*                       if (del_timer(&conntrack->timeout))
*                               conntrack->timeout.function((unsigned long)
*                                                           conntrack);
                        return -NF_REPEAT;

......

How the DEADLOOP happened?

(1)in ctnetlink_del_conntrack()(runs in system call context): the del_timer
is called and then goes to timeout.function.
(2)before timeout.function finish excution(means the conntrack not
removed),an interrupt happens and a SYN packet of the same conntrack comes.CPU goes to irq handle and enventually runs tcp_packet().
(3)in tcp_packet() ,del_timer() will fail because the timer was
already deleted. the timeout.function in tcp_packet will not run, -NF_REPEAT is returned, the SYN packet will be passed back again.
(4)Neither side has the chance to run timeout.function,the
conntrack remains there,deadloop happen,the SYN packet will be passed back again and again.

The fix maybe,add lock the softirq when doing conntrack removing:

+++    local_bh_disable();
      if (del_timer(&ct->timeout))                /*deactive the timer*/
         ct->timeout.function((unsigned long) ct);/*remove conntrack from

conntrack table*/
+++ local_bh_enable();

Thanks, may this be helpful.
My email: [email protected]

It is hard to reproduce , but it really happen on our linux box.

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 70 lines · On Tue, 8 Jul 2008 20:13:20 -0700 (PDT) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=11058

Summary: DEADLOOP in kernel network module
...
Problem Description:

We have seen a deadloop in softirq and we got the cause by creating and analysising the core dump of the kernel.I dont know where to submit a patch , so I report it here,wish you can apply it to furthur version of linux kernel.

The Cause:

Both ctnetlink_del_conntrack() and the tcp_packet() run the code below:

    if (del_timer(&ct->timeout))                /*deactive the timer*/
       ct->timeout.function((unsigned long) ct);/*remove conntrack from

conntrack table*/

the ctnetlink_del_conntrack() context is:
................

        if (cda[CTA_ID-1]) {
                u_int32_t id = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_ID-1]));
                if (ct->id != id) {
                        ip_conntrack_put(ct);
                        return -ENOENT;
                }
        }       

* if (del_timer(&ct->timeout))
* ct->timeout.function((unsigned long)ct);

        ip_conntrack_put(ct);

...........

the tcp_packet() context is:
...........

        case TCP_CONNTRACK_SYN_SENT:
                if (old_state < TCP_CONNTRACK_TIME_WAIT)
                        break;
                if ((conntrack->proto.tcp.seen[dir].flags &
                         IP_CT_TCP_FLAG_CLOSE_INIT)
                    || after(ntohl(th->seq),
                             conntrack->proto.tcp.seen[dir].td_end)) {  
                        /* Attempt to reopen a closed connection.
                        * Delete this connection and look up again. */
                        write_unlock_bh(&tcp_lock);
*                       if (del_timer(&conntrack->timeout))
*                               conntrack->timeout.function((unsigned long)
*                                                           conntrack);
                        return -NF_REPEAT;

......

How the DEADLOOP happened?

(1)in ctnetlink_del_conntrack()(runs in system call context): the del_timer
is called and then goes to timeout.function.
(2)before timeout.function finish excution(means the conntrack not
removed),an interrupt happens and a SYN packet of the same conntrack comes.CPU goes to irq handle and enventually runs tcp_packet().
(3)in tcp_packet() ,del_timer() will fail because the timer was
already deleted. the timeout.function in tcp_packet will not run, -NF_REPEAT is returned, the SYN packet will be passed back again.
(4)Neither side has the chance to run timeout.function,the
conntrack remains there,deadloop happen,the SYN packet will be passed back again and again.

The fix maybe,add lock the softirq when doing conntrack removing:

+++    local_bh_disable();
      if (del_timer(&ct->timeout))                /*deactive the timer*/
         ct->timeout.function((unsigned long) ct);/*remove conntrack from

Quoted 8 lines

conntrack table*/
+++ local_bh_enable();

Thanks, may this be helpful.
My email: [email protected]

It is hard to reproduce , but it really happen on our linux box.

Thanks.

Please submit patches via email as described in Documentation/SubmittingPatches. The file ./MAINTAINERS can be used to determine which individuals and mailing lists the patch should be sent to.

But that's for next time - this patch is small enough for the netfilter developers to be able to type in again ;)

Quoted 9 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 3 lines · On Tue, 8 Jul 2008 20:13:20 -0700 (PDT) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=11058

Summary: DEADLOOP in kernel network module

Quoted 30 lines

Quoted 19 lines

How the DEADLOOP happened?

(1)in ctnetlink_del_conntrack()(runs in system call context): the
del_timer
is called and then goes to timeout.function.
(2)before timeout.function finish excution(means the conntrack not
removed),an interrupt happens and a SYN packet of the same conntrack comes.CPU goes to irq handle and enventually runs tcp_packet().
(3)in tcp_packet() ,del_timer() will fail because the timer was
already deleted. the timeout.function in tcp_packet will not run, -NF_REPEAT is returned, the SYN packet will be passed back again.
(4)Neither side has the chance to run timeout.function,the
conntrack remains there,deadloop happen,the SYN packet will be passed back again and again.

The fix maybe,add lock the softirq when doing conntrack removing:

+++    local_bh_disable();
      if (del_timer(&ct->timeout))                /*deactive the timer*/
         ct->timeout.function((unsigned long) ct);/*remove conntrack from

Quoted 8 lines

conntrack table*/
+++ local_bh_enable();

Thanks, may this be helpful.
My email: [email protected]

It is hard to reproduce , but it really happen on our linux box.

Thanks.

Quoted 6 lines

Please submit patches via email as described in Documentation/SubmittingPatches. The file ./MAINTAINERS can be used to determine which individuals and mailing lists the patch should be sent to.

But that's for next time - this patch is small enough for the netfilter developers to be able to type in again ;)

Good catch, thanks. Basically all del_timer()/timeout.function calls in conntrack can happen in process context, so we'd have to disable BHs every time we do this. I think this fix should also work. The only spot where we return NF_REPEAT is in TCP conntrack, so we can simply make sure we only do this if we actually managed to kill the connection.

Jozsef, what do you think?

Quoted 7 lines · Patrick McHardy

Good catch, thanks. Basically all del_timer()/timeout.function calls in conntrack can happen in process context, so we'd have to disable BHs every time we do this. I think this fix should also work. The only spot where we return NF_REPEAT is in TCP conntrack, so we can simply make sure we only do this if we actually managed to kill the connection.

Quoted 1 line

Jozsef, what do you think?

I agree with you completely - and nice catch, indeed! Your proposed patch looks just fine.

Quoted 9 lines · Jozsef Kadlecsik

Quoted 7 lines · Patrick McHardy

Good catch, thanks. Basically all del_timer()/timeout.function calls in conntrack can happen in process context, so we'd have to disable BHs every time we do this. I think this fix should also work. The only spot where we return NF_REPEAT is in TCP conntrack, so we can simply make sure we only do this if we actually managed to kill the connection.

Quoted 3 lines

Quoted 1 line

Jozsef, what do you think?

I agree with you completely - and nice catch, indeed! Your proposed patch looks just fine.

Thanks, I'll send a backport for 2.6.26 to Dave tonight.