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.
signature.asc
Description: PGP signature
