PROBLEM: tty_buffer_request_room() vs. ppp_sync_input(): a single buffer may contains several frames

PROBLEM: tty_buffer_request_room() vs. ppp_sync_input(): a single buffer may contains several frames

[Stephane Grosjean]

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

Re: PROBLEM: tty_buffer_request_room() vs. ppp_sync_input(): a single buffer may contains several frames

[Alan Cox]

> 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.

Correct. The tty layer doesn't preserve record boundaries.

> Please let me know if I'm right and if this fix is the only way to pass
> synchronous frames to upper layers.

The tty buffer system is not intended for framed data. It doesn't handle
all sorts of cases of it and will split buffers and pack them as it sees
fit.

There is a limited provision for tty style handling of such interfaces
because some hardware is dual mode but it requires you don't use the tty
buffering helpers.

If your hardware is a straight sync board you probably want to look at
drivers/net/wan/hdlc*c which provides the interfaces so you can just
throw frames straight off a card into the network layer and appear as a
network stack. Unlike n_syncppp it also supports other protocols pretty
much for free (Cisco HDLC etc)

Alan