buglwIP - A Lightweight TCP/IP stack - Bugs: bug #44565, PPPOS support in tcpip thread...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #44565: PPPOS support in tcpip thread breaks SLIP

Submitter:  Ivan Delamer <idelamer>
Submitted:  Tue 17 Mar 2015 10:54:46 PM UTC
   
 
Category:  PPP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  idelamer
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 20 Mar 2015 03:14:46 PM UTC, comment #24: 


> I would make a note of the flag name change in release notes in case anyone is affected.


Yes, this is already in the PPP documentation:

http://git.savannah.gnu.org/cgit/lwip.git/tree/doc/ppp.txt#n391

In order to not duplicate documentation, we should just put a pointer in the release notes about the "Upgrading from lwIP <= 1.4.x to lwIP >= 1.5.x" chapter in the PPP documentation.

Sylvain Rochet <gradator>
Group Member
Fri 20 Mar 2015 02:59:55 PM UTC, comment #23: 

I would make a note of the flag name change in release notes in case anyone is affected. And I agree with Simon, lets not break any existing ports by changing ethernet_input handling.

Cheers, Ivan

Ivan Delamer <idelamer>
Group Member
Fri 20 Mar 2015 08:53:52 AM UTC, comment #22: 

Hello Simon,

Yeah, I agree too, fair enough :-)

I guess I can close this bug.

Sylvain

Sylvain Rochet <gradator>
Group Member
Fri 20 Mar 2015 07:28:49 AM UTC, comment #21: 


> Simon, what do you think about Ivan proposal: "I wonder if we
> should do the same with Ethernet packets?" I agree we should.


I don't think we should. It would break all applications (not ports) passing tcpip_input to netif_add. We could rather make ethernet_input the default and add something for "raw" IP input, which seems a far more uncommon case to me.

> Maybe we can also remove the useless struct netif input pointer.


I don't think it's useless: the code calling netif->input is written by a port developer. But the decision of passing tcpip_input or ethernet_input to netif_add is one that has to be done by the application developer. You could argue that this could be solved via '#if NO_SYS', but even then, the problem of little optimization vs. breaking existing ports remains.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 19 Mar 2015 08:56:19 PM UTC, comment #20: 

Patch rebased and applied. I am very happy with the outcome, this long lasting issue is solved for me in the cleanest way.

> I am going to improve the documentation on this point.


Done !


Anyway, I renamed PPP_INPROC_MULTITHREADED to PPP_INPROC_IRQ_SAFE to remove the confusion we (or at least, I) had, shall we add a compilation error if defined(PPP_INPROC_MULTITHREADED) in init.c ?

Simon, what do you think about Ivan proposal: "I wonder if we should do the same with Ethernet packets?" I agree we should.

Maybe we can also remove the useless struct netif input pointer, but if we do it requires a change in all ports :(

Sylvain

Sylvain Rochet <gradator>
Group Member
Thu 19 Mar 2015 09:00:31 AM UTC, comment #19: 

/* if pppos_input_free_current_packet() was called in the meanwhile (connect, listen, or free) */

After thinking a bit more, that's ok, because I am resetting the PPP RX state machine in connect and listen.

It should be ok in interrupt context. I am going to improve  the documentation on this point.

Sylvain Rochet <gradator>
Group Member
Thu 19 Mar 2015 08:55:26 AM UTC, comment #18: 

I forget to add but of course I totally agree it is almost fully thread safe in interrupt context, being interrupt safe and being thread safe is not the same thing at all ! :-)

Sylvain

Sylvain Rochet <gradator>
Group Member
Thu 19 Mar 2015 08:50:14 AM UTC, comment #17: 


> Although this is slightly off topic: isn't PPP_INPROC_MULTITHREADED
> thread-safe when pppos_input() is called from an interrupt? The rest
> of the processing can then only be called from tcpip_thread, so
> pppos_input can interrupt PPP, but PPP cannot interrupt pppos_input.
> That's the way I designed it at least when appliying the fixes in the
> past.


I am not sure, mainly because the pppos->in_state is not cleared by pppos_input_free_current_packet().

[code]
void pppos_input(ppp_pcb *ppp, u8_t *s, int l) {

/* If we received a flag while we are in PDDATA */

} else if (cur_char == PPP_FLAG) {

/* and if by chance the FCS is valid */

        } else {
          struct pbuf *inp;
          /* Trim off the checksum. */
          if(pppos->in_tail->len > 2) {
            pppos->in_tail->len -= 2;

/* if pppos_input_free_current_packet() was called in the meanwhile (connect, listen, or free) */

/* we are then dereferencing the in_tail buffer which is currently NULL */

[/code]


> I admit that "MULTITHREADED" is then not a very good name...


Indeed.


The main thing that tickle me is that I did the mistake of using
PPP_INPROC_MULTITHREADED in a non thread-safe way, even
with my overall perfect knowledge on how lwIP PPP stack is
working. That's mainly due to a total lack of documentation, but
even with that, if I did the mistake, then I am not the only one
going to.

Sylvain Rochet <gradator>
Group Member
Thu 19 Mar 2015 06:27:39 AM UTC, comment #16: 

Much better :-)

Although this is slightly off topic: isn't PPP_INPROC_MULTITHREADED thread-safe when pppos_input() is called from an interrupt? The rest of the processing can then only be called from tcpip_thread, so pppos_input can interrupt PPP, but PPP cannot interrupt pppos_input. That's the way I designed it at least when appliying the fixes in the past.

I admit that "MULTITHREADED" is then not a very good name...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Mar 2015 07:59:27 PM UTC, comment #15: 

I like it. I will probably do the same with 6LowPAN in the future. I wonder if we should do the same with Ethernet packets?

Ivan Delamer <idelamer>
Group Member
Wed 18 Mar 2015 07:54:29 PM UTC, comment #14: 

Added patch which adds TCPIP_MSG_INPKT_PPPOS.

What do you think ?

Sylvain

(file #33392)

Sylvain Rochet <gradator>
Group Member
Wed 18 Mar 2015 05:44:00 PM UTC, comment #13: 

Hello Simon,


> And even if I would have preferred to keep the unescaping and
> packet deframing in interrupt context, without having to switch
> into tcpip_thread for every byte (it was valid to call
> pppos_input for each byte),


I didn't remove this feature, you can still do that. This is
what the PPP documentation is actually saying, which the
necessary addendum about the inherent problem when using
PPP_INPROC_MULTITHREADED.

------------8<------------8<------------8<------------
3 PPPoS input path (raw API, thread safe API, TCPIP API)
========================================================

PPPoS requires a serial I/O SIO port (see include/lwip/sio.h).
Received data on serial port should be sent to lwIP using the pppos_input() or
pppos_input_sys() functions.

If PPP_INPROC_MULTITHREADED is 0 (the default), pppos_input() is not thread safe
and then MUST only be called inside the lwIP context. You should use that if
you are calling pppos_input() from your main loop context when NO_SYS=1.

If PPP_INPROC_MULTITHREADED is 1, pppos_input() is thread safe and can be called
from a dedicated RX-thread or from interrupt context… BUT you should NEVER
call pppos_connect(), pppos_listen() and ppp_free() if pppos_input() can still
be running, doing this is NOT thread safe. You should also avoid calling
pppos_input() if PPPoS session is not started yet.

Using PPP_INPROC_MULTITHREADED is discouraged unless you really know what you
are doing, though it may greatly reduce your need of buffer if pppos_input() is
called byte after byte in your rx serial interrupt, your move ;-)

Anyway, if you are using an OS (NO_SYS=0) and if PPP_INPROC_MULTITHREADED is 0,
you can use the pppos_input_tcpip() function to pass input data to the lwIP
core thread. This is thread safe in all cases but you should avoid passing
data byte after byte because it uses heavy locking (mailbox) and it allocates
pbuf, better fill them !

/*

  • Fonction to call for received data

 *

  • ppp, PPP control block
  • buffer, input buffer
  • buffer_len, buffer length in bytes

 */
void pppos_input(ppp, buffer, buffer_len);

or

void pppos_input_tcpip(ppp, buffer, buffer_len);
------------8<------------8<------------8<------------

I know it is in my broken english and may actually be hard
to understand even if it's crystal clear for me, I gladly
accept any help on the documentation subject, I am VERY bad
doing that :-)


> I accept that it's possibly easier this way to get it
> thread safe.


Yes, this is just another way for passing raw data PPPoS input
function, this is in fact the only way which is thread safe
in any case, for those who really care about thread safety
while having a very simple code on the user part (I am ! ;P).


> However, when asking how this was previously solved, I was
> rather thinking about how did the old version get into
> tcpip_thread. And looking at the old sources, it used
> 'tcpip_callback', not 'tcpip_input'. Now I'm just curious why
> this has been changed, even if the change interferes with
> other modules.


This is still there. And this is not thread safe.

pppos_input() in the old and new code might be running while
ppp_free_input_packet() is called.


Sylvain

Sylvain Rochet <gradator>
Group Member
Wed 18 Mar 2015 05:27:20 PM UTC, comment #12: 

And I don't want to sound rude here. I just like to discuss such a change.

> Maybe we should add some new types such as
> TCPIP_MSG_INPKT_PPPOS, TCPIP_MSG_INPKT_ETHERNET,
> TCPIP_MSG_INPKT_6LOWPAN, etc


That would indeed be the cleanest approach to solve this.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Mar 2015 05:25:40 PM UTC, comment #11: 


> I'm not sure you understand the problem behind PPP_INPROC_MULTITHREADED


Well, I'm sure do, even though I have followed that only partly.

And even if I would have preferred to keep the unescaping and packet deframing in interrupt context, without having to switch into tcpip_thread for every byte (it was valid to call pppos_input for each byte), I accept that it's possibly easier this way to get it thread safe.

However, when asking how this was previously solved, I was rather thinking about how did the old version get into tcpip_thread. And looking at the old sources, it used 'tcpip_callback', not 'tcpip_input'. Now I'm just curious why this has been changed, even if the change interferes with other modules.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Mar 2015 05:16:43 PM UTC, comment #10: 


> Maybe we should add some new types such as TCPIP_MSG_INPKT_PPPOS, TCPIP_MSG_INPKT_ETHERNET, TCPIP_MSG_INPKT_6LOWPAN, etc


I really, really, really like this idea :)

Sylvain Rochet <gradator>
Group Member
Wed 18 Mar 2015 05:11:36 PM UTC, comment #9: 

Hello Simon,

> Ehrm, I'd rather revert the part where the input callback is used for something it's not meant to.


I'm not sure you understand the problem behind PPP_INPROC_MULTITHREADED even though I did very detailed commits log and documentation about what is wrong about it :)

My first fix using a mutex was a way to fix it but you were right about users using it in interrupt context :-)


> How has this been solved before, anyway?


It was not solved at all, this is actually the fix. We were all using the non-thread safe function pppos_input() even if PPP_INPROC_MULTITHREADED is set, there is probably a lot of devices in the wild using this non thread safe way, especially because it was documented as being thread safe, Oops :(


Sylvain

Sylvain Rochet <gradator>
Group Member
Wed 18 Mar 2015 05:04:40 PM UTC, comment #8: 

Another option, for this and other cases is:

Now we use msg->type == TCPIP_MSG_INPKT

Maybe we should add some new types such as TCPIP_MSG_INPKT_PPPOS, TCPIP_MSG_INPKT_ETHERNET, TCPIP_MSG_INPKT_6LOWPAN, etc

Then we don't need all these flags.

Ivan Delamer <idelamer>
Group Member
Wed 18 Mar 2015 04:59:22 PM UTC, comment #7: 


> So, should we set the netif flags to u16_t and then add a
> USE_PPPOS_INPUT_FONCTION flag ?


Ehrm, I'd rather revert the part where the input callback is used for something it's not meant to.

How has this been solved before, anyway?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Mar 2015 04:53:26 PM UTC, comment #6: 

I did submit the fix, but I think it is better to add a PPP flag to the netif and trigger a special case. like for ethernet.

Ivan Delamer <idelamer>
Group Member
Wed 18 Mar 2015 04:44:44 PM UTC, comment #5: 

Hello,

> Sorry for not taking part earlier, but in my opinion, misusing the input callback here is the bug, not breaking SLIP.


> We can argue to omit the input pointer, but making it mandatory for PPP(oS) and at the same time saying we should omit it for the rest sounds strange.


That's not what I meant. I said this was actually not a callback since is was not previously used by the lwIP core code. So the input pointer was previously not necessary at all, (but now it is). So even if I disagree with this non-callback, I was happy to be able to recycle the previously useless (now useful) callback pointer :P


> Can't PPP find a different way of feeding packets into itself? Using the tcpip_input way here looks wrong.
>
> Plus: we just removed the Point-To-Point flag from netif.h, but this sounds like we could still need it.


Yes, what we actually need is a USE_PPPOS_INPUT_FONCTION flag :-)   When I added the feature I was like: "dammit, there is no free flag and I need one !".

So, should we set the netif flags to u16_t and then add a USE_PPPOS_INPUT_FONCTION flag ?

Sylvain

Sylvain Rochet <gradator>
Group Member
Wed 18 Mar 2015 04:33:45 PM UTC, comment #4: 

Sorry for not taking part earlier, but in my opinion, misusing the input callback here is the bug, not breaking SLIP.

We can argue to omit the input pointer, but making it mandatory for PPP(oS) and at the same time saying we should omit it for the rest sounds strange.

Can't PPP find a different way of feeding packets into itself? Using the tcpip_input way here looks wrong.

Plus: we just removed the Point-To-Point flag from netif.h, but this sounds like we could still need it.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Mar 2015 04:05:32 PM UTC, comment #3: 

Yes, I think the input function is a bit historical/legacy, but useful if yo have protocols other than ip/ip6, such as ARP or in your case with PPP.

I will apply the fix soon.

cheers

Ivan Delamer <idelamer>
Group Member
Tue 17 Mar 2015 11:06:40 PM UTC, comment #2: 

Hello Ivan,

I agree this is error prone. I didn't want to add a pointer to the netif structure and used the previously unused netif input pointer on PPP. I know I am using it "reversed".

I consider this input pointer a mistake, it is not used in the lwIP stack at all before this code was added, the user should in my opinion call tcpip_input() and not netif->input() the user set itself to tcpip_input(), this is not what a callback is meant to be :-)   (But now that I am using it, it's not a mistake anymore, eh! eh!)

Good catch, (note the FIXME, I had concern about SLIP ;-), I just expected no SLIP user would actually use the input pointer, I was erm. plain wrong.

Anyway, yep!, the "&& (msg->msg.inp.netif->input != tcpip_input) " is a perfectly fine workaround for me, go ahead ! :)

Sylvain

Sylvain Rochet <gradator>
Group Member
Tue 17 Mar 2015 10:58:03 PM UTC, comment #1: 

I tested adding this line:

  && (msg->msg.inp.netif->input != tcpip_input)

and it seems to work. Sylvain, do you think that would be OK?

Ivan Delamer <idelamer>
Group Member
Tue 17 Mar 2015 10:54:46 PM UTC, original submission:  

PPPOS uses the tcpip thread to process incoming packets. This code was added to tcpip_thread():

#if PPPOS_SUPPORT
      /* FIXME: can be generalized to all point to point interfaces */
      if ((msg->msg.inp.netif->flags & NETIF_FLAG_BROADCAST) == 0
            && msg->msg.inp.netif->input) {
        msg->msg.inp.netif->input(msg->msg.inp.p, msg->msg.inp.netif);
      } else
#endif /* PPPOS_SUPPORT */

I use SLIP netif, where netif->input = tcpip_input;

So every time we get a slip packet it keeps putting it into the tcpip queue forever.

I see how this works well for PPPOS. I guess we could add a condition  to the IF statement:
  
   && (msg->msg.inp.netif->input != tcpip_input)

Cheers
Ivan

Ivan Delamer <idelamer>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by idelamer (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-03-20 gradator StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2015-03-18 gradator StatusFixed In Progress
        Open/ClosedClosed Open
    2015-03-18 gradator Attached File- Added 0001-PPP-PPPoS-improved-tcpip-input-path.patch, #33392
    2015-03-18 idelamer StatusReady For Test Fixed
        Assigned toNone idelamer
        Open/ClosedOpen Closed
    2015-03-17 idelamer StatusNone Ready For Test

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code