At 2017-08-08 17:43:24, "Guillaume Nault" <g.na...@alphalink.fr> wrote:
>Commit e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp
>devices") dropped the xmit_recursion counter incrementation in
>ppp_channel_push() and relied on ppp_xmit_process() for this task.
>But __ppp_channel_push() can also send packets directly (using the
>.start_xmit() channel callback), in which case the xmit_recursion
You're right. We lost this case.
>counter isn't incremented anymore. If such packets get routed back to
>the parent ppp unit, ppp_xmit_process() won't notice the recursion and
>will call ppp_channel_push() on the same channel, effectively creating
>the deadlock situation that the xmit_recursion mechanism was supposed
>to prevent.
>
>This patch re-introduces the xmit_recursion counter incrementation in
>ppp_channel_push(). Since the xmit_recursion variable is now part of
>the parent ppp unit, incrementation is skipped if the channel doesn't
>have any. This is fine because only packets routed through the parent
>unit may enter the channel recursively.
>
>Finally, we have to ensure that pch->ppp is not going to be modified
>while executing ppp_channel_push(). Instead of taking this lock only
>while calling ppp_xmit_process(), we now have to hold it for the full
>ppp_channel_push() execution. This respects the ppp locks ordering
>which requires locking ->upl before ->downl.
>
>Fixes: e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp
>devices")
>Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
>---
> drivers/net/ppp/ppp_generic.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
>index bd4303944e44..a404552555d4 100644
>--- a/drivers/net/ppp/ppp_generic.c
>+++ b/drivers/net/ppp/ppp_generic.c
>@@ -1915,21 +1915,23 @@ static void __ppp_channel_push(struct channel *pch)
> spin_unlock(&pch->downl);
> /* see if there is anything from the attached unit to be sent */
> if (skb_queue_empty(&pch->file.xq)) {
>- read_lock(&pch->upl);
> ppp = pch->ppp;
> if (ppp)
>- ppp_xmit_process(ppp);
>- read_unlock(&pch->upl);
>+ __ppp_xmit_process(ppp);
> }
> }
>
> static void ppp_channel_push(struct channel *pch)
> {
>- local_bh_disable();
>-
>- __ppp_channel_push(pch);
>-
>- local_bh_enable();
>+ read_lock_bh(&pch->upl);
>+ if (pch->ppp) {
>+ (*this_cpu_ptr(pch->ppp->xmit_recursion))++;
>+ __ppp_channel_push(pch);
>+ (*this_cpu_ptr(pch->ppp->xmit_recursion))--;
>+ } else {
>+ __ppp_channel_push(pch);
>+ }
>+ read_unlock_bh(&pch->upl);
If invoked read_lock_bh in ppp_channel_push, it would be unnecessary to invoke
read_lock(&pch->upl)
in the __ppp_channel_push.
Best Regards
Feng
> }
>
> /*
>--
>2.14.0
>
