Date:	Thu, 9 Sep 2010 07:41:54 -0500
From:	"Stephane Grosjean" <grosjean@...ase.com>
To:	<linux-kernel@...r.kernel.org>
Subject: PROBLEM: tty_buffer_request_room() vs. ppp_sync_input(): a single  buffer may contains several frames

Hello,

Thank you to forward that mail to the team which maintains the
drivers/char tty interface.
This problem seems to be present in all versions of the Kernel, I
personnally use 2.6.21 but saw the same code in some newer versions...
 
I'm using ppp_sync line discipline with an internal char driver which
intends to interface between the kernel tty interface and our hardware.
In my isr, I use "tty_insert_flip_string()" when I just received a frame
from my hardware then, I call "tty_schedule_flip()" to ask that frame to
be pass to the ppp_sync ldisc receive callback.
 
My problem is that the "tty_buffer_request_room()" function may use the
same buffer (the tail) if it finds enough place to store the incoming
frame in.
But when such a case does occur, the buffer contains 2 (or more) frames
and nothing to separate them...
This is a problem when "ppp_sync_input()" is then called as it considers
a buffer as a SINGLE frame container (as it is written in the function
header:

/* called when the tty driver has data for us.
 *
 * Data is frame oriented: each call to ppp_sync_input is considered
 * a whole frame. If the 1st flag byte is non-zero then the whole
 * frame is considered to be in error and is tossed.
 */
static void
ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
      char *flags, int count)


Running "pppd" with the "debug" option shows that it doesn't understand
some incoming frames and starts retransmission cycles (see below).
 
Since the "commit" field of a buffer object is set to the value of
"used" field when that buffer is pushed, one fix I found is to change
the condition used to search for a valid buffer in
"tty_buffer_request_room()":
 
   /* OPTIMISATION: We could keep a per tty "zero" sized buffer to
      remove this conditional if its worth it. This would be invisible
      to the callers */
   if ((b = tty->buf.tail) != NULL)
      left = b->size - b->used;
   else
      left = 0;
 
  -if (left < size) {
  +if (left < size || (b->commit >= b->used)) {
      /* This is the slow path - looking for new buffers to use */
      if ((n = tty_buffer_find(tty, size)) != NULL) {

Please let me know if I'm right and if this fix is the only way to pass
synchronous frames to upper layers.
 
Local "pppd" debug ouput shows at least 2 received buffers which contain
2 PPP frames each (see ->):

pppd /dev/ttyDCC0 sync nolock 10.10.10.1:10.10.10.2 -detach debug
      Thu Jan  1 03:54:13 UTC 1970
      using channel 11
      Using interface ppp0
      Connect: ppp0 <--> /dev/ttyDCC0
      sent [LCP ConfReq id=0x1 <asyncmap 0x0> <magic 0xd7da1d35> <pcomp>
<accomp>]
      rcvd [LCP ConfAck id=0x1 <asyncmap 0x0> <magic 0xd7da1d35> <pcomp>
<accomp>]
      rcvd [LCP ConfReq id=0x4b <asyncmap 0x0> <magic 0x767ed47a>
<pcomp> <accomp>]
      sent [LCP ConfAck id=0x4b <asyncmap 0x0> <magic 0x767ed47a>
<pcomp> <accomp>]
      sent [CCP ConfReq id=0x1 <deflate 15> <deflate(old#) 15> <bsd v1
15>]
      sent [IPCP ConfReq id=0x1 <compress VJ 0f 01> <addr 10.10.10.1>]
   -> rcvd [LCP EchoReq id=0x0 magic=0x767ed47a] 80 21 01 73 00 10 02 06
00 2d 0f 01 03 06 0a 0a 0a 02 80 fd 01 4b 00 04 80 fd 04 01 00 08 1a 04
...
      sent [LCP EchoRep id=0x0 magic=0xd7da1d35]
      rcvd [IPCP ConfAck id=0x1 <compress VJ 0f 01> <addr 10.10.10.1>]
   -> rcvd [IPCP ConfReq id=0x73 <compress VJ 0f 01> <addr 10.10.10.2>]
80 fd 01 4b 00 04
      sent [IPCP ConfAck id=0x73 <compress VJ 0f 01> <addr 10.10.10.2>]
      local  IP address 10.10.10.1
 
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists