System crash in tcp_fragment()

System crash in tcp_fragment()

[george anzinger]

I wonder if you could help me squash a bug in the tcp code. 
Here is what we know thus far:

An SMP (x386 dual) 2.4.17 kernel crashes with an attempt to
deference NULL at the end of tcp_fragment() (in
net/ipv4/tcp_output.c) while attempting to link in the newly
created fragment.  
The bugzilla report is:


http://www.telecomlinux.org/bugzilla/show_bug.cgi?id=503

Incase you can not see this, it appears that the addresses
of each skb are alright, so the assumption is that the skb
passed to tcp_fragment() has been unlinked while
tcp_fragment() was doing its thing.  This implies a need for
locking at some higher level and we don't know enough about
the tcp code to divine where this might best be done.

Here is the call stack:

Panic screen:
    <1>Unable to handle kernel NULL pointer deference at
virtual address 
00000004
    <4> printing eip:
    <4>c0256fb2
    <1>*pde = 00000000
    <4>Oops: 0002
    <4>CPU:    1
    <4>EIP:    0010:[<c0256fb2>]    Not tainted
    <4>EFLAGS: 00010296
    <4>eax: 00000000   ebx: c4d3ada0   ecx: c4d3ada0   edx:
00000000
    <4>esi: c4e60780   edi: 000005a8   ebp: 00000610   esp:
c1219e78
    <4>ds: 0018   es: 0018   ss: 0018
    <4>Process swapper (pid: 0, stackpage=c1219000)
    <4>Stack: c4c84478 00000064 c88937cd 00006270 00000010
c4e60780 c4c84478 
000005a8 
    <4>       000005a8 c025787f c4c843a0 c4e60780 000005a8
c4c843a0 c4c84478 
c4c843a0 
    <4>       004bd6a9 c0259a32 c4c843a0 c4e60780 c4c843a0
00000000 c1219ee8 
00004050 
    <4>Call Trace: [<c88937cd>] [<c025787f>] [<c0259a32>]
[<c01bedc5>] 
[<c0259c36>] 
    <4>   [<c0128d6a>] [<c0259b50>] [<c0128e6d>]
[<c01246fb>] [<c0109604>] 
[<c0105490>] 
    <4>   [<c0105490>] [<c0105490>] [<c01054bc>]
[<c0105542>] [<c011d3db>] 
[<c011d76d>] 
    <4>
    <4>Code: 89 5a 04 89 1e 89 43 08 ff 40 08 31 c0 83 c4 14
5b 5e 5f 5d 
    <1>Dumping from interrupt handler !
    <1>Uncertain scenario - but will try my best
    <4>
    <4>dump: Dumping to device 0x806 [sd(8,6)] on CPU 1 ...
    <4>dump: Compression value is 0x0, Writing dump header 
    <4>
    <4>dump: Pass 1: Saving Reserved Pages: 
    <4>dump: Memory Bank[0]: 0 ... 7feffff: 
    [...]

lcrash backtrace:
>> bt
================================================================
STACK TRACE FOR TASK: 0xc1218000(swapper)

 0 tcp_fragment+674 [0xc0256fb2]
 1 tcp_retransmit_skb+170 [0xc025787a]
 2 tcp_retransmit_timer+493 [0xc0259a2d]
 3 tcp_write_timer+225 [0xc0259c31]
 4 timer_bh+710 [0xc0128d66]
 5 timer_softirq+40 [0xc0128e68]
 6 do_softirq+185 [0xc01246f9]
 7 do_IRQ+511 [0xc01095ff]
 8 do_IRQ+511 [0xc01095ff]
TRACE ERROR 0x1
================================================================

We assumed that this might be related to preempt code in the
kernel, however, this now appears unlikely.  The primary
reason for preempt related failures is the use of
unprotected "cpu ids" to access "per cpu" data structures. 
To this end we have made changes to the "skb" management
code to include the smp_processor_id() calls in the relevant
interrupt off areas, however, this problem does not seem to
have any such issues.

Is is possible for the other cpu (or even this one given the
ksoftirqd stuff) to remove or alter the skb that
tcp_fragment() is processing?  What locks, if any, are
needed to prevent this.
-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[Andi Kleen]

