Re: [lwip-devel] ppp-new

[Sylvain Rochet]

Hi Ivan,


On Wed, Jul 04, 2012 at 12:06:51PM -0600, Ivan Delamer wrote:
> Sylvain,
> 
> I tried to run the ppp-new branch, but unfortunately I got some hard
> faults. Probably something wrong in my configuration, but I need to dig
> deeper and will find time to do this in a couple weeks.

Well, this is a bad news :-)  How are you starting the PPP session ?


> Some suggestions based on what I saw:
> - change uint_32_t type declarations (already done, I see!)

Yeah, I saw the issue about uint32_t when building for my AVR32 target.


> - in magic.c, lets try to avoid library srand() and rand() functions. 
> We have a LWIP_RAND() definition which is used in IGMP. Could we reuse 
> this? The reason is that the library introduces all sorts of 
> additional code to support rand functions -> code bloating. Otherwise, 
> I can provide a lightweight rand() implementation that I use.

srand() and rand() are only used if PPP_MD5_RANDM is false, otherwise 
the magic module use MD5 as random pool + entropy from pseudo randoms 
events like packet input, the combination of both provide an acceptable 
random source.

Maybe we should move to the core code the PPP random module and let the 
user the choice of using the provided random functions or not.

PPP is using random for cryptography, it requires the random pool to be 
"churned" so that we get not-so predictable random challenges.

I am not a entropy-random-crypto-... expert, but I guess that using 
LWIP_RAND() in PPP require at least a new LWIP_CHURNRAND() definition. 
Doing changes without enough cryptography knowledge in a random 
generator used for cryptography might end up in a terrible disaster, 
this is why I didn't change much the random code used in the previous 
PPP port.


> - are there any thread-safety issue with the public API? I am using 
> tcpip callbacks for sigHUP, but calling other functions directly. Any 
> comments?

This is a good question, quick look results:

ppp_new() is thread safe, it only use memp_malloc() and change its own 
allocated memory.

ppp_set_auth() is of course thread safe too

regarding I/O, for PPPoS:

ppp_write() might be called from ppp_over_serial_open() (first LCP MRU 
sent to the peer) and is called from the thread calling pppos_input() 
regarding PPP negotiation. Operating in full duplex mode, it might 
happens that ppp_write() suffer a serialisation issue if we receive a 
LCP requests while sending the first LCP MRU request to the peer, at 
least, in theory it could.

regarding I/O, for PPPoE:

pppoe_create() is thread safe

pppoe_output() called by pppoe_send_padi() called by pppoe_connect() 
called by ppp_over_ethernet_open() called from a thread other than the 
lwIP thread is calling ethif->linkoutput(), I guess it suffers from a 
serialisation issue, and not only in theory. I don't think the 
linkoutput() to be thread safe, and if so, it depends on the port, I 
don't think we require the port to provide a serialized linkoutput() 
callback.

I think we can stop the quick overview here, we need to provide 
a netconn()-like API for PPP.


> Thanks for the great work! I noticed RAM use decreased,

Yeah, a PPP session without much features compiled-in (PAP+CHAP) use 
about 1 KiB of RAM for PPPoE, and about 1 KiB + PPPOS_RX_BUF for PPPoS.


> but flash increased by about 6K. Will have to review new PPP options 
> to see if I can decrease this.

This is not so much, less than I expected first :-)

There is however probably some other useless features that can be set at 
compile-time options.


Sylvain

signature.asc
Description: Digital signature