hdlc_ppp: why no detach()?
Subsystems (2)
Hello Krzyztof,
I am looking at your hdlc_ppp code and I don't understand: why is there not the equivalent of fr_detach() in there?
pc8300_drv:cpc_remove_one() frees netdevs quite confidently but I wonder how it can be so sure that there are not skbs in hdlc_ppp's tx_queue associated with those devices before freeing them....
Even if you wanted to switch a device from PPP to Frame Relay, I don't see the method right now. If I may ask, please, what am I missing?
If you agree there is a need for detach(), I would be happy to work on it and make a submission.
thanks for your time,
Signature · 2 lines
Michael Barkowski RuggedCom, Inc.
Quoted 9 lines · Michael Barkowski
Hello Krzyztof,
I am looking at your
hdlc_pppcode and I don't understand: why is there not the equivalent offr_detach()in there?pc8300_drv:cpc_remove_one() frees netdevs quite confidently but I wonder how it can be so sure that there are not skbs in hdlc_ppp's
tx_queueassociated with those devices before freeing them....
the above is the real danger I see - free the netdev, then ppp's timer comes along and dequeues from tx_queue an skb with invalid device.
Quoted 3 lines
Even if you wanted to switch a device from PPP to Frame Relay, I don't see the method right now. If I may ask, please, what am I missing?
Ok - this part was a momentary lapse on my part - please strike from the record :)
Quoted 5 lines
If you agree there is a need for
detach(), I would be happy to work on it and make a submission.thanks for your time,
Signature · 2 lines
Michael Barkowski 905-482-4577
Hello Michael,
Quoted 2 lines · Michael Barkowski
I am looking at your
hdlc_pppcode and I don't understand: why is there not the equivalent offr_detach()in there?
I assume you mean .detach = fr_destroy(). It's used only to kill subdevices, i.e. it has nothing to do with the interface being up/down.
Quoted 3 lines
pc8300_drv:cpc_remove_one() frees netdevs quite confidently but I wonder how it can be so sure that there are not skbs in hdlc_ppp's
tx_queueassociated with those devices before freeing them....q
Theoretically all paths adding skbs to the tx_queue should send them out before returning (possibly also on behalf of other devices). However I wonder if it's the case. Let's see: Only ppp_tx_cp() adds to the queue directly:
- ppp_rx() (calls ppp_tx_flush())
- ppp_timer (calls ppp_tx_flush())
- ppp_cp_event():
- ppp_cp_parse_cr() (calls ppp_tx_flush())
- ppp_stop() calls ppp_cp_event(), but it won't queue any skb, it only marks the connection as closed and does the same to IPCP and IPV6CP.
This means the problematic part is ppp_start() which calls ppp_cp_event(LCP, START) = IRC | SCR | 3 meaning Initialize-Restart-Count, Send-Configure-Request and change state to REQ_SENT. This causes two problems:
1. The SCR packet will be delayed by 2 seconds (both first and second SCR will be sent the same time). Perhaps we delay only a little (instead of full 2 seconds) and only then send the initial packet.
2. (as you noted) the skb will be added to tx_queue and left there. If we happen to "ifconfig up" and "rmmod driver" before receiving any packet and before ppp->req_timeout (2 seconds) and before any other PPP interface does the same, we will eventually get skb with invalid ->dev. This is simple to drain in .close (detach is a wrong place since it may be called long after the interface is deactivated, there is no need to delay it past .close). The fix for #1 will already fix #2, but the redundant safety doesn't cost us anything.
Thanks for noting the problem, I'll post a patch shortly.
Also it seems the timeouts etc. should be configurable. ATM we're only fixing bugs, good.
Signature · 1 line
Krzysztof Halasa
Quoted 3 lines
1. The SCR packet will be delayed by 2 seconds (both first and second SCR will be sent the same time). Perhaps we delay only a little (instead of full 2 seconds) and only then send the initial packet.
BTW can I sleep in a netdevice_notifier callback, no?
Signature · 1 line
Krzysztof Halasa