> Incase you can not see this, it appears that the addresses
> of each skb are alright, so the assumption is that the skb
> passed to tcp_fragment() has been unlinked while
> tcp_fragment() was doing its thing.  This implies a need for
> locking at some higher level and we don't know enough about
> the tcp code to divine where this might best be done.

2.4 TCP should in theory already have enough locking to prevent this
(the socket lock that is aquired by timers and user context socket users) 

-Andi

Re: System crash in tcp_fragment()

[george anzinger]

Andi Kleen wrote:
> 
> > Incase you can not see this, it appears that the addresses
> > of each skb are alright, so the assumption is that the skb
> > passed to tcp_fragment() has been unlinked while
> > tcp_fragment() was doing its thing.  This implies a need for
> > locking at some higher level and we don't know enough about
> > the tcp code to divine where this might best be done.
> 
> 2.4 TCP should in theory already have enough locking to prevent this
> (the socket lock that is aquired by timers and user context socket users)
> 
> -Andi
Here is another oops, not quite the same, AND with an assert
failure ahead of it.  I append the whole report and some and
some observations:


We had two more panics over the weekend.
Here is the analysis from one of them.

---------comments from Dave Howell--------------
Looking at the sysint4l dump, some observations:
- Panic was due to an Oops (Null pointer dereference kernel
incident)
- Full system configuration is in kernel startup logs
(memory, disks, chipsets, 
etc)
- Last part of kernel log has oops info, follows kernel
assertion failed 
warning:
<4>KERNRL: assertion (atomic_read(&sk->wmem_alloc) == 0)
failed at af_inet.c  <==============
(174):inet_sock_destruct
<1>Unable to handle kernel NULL pointer dereference at
virtual address 00000049
<4> printing eip:
<4>c0255196
<1>*pde = 00000000
<4>Oops: 0000
<4>CPU:    1
<4>EIP:    0010:[<c0255196>]    Not tainted
<4>EFLAGS: 00010213
<4>eax: c6ace4c8   ebx: 00000000   ecx: 00000004   edx:
00000000
<4>esi: c6ace538   edi: c6ace460   ebp: 00000026   esp:
c1219eb4
<4>ds: 0018   es: 0018   ss: 0018
<4>Process swapper (pid: 0, stackpage=c1219000)
<4>Stack: c6ace460 c6ace538 c6ace460 004ec3ef c025de3e
c6ace460 00000000 
c72011a0 
<4>       c1218050 004ec2d2 c02395b2 c6ace460 c6ace538
c1218000 004ec3ef 
c025e056 
<4>       c6ace460 c1218000 00000046 004ebfe7 00000000
c1218000 00cf70a0 
c0128eaa 
<4>Call Trace: [<c025de3e>] [<c02395b2>] [<c025e056>]
[<c0128eaa>] [<c025df70>] 
<4>   [<c0128fad>] [<c012483b>] [<c0109704>] [<c0105490>]
[<c0105490>] 
[<c0105490>] 
<4>   [<c01054bc>] [<c0105542>] [<c011d51b>] [<c011d8ad>] 
<4>
<4>Code: 0f b6 4b 49 45 f6 c1 82 74 0c 31 d2 89 96 78 01 00
00 0f b6 

