From: Tom Parkin <tparkin@katalix•com>
To: Guillaume Nault <gnault@redhat•com>
Cc: netdev@vger•kernel.org, jchapman@katalix•com
Subject: Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl
Date: Tue, 10 Nov 2020 12:04:29 +0000 [thread overview]
Message-ID: <20201110120429.GB5635@katalix.com> (raw)
In-Reply-To: <20201109232401.GM2366@linux.home>
[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]
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:
> > + case PPPIOCBRIDGECHAN:
> > + if (get_user(unit, p))
> > + break;
> > + err = -ENXIO;
> > + if (pch->bridge) {
> > + err = -EALREADY;
> > + break;
> > + }
> > + pn = ppp_pernet(current->nsproxy->net_ns);
> > + spin_lock_bh(&pn->all_channels_lock);
> > + pchb = ppp_find_channel(pn, unit);
> > + if (pchb) {
> > + refcount_inc(&pchb->file.refcnt);
> > + pch->bridge = pchb;
>
> I think we shouldn't modify ->bridge if it's already set or if the
> channel is already part of of a PPP unit (that is, if pch->ppp or
> pch->bridge is not NULL).
>
> This also means that we have to use appropriate locking.
Yes, good point about checking for the channel being part of a PPP
unit.
> > + 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?
>
> > + 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.
However, my primary motivation for using ppp_channel_push was actually
the handling for managing dropping the packet if the channel was
deregistered.
It'd be simple enough to add another function which performed the same
deregistration check but didn't transmit via. the queue.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-11-10 12:04 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-06 18:16 [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels Tom Parkin
2020-11-06 18:16 ` [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl Tom Parkin
2020-11-09 23:24 ` Guillaume Nault
2020-11-10 12:04 ` Tom Parkin [this message]
2020-11-15 16:28 ` Guillaume Nault
2020-11-17 12:26 ` Tom Parkin
2020-11-17 14:06 ` Guillaume Nault
2020-11-06 18:16 ` [RFC PATCH 2/2] docs: update ppp_generic.rst to describe ioctl PPPIOCBRIDGECHAN Tom Parkin
2020-11-09 22:51 ` [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels Guillaume Nault
2020-11-10 11:54 ` Tom Parkin
2020-11-15 11:59 ` Guillaume Nault
2020-11-17 12:12 ` Tom Parkin
2020-11-09 23:52 ` Jakub Kicinski
2020-11-10 9:28 ` Guillaume Nault
2020-11-10 12:42 ` Tom Parkin
2020-11-10 15:02 ` Guillaume Nault
2020-11-10 16:47 ` Jakub Kicinski
2020-11-17 12:54 ` Tom Parkin
2020-11-17 14:17 ` Guillaume Nault
2020-11-17 16:52 ` Jakub Kicinski
2020-11-18 20:24 ` Guillaume Nault
2020-11-20 1:17 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201110120429.GB5635@katalix.com \
--to=tparkin@katalix$(echo .)com \
--cc=gnault@redhat$(echo .)com \
--cc=jchapman@katalix$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox