Re: Fw: [Bug 199109] New: pptp: kernel printk "recursion detected", and then reboot itself

[Guillaume Nault <g.nault@alphalink.fr>]

On Fri, Mar 16, 2018 at 09:02:40PM +0100, Guillaume Nault wrote:
> On Fri, Mar 16, 2018 at 02:49:40PM +0800, xu heng wrote:
> > 
> >         For testing, in __ppp_channel_push(), disable sending anything from the attached unit, just disable __ppp_xmit_process(ppp) in __ppp_channel_push(). In my opinion, __ppp_xmit_process() should only called by ppp_xmit_process(), because of ppp->xmit_recursion __percpu.
> > 
> ppp_channel_push() needs to call __ppp_xmit_process() because some
> drivers (like ppp_async) need to notify ppp_generic when they can
> accept new packets. This is done by ppp_output_wakeup() which then
> calls ppp_channel_push(). So we have to handle the unit backlog there.
> 
> 
> Please try the following patch (untested).
> 
FYI, I've now fully tested the patch. I'm going to send it formally.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index b883af93929c..af22eb11bbaa 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -255,7 +255,7 @@ struct ppp_net {
>  /* Prototypes. */
>  static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
>  			struct file *file, unsigned int cmd, unsigned long arg);
> -static void ppp_xmit_process(struct ppp *ppp);
> +static void ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb);
>  static void ppp_send_frame(struct ppp *ppp, struct sk_buff *skb);
>  static void ppp_push(struct ppp *ppp);
>  static void ppp_channel_push(struct channel *pch);
> @@ -511,13 +511,12 @@ static ssize_t ppp_write(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>  
> -	skb_queue_tail(&pf->xq, skb);
> -
>  	switch (pf->kind) {
>  	case INTERFACE:
> -		ppp_xmit_process(PF_TO_PPP(pf));
> +		ppp_xmit_process(PF_TO_PPP(pf), skb);
>  		break;
>  	case CHANNEL:
> +		skb_queue_tail(&pf->xq, skb);
>  		ppp_channel_push(PF_TO_CHANNEL(pf));
>  		break;
>  	}
> @@ -1260,8 +1259,8 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	put_unaligned_be16(proto, pp);
>  
>  	skb_scrub_packet(skb, !net_eq(ppp->ppp_net, dev_net(dev)));
> -	skb_queue_tail(&ppp->file.xq, skb);
> -	ppp_xmit_process(ppp);
> +	ppp_xmit_process(ppp, skb);
> +
>  	return NETDEV_TX_OK;
>  
>   outf:
> @@ -1415,13 +1414,14 @@ static void ppp_setup(struct net_device *dev)
>   */
>  
>  /* Called to do any work queued up on the transmit side that can now be done */
> -static void __ppp_xmit_process(struct ppp *ppp)
> +static void __ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
>  {
> -	struct sk_buff *skb;
> -
>  	ppp_xmit_lock(ppp);
>  	if (!ppp->closing) {
>  		ppp_push(ppp);
> +
> +		if (skb)
> +			skb_queue_tail(&ppp->file.xq, skb);
>  		while (!ppp->xmit_pending &&
>  		       (skb = skb_dequeue(&ppp->file.xq)))
>  			ppp_send_frame(ppp, skb);
> @@ -1435,7 +1435,7 @@ static void __ppp_xmit_process(struct ppp *ppp)
>  	ppp_xmit_unlock(ppp);
>  }
>  
> -static void ppp_xmit_process(struct ppp *ppp)
> +static void ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
>  {
>  	local_bh_disable();
>  
> @@ -1443,7 +1443,7 @@ static void ppp_xmit_process(struct ppp *ppp)
>  		goto err;
>  
>  	(*this_cpu_ptr(ppp->xmit_recursion))++;
> -	__ppp_xmit_process(ppp);
> +	__ppp_xmit_process(ppp, skb);
>  	(*this_cpu_ptr(ppp->xmit_recursion))--;
>  
>  	local_bh_enable();
> @@ -1452,6 +1452,7 @@ static void ppp_xmit_process(struct ppp *ppp)
>  
>  err:
>  	local_bh_enable();
> +	kfree_skb(skb);
>  
>  	if (net_ratelimit())
>  		netdev_err(ppp->dev, "recursion detected\n");
> @@ -1937,7 +1938,7 @@ static void __ppp_channel_push(struct channel *pch)
>  	if (skb_queue_empty(&pch->file.xq)) {
>  		ppp = pch->ppp;
>  		if (ppp)
> -			__ppp_xmit_process(ppp);
> +			__ppp_xmit_process(ppp, NULL);
>  	}
>  }
>  
>