- Finally at the bottom of the trace the active backtrace, a
bit suspect 
because it's on the interrupt 
  side (not trace but process it's attributed to).
===========================
STACK TRACE OF FAILING TASK
===========================

================================================================
STACK TRACE FOR TASK: 0xc1218000 (swapper)

 0 tcp_enter_loss+198 [0xc0255196]
 1 tcp_retransmit_timer+473 [0xc025de39]
 2 tcp_write_timer+225 [0xc025e051]
 3 timer_bh+710 [0xc0128ea6]
 4 timer_softirq+40 [0xc0128fa8]
 5 do_softirq+185 [0xc0124839]
 6 do_IRQ+511 [0xc01096ff]
 7 do_IRQ+511 [0xc01096ff]
TRACE ERROR 0x1
================================================================

- In comparison with previous dump looks like the same
upstream event occured, 
with a timer bottom half running and invoking the
tcp_retransmit_timer. Last 
one caught it oopsing in the tcp_fragment code, this is a
bit different but the 
upstream path there is the same.

- Same pile of unknown symbol references bringing up dump
manually in lcrash, 
must be corrupt or wrong system.0 or kerntypes.0. Needs a
look.

- Dumped tcp_enter_loss+0 to tcp_enter_loss+200 to see site
at 
tcp_enter_loss+198. 
  Code at this site is:
        movzbl 0x49(%ebx),%ecx
  %ebx is NULL at this point (see above), hence the oops at
00000049. 
  Code for function is in net/ipv4/tcp_input.c starting at
line 987.

- The failure is in the loop starting at line 1002:
        
    for_retrans_queue(skb, sk, tp) {
                cnt++;
                if (TCP_SKB_CB(skb)->sacked&TCPCB_RETRANS)
                        tp->undo_marker = 0;
                TCP_SKB_CB(skb)->sacked &=
(~TCPCB_TAGBITS)|TCPCB_SACKED_ACKED;
                if
(!(TCP_SKB_CB(skb)->sacked&TCPCB_SACKED_ACKED) || how) {
                        TCP_SKB_CB(skb)->sacked &=
~TCPCB_SACKED_ACKED;
                        TCP_SKB_CB(skb)->sacked |=
TCPCB_LOST;
                        tp->lost_out++;
                } else {
                        tp->sacked_out++;
                        tp->fackets_out = cnt;
                }
        }
I didn't fully map the code but think that the expansion of:
           if (TCP_SKB_CB(skb)->sacked&TCPCB_RETRANS)
is where the zeroed pointer is used. Looks like the intent
is that skp is the 
iterater variable to loop through the retrans_queue and it
got the zero value
set on some iteration, not the first. So my guess is a
corrupted queue element
pointer being picked up and used.

- I still would look upstream at the timer bottom half
invocation as in both 
  of the dumps this upstream trace is present, and it seems
like an exception 
  path for a timeout that leads to a retransmit. 
  
- Also needs a look is the kernel assertion that failed and
likely led to the 
  oops, looks a lot like an allocation failed and returned a
NULL value, this
  would be my top culprit to pursue.
  Code from af_net.c at line 174:

void inet_sock_destruct(struct sock *sk)
{
        __skb_queue_purge(&sk->receive_queue);
        __skb_queue_purge(&sk->error_queue);

        if (sk->type == SOCK_STREAM && sk->state !=
TCP_CLOSE) {
                printk("Attempt to release TCP socket in
state %d %p\n",
                       sk->state,
                       sk);
                return;
        }
        if (!sk->dead) {
                printk("Attempt to release alive inet socket
%p\n", sk);
                return;
        }

        BUG_TRAP(atomic_read(&sk->rmem_alloc) == 0);
        BUG_TRAP(atomic_read(&sk->wmem_alloc) == 0);    <<--
assert reported 
here
        BUG_TRAP(sk->wmem_queued == 0);
        BUG_TRAP(sk->forward_alloc == 0);

        if (sk->protinfo.af_inet.opt)
                kfree(sk->protinfo.af_inet.opt);

  Continuing on after this likely led to the oops that
killed us.


-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[kuznet]

Hello!

> Is is possible for the other cpu (or even this one given the
> ksoftirqd stuff) to remove or alter the skb that
> tcp_fragment() is processing?

No.
> What locks, if any, are needed to prevent this.

They are already applied.


Looking at the last lines of the bugzilla thread, I see the answer:

> - Observation, the BUG_TRAP assertion likely should have done something better
> than just report the condition, wonder if an earlier panic or other corrective
> action may have kept the TCP stack moving instead of the oops? Looks lazy to
> me...

Well, add BUG() to BIG_TRAP() to oops it earlier. Maybe this will move
you closer to real problem.


And also your reasoning about smp_processor_id() sounds strange,
preemption code must reschedule thread preemted in the kernel to the
same cpu, it is enough to avoid troubles with this. 

Alexey

Re: System crash in tcp_fragment()

[David S. Miller]

   From: george anzinger <george@mvista.com>
   Date: Mon, 20 May 2002 12:42:08 -0700

   I wonder if you could help me squash a bug in the tcp code. 
   Here is what we know thus far:
   
   An SMP (x386 dual) 2.4.17 kernel crashes with an attempt to
   deference NULL at the end of tcp_fragment() (in
   net/ipv4/tcp_output.c) while attempting to link in the newly
   created fragment.  
   The bugzilla report is:

%99 of all such bug reports turn out to be driver bugs
where the net driver frees SKBs improperly or there is some
missing internal locking in the net device driver.

I think you efforts are better spent auditing what net
drivers are being used on this machine :-)

Re: System crash in tcp_fragment()

[george anzinger]

kuznet@ms2.inr.ac.ru wrote:
> 
> Hello!
> 
> > Is is possible for the other cpu (or even this one given the
> > ksoftirqd stuff) to remove or alter the skb that
> > tcp_fragment() is processing?
> 
> No.
> > What locks, if any, are needed to prevent this.
> 
> They are already applied.
> 
> Looking at the last lines of the bugzilla thread, I see the answer:
> 
> > - Observation, the BUG_TRAP assertion likely should have done something better
> > than just report the condition, wonder if an earlier panic or other corrective
> > action may have kept the TCP stack moving instead of the oops? Looks lazy to
> > me...
> 
> Well, add BUG() to BIG_TRAP() to oops it earlier. Maybe this will move
> you closer to real problem.

Right. Will do.
> 
> And also your reasoning about smp_processor_id() sounds strange,
> preemption code must reschedule thread preemted in the kernel to the
> same cpu, it is enough to avoid troubles with this.

That (ahem) hack was tried and rejected by the powers that
be.  I admit that is it more work to find the resulting
problems, but the fixes seem to be easy and do not to add to
overhead, at least thus far.
> 
> Alexey

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[Andi Kleen]

> And also your reasoning about smp_processor_id() sounds strange,
> preemption code must reschedule thread preemted in the kernel to the
> same cpu, it is enough to avoid troubles with this. 

It's unfortunately the truth. Even in_interrupt() is buggy current 
on SMP preemptive.

-Andi

Re: System crash in tcp_fragment()

[kuznet]

Hello!

> It's unfortunately the truth. Even in_interrupt() is buggy current 
> on SMP preemptive.

And why folks working on preemtive kernel do not repair this?
To all that I remember it is well known issue.

Alexey

Re: System crash in tcp_fragment()

[David S. Miller]

   From: george anzinger <george@mvista.com>
   Date: Mon, 20 May 2002 17:26:29 -0700

   Ah, but we have.  in_interrupt() was fixed about a month
   ago.  I think Robert got it into all the patches.  Whats
   more, it is a bit faster too :)

Well, back to the main point, this code is pretty stable by
any measurement.

I truly believe it is some side effect of either a preemption
gotcha or a driver bug.

Re: System crash in tcp_fragment()

[Andi Kleen]

On Tue, May 21, 2002 at 04:11:00AM +0400, A.N.Kuznetsov wrote:
> Hello!
> 
> > It's unfortunately the truth. Even in_interrupt() is buggy current 
> > on SMP preemptive.
> 
> And why folks working on preemtive kernel do not repair this?
> To all that I remember it is well known issue.

It is fixed on x86-64 @)

