Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called while timer is running

[Xie He]

On Mon, Dec 28, 2020 at 1:17 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Sun, 27 Dec 2020 18:53:39 -0800 Xie He wrote:
> > ppp_cp_event is called directly or indirectly by ppp_rx with "ppp->lock"
> > held. It may call mod_timer to add a new timer. However, at the same time
> > ppp_timer may be already running and waiting for "ppp->lock". In this
> > case, there's no need for ppp_timer to continue running and it can just
> > exit.
>
> Because the timer callback loses the race in acquiring the ppp->lock
> does not mean it should abort.

I think aborting ppp_timer is the correct solution. When mod_timer is
called by ppp_cp_event, which is (directly or indirectly) called by
ppp_rx, this means we received something on the line that makes the
original timer no longer necessary. If the timer is pending, mod_timer
will delete it. If the timer is running and waiting for the lock, I
think it should abort. This way it would appear that the timer hasn't
fired and is deleted before it fires.

> > If we let ppp_timer continue running, it may call add_timer. This causes
> > kernel panic because add_timer can't be called with a timer pending.
>
> Meanwhie we can defuse the peril following add_timer() added in
> e022c2f07ae5, say by replacing add_timer() with mod_timer().

The code path that calls add_timer is for sending keep-alive packets
when operating in the OPENED state. If we have just changed to the
OPENED state in ppp_cp_event, we should wait for the amount of time
set by the (2nd) mod_timer call in ppp_cp_event, before firing the
timer. We shouldn't fire the timer immediately after we change to the
OPENED state. This is not the intention of the (2nd) mod_timer call in
ppp_cp_event. Therefore aborting ppp_timer is the correct solution.