Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201228025339.3210-1-xie.he.0141@gmail.com> <20201228091654.1963-1-hdanton@sina.com>
In-Reply-To: <20201228091654.1963-1-hdanton@sina.com>
From:   Xie He <xie.he.0141@gmail.com>
Date:   Mon, 28 Dec 2020 02:22:37 -0800
Message-ID: <CAJht_ENqaJtUwfcXCC2WXNJi+9_HuYX4SrY-QZd19mZrL0EHBg@mail.gmail.com>
Subject: Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called
 while timer is running
To:     Hillf Danton <hdanton@sina.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Krzysztof Halasa <khc@pm.waw.pl>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Dec 28, 2020 at 1:17 AM Hillf Danton <hdanton@sina.com> 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.