if [ "$CONFIG_SMP" = "n" ]; then
   bool 'Preemptible Kernel' CONFIG_PREEMPT
fi

-Andi

Re: System crash in tcp_fragment()

[george anzinger]

kuznet@ms2.inr.ac.ru wrote:
> 
> Hello!
> 
> > It's unfortunately the truth. Even in_interrupt() is buggy current
> > on SMP preemptive.
> 
> And why folks working on preemtive kernel do not repair this?
> To all that I remember it is well known issue.
> 
Ah, but we have.  in_interrupt() was fixed about a month
ago.  I think Robert got it into all the patches.  Whats
more, it is a bit faster too :)

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[David S. Miller]

   From: kuznet@ms2.inr.ac.ru
   Date: Tue, 21 May 2002 04:41:39 +0400 (MSD)

   +Two similar problems arise. An example code snippet:
   +
   +       struct this_needs_locking tux[NR_CPUS];
   +       tux[smp_processor_id()] = some_value;
   +       /* task is preempted here... */
   +       something = tux[smp_processor_id()];
   
   If you are not going to break all the kernel just make sure that
   tasks preempted in the kernel do not migrate. That's all, simple & stupid.
   
Such rule does not even make this piece of code legal.  Consider:

task1:cpu0:	x = counters[smp_processor_id()];
      cpu0:	PREEMPT
task2:cpu0:	x = counters[smp_processor_id()];
task2:cpu0:	counters[smp_processor_id()] = x + 1;
      cpu0:	PREEMPT
task1:cpu0:	counters[smp_processor_id()] = x + 1;
		full garbage

But it does bring up important point, preemption people need to
fully audit entire networking.

It is totally broken by preemption the more I think about it.

At the very beginning, all the SNMP counter bumping tricks will
totally fail with preemption enabled.

Re: System crash in tcp_fragment()

[george anzinger]

Andi Kleen wrote:
> 
> On Tue, May 21, 2002 at 04:11:00AM +0400, A.N.Kuznetsov wrote:
> > Hello!
> >
> > > It's unfortunately the truth. Even in_interrupt() is buggy current
> > > on SMP preemptive.
> >
> > And why folks working on preemtive kernel do not repair this?
> > To all that I remember it is well known issue.
> 
> It is fixed on x86-64 @)
> 
> if [ "$CONFIG_SMP" = "n" ]; then
>    bool 'Preemptible Kernel' CONFIG_PREEMPT
> fi
> 
That is not a fix!  It is dodging the issue. ;)

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[Andi Kleen]

On Tue, May 21, 2002 at 02:26:29AM +0200, george anzinger wrote:
> kuznet@ms2.inr.ac.ru wrote:
> > 
> > Hello!
> > 
> > > It's unfortunately the truth. Even in_interrupt() is buggy current
> > > on SMP preemptive.
> > 
> > And why folks working on preemtive kernel do not repair this?
> > To all that I remember it is well known issue.
> > 
> Ah, but we have.  in_interrupt() was fixed about a month
> ago.  I think Robert got it into all the patches.  Whats
> more, it is a bit faster too :)

It's not fixed in 2.5.15/16.  I don't know about your patches.

-Andi

Re: System crash in tcp_fragment()

[kuznet]

Hello!

> > if [ "$CONFIG_SMP" = "n" ]; then
> >    bool 'Preemptible Kernel' CONFIG_PREEMPT
> > fi
> > 
> That is not a fix!  It is dodging the issue. ;)

It is the only real fix, if you are going to follow this instruction:

+RULE #1: Per-CPU data structures need explicit protection
+
+
+Two similar problems arise. An example code snippet:
+
+       struct this_needs_locking tux[NR_CPUS];
+       tux[smp_processor_id()] = some_value;
+       /* task is preempted here... */
+       something = tux[smp_processor_id()];
+
+First, since the data is per-CPU, it may not have explicit SMP locking, but
+require it otherwise.  Second, when a preempted task is finally rescheduled,
+the previous value of smp_processor_id may not equal the current.  You must
+protect these situations by disabling preemption around them.

If you are not going to break all the kernel just make sure that
tasks preempted in the kernel do not migrate. That's all, simple & stupid.

Alexey

Re: System crash in tcp_fragment()

[kuznet]

Hello!

> Such rule does not even make this piece of code legal.  Consider:
> 
> task1:cpu0:	x = counters[smp_processor_id()];
>       cpu0:	PREEMPT
> task2:cpu0:	x = counters[smp_processor_id()];
> task2:cpu0:	counters[smp_processor_id()] = x + 1;
>       cpu0:	PREEMPT
> task1:cpu0:	counters[smp_processor_id()] = x + 1;
> 		full garbage

Yup. And this has nothing to do with SMP...


> But it does bring up important point, preemption people need to
> fully audit entire networking.

Well, we can make this. It is too serious. Anyway, this means that
preemptive patch for 2.4 is "tainting" :-)

Alexey

Re: System crash in tcp_fragment()

[Nivedita Singhvi]

On Mon, 20 May 2002, David S. Miller wrote:

> Such rule does not even make this piece of code legal.  Consider:
> 
> task1:cpu0:	x = counters[smp_processor_id()];
>       cpu0:	PREEMPT
> task2:cpu0:	x = counters[smp_processor_id()];
> task2:cpu0:	counters[smp_processor_id()] = x + 1;
>       cpu0:	PREEMPT
> task1:cpu0:	counters[smp_processor_id()] = x + 1;
> 		full garbage
> 
> But it does bring up important point, preemption people need to
> fully audit entire networking.
> 
> It is totally broken by preemption the more I think about it.
> 
> At the very beginning, all the SNMP counter bumping tricks will
> totally fail with preemption enabled.
> 

