Re: [PATCH v3 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

[Guillaume Nault <gnault@redhat.com>]

On Fri, Dec 04, 2020 at 04:36:55PM +0000, Tom Parkin wrote:
> +static int ppp_unbridge_channels(struct channel *pch)
> +{
> +	struct channel *pchb, *pchbb;
> +
> +	write_lock_bh(&pch->upl);
> +	pchb = rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl));
> +	if (!pchb) {
> +		write_unlock_bh(&pch->upl);
> +		return -EINVAL;
> +	}
> +	RCU_INIT_POINTER(pch->bridge, NULL);
> +	write_unlock_bh(&pch->upl);
> +
> +	write_lock_bh(&pchb->upl);
> +	pchbb = rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl));
> +	if (pchbb == pch)
> +		RCU_INIT_POINTER(pchb->bridge, NULL);
> +	write_unlock_bh(&pchb->upl);
> +
> +	synchronize_rcu();
> +
> +	if (pchbb == pch)
> +		if (refcount_dec_and_test(&pch->file.refcnt))
> +			ppp_destroy_channel(pch);

Since a respin is needed (see below), maybe add a comment explaining
why we need to verify that pchbb == pch.

> +	if (refcount_dec_and_test(&pchb->file.refcnt))
> +		ppp_destroy_channel(pchb);
> +
> +	return 0;
> +}
> +
>  static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct ppp_file *pf;
> @@ -641,8 +714,9 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	}
>  
>  	if (pf->kind == CHANNEL) {
> -		struct channel *pch;
> +		struct channel *pch, *pchb;
>  		struct ppp_channel *chan;
> +		struct ppp_net *pn;
>  
>  		pch = PF_TO_CHANNEL(pf);
>  
> @@ -657,6 +731,29 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  			err = ppp_disconnect_channel(pch);
>  			break;
>  
> +		case PPPIOCBRIDGECHAN:
> +			if (get_user(unit, p))
> +				break;
> +			err = -ENXIO;
> +			pn = ppp_pernet(current->nsproxy->net_ns);
> +			spin_lock_bh(&pn->all_channels_lock);
> +			pchb = ppp_find_channel(pn, unit);
> +			/* Hold a reference to prevent pchb being freed while
> +			 * we establish the bridge.
> +			 */
> +			if (pchb)
> +				refcount_inc(&pchb->file.refcnt);

The !pchb case isn't handled. With this code, if ppp_find_channel()
returns NULL, ppp_bridge_channels() will crash when trying to lock
pchb->upl.

> +			spin_unlock_bh(&pn->all_channels_lock);
> +			err = ppp_bridge_channels(pch, pchb);
> +			/* Drop earlier refcount now bridge establishment is complete */
> +			if (refcount_dec_and_test(&pchb->file.refcnt))
> +				ppp_destroy_channel(pchb);
> +			break;
> +

The rest looks good to me.