IrDA woes..

IrDA woes..

[Linus Torvalds]

Ok, so nobody sane likely uses IrDA any more, but - surprise surprise
- some dive computers aren't sane. And there's reports of some really
excessive slowdowns with modern kernels (downloading the memory dump
from a dive computer taking 18 minutes on a 3.2-based kernel, and 80
minutes with a 3.11 kernel (it apparently takes 12 minutes on WinXP).

There has been basically zero changes to the driver in question
(stir4200), so the slowdown is likely due to generic networking or
irda changes.. Some timeout change or whatever.

I'm still waiting for a couple of IrDA USB dongles to try things out
on real hardware (UPS is apparently still having delivery issues, so
the dongles that were supposed to arrive today won't be here until
Thursday, and nobody sells those things in brick-and-mortar stores any
more). Maybe I can reproduce the slowness, maybe I can't. We'll see.

In the meantime, I am playing with IrDA attached to a pty, and hitting
interesting kernel oopses (unrelated side note: SELinux also hates
playing irda/pty games, you have to put things into permissive mode
etc).

One of the oopses seems simple: irda_attach() will do

        if (sk->sk_prot->disconnect(sk, flags))
                sock->state = SS_DISCONNECTING;

if the connection fails. But sk_prot->disconnect is NULL for IrDA, so
that will just oops. Apparently real devices don't end up ever
triggering that, but I don't think it can ever have worked.

The next one was harder to trigger, and is much less obvious, even if
it's also a trivial NULL pointer dereference:

  Unable to handle kernel NULL pointer dereference at 00000000000000d8
  IP: skb_copy+0x11
    rdi=0x0000000000000000
    rsi=0x0000000000000020 (GFP_ATMIC = __GFP_HIGH)
  Call trace:
    irlap_resend_rejected_frames
    irlap_state_nrm_s
    irlap_do_event
    irlap_driver_rcv
    __netif_receive_skb_core
    __netif_receive_skb
    process_backlog
    net_rx_action
    __do_softirq
    irq_exit

  Code:
        55                      push   %rbp
        b9 ff ff ff ff          mov    $0xffffffff,%ecx
        48 89 e5                mov    %rsp,%rbp
        41 55                   push   %r13
        41 54                   push   %r12
        53                      push   %rbx
        48 89 fb                mov    %rdi,%rbx
        4c 8b af d8 00 00 00    mov    0xd8(%rdi),%r13          <--
trapping instruction
        0f b6 93 aa 00 00 00    movzbl 0xaa(%rbx),%edx
        4c 2b af d0 00 00 00    sub    0xd0(%rdi),%r13

so it seems that irlap_resend_rejected_frames() does a skb_copy() with
a NULL skb. Which in turn seems to be due to corruption or lack of
locking, since the skb is the result of

      skb_queue_walk(&self->wx_list, skb) {
          ...

Does anybody have any ideas? Note that this is likely *not* a new thing.

                    Linus

Re: IrDA woes..

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 31 Dec 2013 12:08:43 -0800

> One of the oopses seems simple: irda_attach() will do

I assume you mean irda_connect().

>         if (sk->sk_prot->disconnect(sk, flags))
>                 sock->state = SS_DISCONNECTING;
> 
> if the connection fails. But sk_prot->disconnect is NULL for IrDA, so
> that will just oops. Apparently real devices don't end up ever
> triggering that, but I don't think it can ever have worked.

That sk_prot should be irda_proto, which doesn't provide any non-NULL
operations ?!?!

That snippet was probably cut and pasted from another protocol's
create implementation.

Generic code (such as that in net/core/sock.c) checks if the
various sk_prot methods are NULL before trying to invoke them.

I think the thing to do for now is to simply remove those two lines.

I don't believe that anyone has looked closely at this code for
a long time except with automated tools like semantic checkers.

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 12:46 AM, David Miller <davem@davemloft.net> wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Tue, 31 Dec 2013 12:08:43 -0800
>
>> One of the oopses seems simple: irda_attach() will do
>
> I assume you mean irda_connect().

Yup, sorry, mindfart.

>>         if (sk->sk_prot->disconnect(sk, flags))
>>                 sock->state = SS_DISCONNECTING;
>>
>> if the connection fails. But sk_prot->disconnect is NULL for IrDA, so
>> that will just oops. Apparently real devices don't end up ever
>> triggering that, but I don't think it can ever have worked.
>
> That sk_prot should be irda_proto, which doesn't provide any non-NULL
> operations ?!?!

That was how I read it too. I'm just surprised by how google doesn't
seem to find any matching oopses with irda_connect(). Admittedly I'm
doing odd things, so..

> I think the thing to do for now is to simply remove those two lines.

Ack, that was my feeling too. I added just a test for NULL for my
further testing, but I can't see how it could ever *not* be NULL.

> I don't believe that anyone has looked closely at this code for
> a long time except with automated tools like semantic checkers.

So looking at the code, the locking seems to be entirely lacking too,
which is what I *think* is the source of my other oops. It traverses
the internal irda wx_list at softirq time, and as far as I can tell,
there is no locking anywhere. And the irda code ends up being called
from timers, from the network receive function, and presumably from
process context.

I get the feeling that the code was written back in the bad old days
when we had the big kernel lock, and timers and network bottom half
handling were all mutually exclusive.

Is anybody actually really using or maintaining irda any more? Because
nothing seems to work. I think the following is *supposed* to work as
a way to emulate having two irda dongles without actually having the
hardware:

 - in terminal 1:

        socat PTY,link=/tmp/ttyS0 PTY,link=/tmp/ttyS1

 - terminal 2:

        irattach $(readlink /tmp/ttyS0) -s
        irattach $(readlink /tmp/ttyS1) -s

 - in terminal 3:

        echo "Duh" > /dev/ircomm0

 - in terminal 4:

        cat /dev/ircomm1

(expecting to see "Duh" in terminal 4) but it really doesn't work for me.

The odd thing is that I *have* seen it work, a couple of times, so
it's not like it *never* works (in which case I'd just think that the
above simply isn't even supposed to work at all), but the "it actually
worked" is a very rare occurrence. Most of the time opening the ircomm
blocks. But sometimes it works after a few seconds. Very odd.

Samuel is listed as maintainer, but I'm getting the feeling he has
long since moved on.

               Linus

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 11:11 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I get the feeling that the code was written back in the bad old days
> when we had the big kernel lock, and timers and network bottom half
> handling were all mutually exclusive.

Hmm. It even has a comment to that effect, see
irlap_resend_rejected_frames(), which does that skb_queue_walk()
without taking the queue lock. It says:

 *    Resend frames which has not been acknowledged. Should be safe to
 *    traverse the list without locking it since this function will only be
 *    called from interrupt context (BH)

and I don't *think* that is true any more.

Now, because it is in bottom half contect, I think it won't be
re-entered on the same CPU, but another cpu might be busy doing
irda_rcv(). No?

So I'd be inclined to take the queue lock (self->wx_list.lock) around
that skb_queue_walk(). Or am I just dazed and confused?

                    Linus

Re: IrDA woes..

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 2 Jan 2014 13:07:05 -0800

> So I'd be inclined to take the queue lock (self->wx_list.lock) around
> that skb_queue_walk(). Or am I just dazed and confused?

Yes, something like the following untested patch.

I used irq blocking locking to be consistent with what the helpers
like skb_dequeue() use.  And you have to be careful to adjust code
to __skb_dequeue() that you put behind explicit taking of the lock.
skb_dequeue() is just a wrapper that take the lock around a call
to __skb_dequeue().

diff --git a/net/irda/irlap.c b/net/irda/irlap.c
index 005b424..1334625 100644
--- a/net/irda/irlap.c
+++ b/net/irda/irlap.c
@@ -698,19 +698,22 @@ int irlap_generate_rand_time_slot(int S, int s)
 void irlap_update_nr_received(struct irlap_cb *self, int nr)
 {
 	struct sk_buff *skb = NULL;
+	unsigned long flags;
 	int count = 0;
 
 	/*
 	 * Remove all the ack-ed frames from the window queue.
 	 */
 
+	spin_lock_irqsave(&self->wx_list.lock, flags);
+
 	/*
 	 *  Optimize for the common case. It is most likely that the receiver
 	 *  will acknowledge all the frames we have sent! So in that case we
 	 *  delete all frames stored in window.
 	 */
 	if (nr == self->vs) {
-		while ((skb = skb_dequeue(&self->wx_list)) != NULL) {
+		while ((skb = __skb_dequeue(&self->wx_list)) != NULL) {
 			dev_kfree_skb(skb);
 		}
 		/* The last acked frame is the next to send minus one */
@@ -720,7 +723,7 @@ void irlap_update_nr_received(struct irlap_cb *self, int nr)
 		while ((skb_peek(&self->wx_list) != NULL) &&
 		       (((self->va+1) % 8) != nr))
 		{
-			skb = skb_dequeue(&self->wx_list);
+			skb = __skb_dequeue(&self->wx_list);
 			dev_kfree_skb(skb);
 
 			self->va = (self->va + 1) % 8;
@@ -730,6 +733,8 @@ void irlap_update_nr_received(struct irlap_cb *self, int nr)
 
 	/* Advance window */
 	self->window = self->window_size - skb_queue_len(&self->wx_list);
+
+	spin_unlock_irqrestore(&self->wx_list.lock, flags);
 }
 
 /*
diff --git a/net/irda/irlap_frame.c b/net/irda/irlap_frame.c
index 9ea0c93..45fd93f 100644
--- a/net/irda/irlap_frame.c
+++ b/net/irda/irlap_frame.c
@@ -983,10 +983,13 @@ void irlap_resend_rejected_frames(struct irlap_cb *self, int command)
 {
 	struct sk_buff *tx_skb;
 	struct sk_buff *skb;
+	unsigned long flags;
 
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
+	spin_lock_irqsave(&self->wx_list.lock, flags);
+
 	/*  Resend unacknowledged frame(s) */
 	skb_queue_walk(&self->wx_list, skb) {
 		irlap_wait_min_turn_around(self, &self->qos_tx);
@@ -1039,16 +1042,20 @@ void irlap_resend_rejected_frames(struct irlap_cb *self, int command)
 		}
 	}
 #endif
+	spin_unlock_irqrestore(&self->wx_list.lock, flags);
 }
 
 void irlap_resend_rejected_frame(struct irlap_cb *self, int command)
 {
 	struct sk_buff *tx_skb;
+	unsigned long flags;
 	struct sk_buff *skb;
 
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
+	spin_lock_irqsave(&self->wx_list.lock, flags);
+
 	/*  Resend unacknowledged frame(s) */
 	skb = skb_peek(&self->wx_list);
 	if (skb != NULL) {
@@ -1072,6 +1079,8 @@ void irlap_resend_rejected_frame(struct irlap_cb *self, int command)
 
 		irlap_send_i_frame(self, tx_skb, command);
 	}
+
+	spin_unlock_irqrestore(&self->wx_list.lock, flags);
 }
 
 /*

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 2:41 PM, David Miller <davem@davemloft.net> wrote:
>
> Yes, something like the following untested patch.

I did that initially too, but that doesn't work, because the code
inside the skb_queue_walk() loop ends up calling irlap_send_i_frame()
which calls irlap_queue_xmit(), which does dev_queue_xmit().

Which will do a bh_disable/bh_enable() (as part of spin_[un]lock_bh(), iirc).

So you can't use "spin_lock_irqsave()", because it doesn't nest around
a softirq-disable.

You could use spin_lock_bh(), but if it's true that it's all in a
softirq context, I think it should be safe to just do "spin_lock()".
Afaik, there is nothing that actually does anything in real hardirq
context in there. I *think* all the network _rcv() functions are
called from softirqs, right?

             Linus

Re: IrDA woes..

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 2 Jan 2014 14:47:44 -0800

> You could use spin_lock_bh(), but if it's true that it's all in a
> softirq context, I think it should be safe to just do "spin_lock()".
> Afaik, there is nothing that actually does anything in real hardirq
> context in there. I *think* all the network _rcv() functions are
> called from softirqs, right?

I'm worried that mixing hardIRQ locking (via the direct calls to
skb_dequeue() et al.) with explicit softIRQ locking will make lockdep
complain.

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 4:05 PM, David Miller <davem@davemloft.net> wrote:
>
> I'm worried that mixing hardIRQ locking (via the direct calls to
> skb_dequeue() et al.) with explicit softIRQ locking will make lockdep
> complain.

You're probably right. I don't see how to do it right, though. lockdep
(or maybe it's DEBUG_ATOMIC_SLEEP or whatever) definitely complains
about the "enable softirq's with hardirqs disabled" part, and the way
the queueing is set up in irda, I don't see how else to do it.

Unless it would be ok to only re-send the oldest packet, not all of
them. And thus get rid of the loop entirely. But with the obvious lack
of testing irda is getting, that kind of change sounds like a really
bad idea.

The warning is, in case you care:

  WARNING: CPU: 2 PID: 0 at kernel/softirq.c:156 local_bh_enable+0x62/0x90()
  Call Trace:
   dev_queue_xmit
   irlap_queue_xmit
   irlap_send_i_frame
   irlap_resend_rejected_frames
   irlap_state_nrm_s
   irlap_do_event
   irlap_driver_rcv
   __netif_receive_skb_core

and without the _irqsave() at least that part is fine. But no, I
didn't try it with lockdep, and I think you're right that it would
break.

Oh well. Since I can't seem to get my test-case to work anyway, I
guess it doesn't much matter..

              Linus

Re: IrDA woes..

[Marcel Holtmann]

Hi Linus,

> Is anybody actually really using or maintaining irda any more? Because
> nothing seems to work. I think the following is *supposed* to work as
> a way to emulate having two irda dongles without actually having the
> hardware:
> 
> - in terminal 1:
> 
>        socat PTY,link=/tmp/ttyS0 PTY,link=/tmp/ttyS1
> 
> - terminal 2:
> 
>        irattach $(readlink /tmp/ttyS0) -s
>        irattach $(readlink /tmp/ttyS1) -s
> 
> - in terminal 3:
> 
>        echo "Duh" > /dev/ircomm0
> 
> - in terminal 4:
> 
>        cat /dev/ircomm1
> 
> (expecting to see "Duh" in terminal 4) but it really doesn't work for me.

if you are using the stir4200 driver then you are using an USB dongle that does not need the irattach to make it known to the IrDA subsystem. It will automatically make it known. For every IrDA dongle you attach you should see an irdaX with ifconfig.

The irattach (if I remember this correctly since this is long long time ago) is to take an IrDA device that is attached to a TTY and attach the IrDA ldisc to it to make it know to the IrDA subsystem. Not something that is needed for USB dongles with a proper driver. So the above should have never worked for you.

While the IrDA communication via /dev/ircommX is somehow possible, I do not remember how. What we used back in the days were IrDA sockets. I think if you look into Gnokii source you mind find an example on how to create an IrDA socket to talk to Nokia phone. Then again, this is so long time ago, I am not going to look at that since I am afraid that code is pretty ugly by todays standards.

Regards

Marcel

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 1:41 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> if you are using the stir4200 driver then you are using an USB dongle that does not need the irattach to make it known to the IrDA subsystem. It will automatically make it known. For every IrDA dongle you attach you should see an irdaX with ifconfig.

I don't actually want to use the USB dongle if I can avoid it. I've
*tried* it, but it has the exact same issue as the pty has, namely
that it doesn't really work reliably.

So for debugging I'd actually much prefer to use the pty approach.

I don't have any devices that use irda (the dive computers that showed
the problem are expensive and not any that I personally would actually
want to buy), so I'm trying to recreate the problems using a loopback
system.

(I have a non-ircomm version too, which has a device emulation on one
end of the pty, and uses a irda socket on another end. That one is
very specialized for the dive computer thing, though, which is why I
wanted to first try to do something "simpler" using just ircomm at
both ends).

               Linus

Re: IrDA woes..

[Marcel Holtmann]

Hi Linus,

>> if you are using the stir4200 driver then you are using an USB dongle that does not need the irattach to make it known to the IrDA subsystem. It will automatically make it known. For every IrDA dongle you attach you should see an irdaX with ifconfig.
> 
> I don't actually want to use the USB dongle if I can avoid it. I've
> *tried* it, but it has the exact same issue as the pty has, namely
> that it doesn't really work reliably.
> 
> So for debugging I'd actually much prefer to use the pty approach.
> 
> I don't have any devices that use irda (the dive computers that showed
> the problem are expensive and not any that I personally would actually
> want to buy), so I'm trying to recreate the problems using a loopback
> system.
> 
> (I have a non-ircomm version too, which has a device emulation on one
> end of the pty, and uses a irda socket on another end. That one is
> very specialized for the dive computer thing, though, which is why I
> wanted to first try to do something "simpler" using just ircomm at
> both ends).

I do not think that the irattach emulation idea of PTY is going to work for you. irattach is pretty much specialized to the underlaying IrDA hardware that is attached to its exposed (or physically connected) TTY.

As far as I recall, there was never any loopback emulation implemented in Linux that you could use. So you need to go through real IrDA hardware if you want to do anything useful. You might be just out of luck with the IrDA subsystem and its capabilities to do any easy development.

Maybe irdadump is helping you do debug some parts, but that tool might be as outdated as everything else.

Back in the days, I had more success with the stir4200 based USB dongles than any of the TTY based ones. But my bet is also that this all goes back 10 years, so my brain could clearly play tricks on me.

Regards

Marcel

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 2:13 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Back in the days, I had more success with the stir4200 based USB dongles than any of the TTY based ones. But my bet is also that this all goes back 10 years, so my brain could clearly play tricks on me.

So I do have two dongles, because I was afraid of that. Can you tell
me how to debug *those* just connected to each other? Preferably on
the same machine, because I'll be traveling soon..

             Linus

Re: IrDA woes..

[Linus Torvalds]

On Thu, Jan 2, 2014 at 2:29 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I do have two dongles, because I was afraid of that. Can you tell
> me how to debug *those* just connected to each other? Preferably on
> the same machine, because I'll be traveling soon..

Side note: is there any *reason* why two pty's connected to each other
can't work? They should look very much like a dongle that just doesn't
do any encapsulation of the data, and it really *almost* works. I can
actually get the simulator that uses the irda sockets to run over
them, except it will hang eventually in some timing-dependent manner.

It works well enough that I'd be inclined to blame some kernel bug.
Especially considering that I found what looked like obvious kernel
bugs already (neither of which fixes the hang, though).

             Linus

Re: IrDA woes..

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 2 Jan 2014 11:11:42 -0800

> Is anybody actually really using or maintaining irda any more?

No, not to my knowledge.

Re: IrDA woes..

[Samuel Ortiz]

Hi Linus,

On Thu, 2014-01-02 at 11:11 -0800, Linus Torvalds wrote:
> The odd thing is that I *have* seen it work, a couple of times, so
> it's not like it *never* works (in which case I'd just think that the
> above simply isn't even supposed to work at all), but the "it actually
> worked" is a very rare occurrence. Most of the time opening the ircomm
> blocks. But sometimes it works after a few seconds. Very odd.
I'm pretty sure the stack was never designed nor tested for that kind of
usage, so I'm even surprised it ever worked for you.

> Samuel is listed as maintainer, but I'm getting the feeling he has
> long since moved on.
That's correct, and the MAINTAINERS file should be updated I guess.
The combination of my last IrDA enabled phone dying and the IrDA
association removing my access to the specs significantly lowered my
interest.

Cheers,
Samuel.

Re: IrDA woes..

[Loganaden Velvindron]

On Sat, Jan 4, 2014 at 5:23 AM, Samuel Ortiz <samuel@sortiz.org> wrote:
> Hi Linus,
>
> On Thu, 2014-01-02 at 11:11 -0800, Linus Torvalds wrote:
>> The odd thing is that I *have* seen it work, a couple of times, so
>> it's not like it *never* works (in which case I'd just think that the
>> above simply isn't even supposed to work at all), but the "it actually
>> worked" is a very rare occurrence. Most of the time opening the ircomm
>> blocks. But sometimes it works after a few seconds. Very odd.
> I'm pretty sure the stack was never designed nor tested for that kind of
> usage, so I'm even surprised it ever worked for you.
>
>> Samuel is listed as maintainer, but I'm getting the feeling he has
>> long since moved on.
> That's correct, and the MAINTAINERS file should be updated I guess.
> The combination of my last IrDA enabled phone dying and the IrDA
> association removing my access to the specs significantly lowered my
> interest.

May we know what was the reason given to you ?


>
> Cheers,
> Samuel.
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
This message is strictly personal and the opinions expressed do not
represent those of my employers, either past or present.