A lot of the synchronization between process context and interrupt
context is based on per-cpu data structures or simple locks
(without disabling irq's globally) eg:

softnet_data queue (we only disable local interrupts), and
synchronization between tcp_readmsg() and tcp_rcv() over
the receive queue would get confused (lock.users flag would
be different on another CPU)..

Wonder how any of it could possibly work..

thanks,
Nivedita

Re: System crash in tcp_fragment()

[David S. Miller]

   From: george anzinger <george@mvista.com>
   Date: Mon, 20 May 2002 23:08:38 -0700

   Nivedita Singhvi wrote:
   > 
   > On Mon, 20 May 2002, David S. Miller wrote:
   > 
   > > Such rule does not even make this piece of code legal.  Consider:
   > >
   > > task1:cpu0:   x = counters[smp_processor_id()];
   > >       cpu0:   PREEMPT
   > > task2:cpu0:   x = counters[smp_processor_id()];
   > > task2:cpu0:   counters[smp_processor_id()] = x + 1;
   > >       cpu0:   PREEMPT
   > > task1:cpu0:   counters[smp_processor_id()] = x + 1;
   > >               full garbage
   
   May be someone could tell me if these matter.  If you are
   bumping a counter and you switch cpus in the middle, a.)
   does it matter? and b.) if so which cpu should get the
   count?  I sort of thought that, if this were going on, it
   did not really matter as long as some counter was bumped.

That's not the problem.  We use per-cpu values for each counter (and
when the user asks for the value, we add together the values from
each processor).

Please review the example I quote above, you aren't reading it
carefully enough.

Let us imagine that we are dealing with counter "X", and
that the values at the beginning of the example are:

	X[0] = 5
	X[1] = 7
	X[2] = ...

Actually, no values matter for the purposes of this example
except the one for cpu 0.  Here is what happens, watch carefully:

   > > task1:cpu0:   x = counters[smp_processor_id()];
   > >       cpu0:   PREEMPT

task1 sees 'x' as '5'

   > > task2:cpu0:   x = counters[smp_processor_id()];
   > > task2:cpu0:   counters[smp_processor_id()] = x + 1;
   > >       cpu0:   PREEMPT

task2 bumps the counter to '6'

   > > task1:cpu0:   counters[smp_processor_id()] = x + 1;
   > >               full garbage

task1 also bumps the counter to '6'

This is the problem.  We make these counters non-atomic on purpose
for performance reasons, so do not mention that as a possible fix.

Re: System crash in tcp_fragment()

[george anzinger]

Nivedita Singhvi wrote:
> 
> On Mon, 20 May 2002, David S. Miller wrote:
> 
> > Such rule does not even make this piece of code legal.  Consider:
> >
> > task1:cpu0:   x = counters[smp_processor_id()];
> >       cpu0:   PREEMPT
> > task2:cpu0:   x = counters[smp_processor_id()];
> > task2:cpu0:   counters[smp_processor_id()] = x + 1;
> >       cpu0:   PREEMPT
> > task1:cpu0:   counters[smp_processor_id()] = x + 1;
> >               full garbage
> >
> > But it does bring up important point, preemption people need to
> > fully audit entire networking.
> >
> > It is totally broken by preemption the more I think about it.
> >
> > At the very beginning, all the SNMP counter bumping tricks will
> > totally fail with preemption enabled.

