Date:   Tue, 17 Nov 2020 12:26:38 +0000
From:   Tom Parkin <tparkin@...alix.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     netdev@...r.kernel.org, jchapman@...alix.com
Subject: Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

On  Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote:
> On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> > On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > > On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > > > +				err = 0;
> > > > +			}
> > > > +			spin_unlock_bh(&pn->all_channels_lock);
> > > > +			break;
> > > >  		default:
> > > >  			down_read(&pch->chan_sem);
> > > >  			chan = pch->chan;
> > > > @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> > > >  		return;
> > > >  	}
> > > >  
> > > > +	if (pch->bridge) {
> > > > +		skb_queue_tail(&pch->bridge->file.xq, skb);
> > > 
> > > The bridged channel might reside in a different network namespace.
> > > So it seems that skb_scrub_packet() is needed before sending the
> > > packet.
> > 
> > I'm not sure about this.
> > 
> > PPPIOCBRIDGECHAN is looking up the bridged channel in the ppp_pernet
> > list.  Unless the channel can migrate across network namespaces after
> > the bridge is set up I don't think it would be possible for the
> > bridged channel to be in a different namespace.
> > 
> > Am I missing something here?
> 
> So yes, channels can't migrate across namespaces. However, the bridged
> channel is looked up from the caller's current namespace, which isn't
> guaranteed to be the same namespace as the channel used in the ioctl().
> 
> For example:
> 
> setns(ns1, CLONE_NEWNET);
> chan_ns1 = open("/dev/ppp");
> ...
> setns(ns2, CLONE_NEWNET);
> chan_ns2 = open("/dev/ppp");
> ...
> ioctl(chan_ns1, PPPIOCBRIDGECHAN, chan_ns2_id);
> 
> Here, chan_ns1 belongs to ns1, but chan_ns2_id will be looked up in the
> context of ns2. I find it nice to have the possibility to bridge
> channels from different namespaces, but we have to handle the case
> correctly.

Ah, of course, I see what you're saying.

Agreed we should add the skb_scrub_packet() call.

> > > > +		ppp_channel_push(pch->bridge);
> > > 
> > > I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
> > > is necessary. We're not going through a PPP unit, so we have no risk of
> > > recursion here. Also, if ->start_xmit() fails, I see no reason for
> > > requeuing the skb, like __ppp_channel_push() does. I'd have to think
> > > more about it, but I believe that we could call the channel's
> > > ->start_xmit() function directly (respecting the locking constraints
> > > of course).
> > 
> > I take your point about re-queuing based on the return of
> > ->start_xmit().  For pppoe and pppol2tp start_xmit just swallows the
> > skb on failure in any case, so for this specific usecase queuing is
> > not an issue.
> 
> Indeed.
> 
> > However, my primary motivation for using ppp_channel_push was actually
> > the handling for managing dropping the packet if the channel was
> > deregistered.
> 
> I might be missing something, but I don't see what ppp_channel_push()
> does appart from holding the xmit lock and handling the xmit queue.
> If we agree that there's no need to use the xmit queue, all
> ppp_channel_push() does for us is taking pch->downl, which we probably
> can do on our own.
> 
> > It'd be simple enough to add another function which performed the same
> > deregistration check but didn't transmit via. the queue.
> 
> That's probably what I'm missing: what do you mean by "deregistration
> check"? I can't see anything like this in ppp_channel_push().

It's literally just the check on pch->chan once pch->downl is held.
So it would be trivial to do the same thing in a different codepath: I
just figured why reinvent the wheel :-)

Sorry for the confusion.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists