Re:[PATCH net] ppp: fix xmit recursion detection on ppp channels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


At 2017-08-08 17:43:24, "Guillaume Nault" <g.nault@xxxxxxxxxxxx> 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.nault@xxxxxxxxxxxx>
> 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

> }
> /*
?韬{.n?壏煯壄?%娝?檩?w?{.n?壏{炳歩?韰骅w*jg?秹殠娸?G珴?⒏⒎:+v墾妛鑚豰稛??畐娻"穐殢鉂?嗁?

[Index of Archives]     [Linux Audio Users]     [Linux for Hams]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Fedora Users]

  Powered by Linux