May be someone could tell me if these matter.  If you are
bumping a counter and you switch cpus in the middle, a.)
does it matter? and b.) if so which cpu should get the
count?  I sort of thought that, if this were going on, it
did not really matter as long as some counter was bumped.
> >
> 
> A lot of the synchronization between process context and interrupt
> context is based on per-cpu data structures or simple locks
> (without disabling irq's globally) eg:
> 
> softnet_data queue (we only disable local interrupts), and
> synchronization between tcp_readmsg() and tcp_rcv() over
> the receive queue would get confused (lock.users flag would
> be different on another CPU)..

Disabling local interrupts also disables preemption, as does
interrupt context.
> 
> Wonder how any of it could possibly work..

It seems to take a LOT of work to break it.  Even then, I
think this problem at hand is in the driver (a new one from
the intel folks).

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[David S. Miller]

   From: george anzinger <george@mvista.com>
   Date: Tue, 21 May 2002 00:25:46 -0700
   
   I understand the issue.  The question is what is the
   result.  Bogus numbers do.. what?  Does the kernel crash or
   does the user think strange things while every thing just
   keeps on working?

It's a quality of implementation issue.

It is just one example of a place where we use these kinds
of assumptions to index tables and the like.

Re: System crash in tcp_fragment()

[george anzinger]

"David S. Miller" wrote:
> 
>    From: george anzinger <george@mvista.com>
>    Date: Mon, 20 May 2002 23:08:38 -0700
> 
>    Nivedita Singhvi wrote:
>    >
>    > On Mon, 20 May 2002, David S. Miller wrote:
>    >
>    > > Such rule does not even make this piece of code legal.  Consider:
>    > >
>    > > task1:cpu0:   x = counters[smp_processor_id()];
>    > >       cpu0:   PREEMPT
>    > > task2:cpu0:   x = counters[smp_processor_id()];
>    > > task2:cpu0:   counters[smp_processor_id()] = x + 1;
>    > >       cpu0:   PREEMPT
>    > > task1:cpu0:   counters[smp_processor_id()] = x + 1;
>    > >               full garbage
> 
>    May be someone could tell me if these matter.  If you are
>    bumping a counter and you switch cpus in the middle, a.)
>    does it matter? and b.) if so which cpu should get the
>    count?  I sort of thought that, if this were going on, it
>    did not really matter as long as some counter was bumped.
> 
> That's not the problem.  We use per-cpu values for each counter (and
> when the user asks for the value, we add together the values from
> each processor).
> 
> Please review the example I quote above, you aren't reading it
> carefully enough.
> 
> Let us imagine that we are dealing with counter "X", and
> that the values at the beginning of the example are:
> 
>         X[0] = 5
>         X[1] = 7
>         X[2] = ...
> 
> Actually, no values matter for the purposes of this example
> except the one for cpu 0.  Here is what happens, watch carefully:
> 
>    > > task1:cpu0:   x = counters[smp_processor_id()];
>    > >       cpu0:   PREEMPT
> 
> task1 sees 'x' as '5'
> 
>    > > task2:cpu0:   x = counters[smp_processor_id()];
>    > > task2:cpu0:   counters[smp_processor_id()] = x + 1;
>    > >       cpu0:   PREEMPT
> 
> task2 bumps the counter to '6'
> 
>    > > task1:cpu0:   counters[smp_processor_id()] = x + 1;
>    > >               full garbage
> 
> task1 also bumps the counter to '6'
> 
> This is the problem.  We make these counters non-atomic on purpose
> for performance reasons, so do not mention that as a possible fix.

I understand the issue.  The question is what is the
result.  Bogus numbers do.. what?  Does the kernel crash or
does the user think strange things while every thing just
keeps on working?

As for the fix, I would think that would be more up to the
network folks than me.  Atomic is one option, but you can
also disable preemption.  It is really light weight, and may
already be disabled in some of these cases.

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: System crash in tcp_fragment()

[Andi Kleen]

> That's not the problem.  We use per-cpu values for each counter (and
> when the user asks for the value, we add together the values from
> each processor).

At least on x86 gcc usually seems to just generate an incl, which should
be ok because it is atomic enough (even when a reschedule happens it will 
act as a full memory barrier)

So it'll likely just be a problem for load-store architectures.


-Andi

Re: System crash in tcp_fragment()

[kuznet]

Hello!

> I understand the issue.

I do not.


+#define preempt_disable() \
+do { \
+       ++current->preempt_count; \
+       barrier(); \
+} while (0)

Why does this work?

Alexey

Re: System crash in tcp_fragment()

[Andi Kleen]

> As for the fix, I would think that would be more up to the
> network folks than me.  Atomic is one option, but you can
> also disable preemption.  It is really light weight, and may
> already be disabled in some of these cases.

In most of them because it is running in a softirq.
Just the user context stuff has a possible problem on non i386/x86-64.

-Andi

Re: System crash in tcp_fragment()

[george anzinger]

kuznet@ms2.inr.ac.ru wrote:
> 
> Hello!
> 
> > I understand the issue.
> 
> I do not.
> 
> +#define preempt_disable() \
> +do { \
> +       ++current->preempt_count; \
> +       barrier(); \
> +} while (0)
> 
> Why does this work?
> 
> Alexey
Preemption can only happen if the task is interrupted.  The
interrupt exit code will not preempt if preempt_count is
more than zero.  The barrier() forces the result to memory
so the interrupt code can find it.
-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml