[Patch net] ppp: defer netns reference release for ppp channel

[Patch net] ppp: defer netns reference release for ppp channel

[Cong Wang]

Matt reported that we have a NULL pointer dereference
in ppp_pernet() from ppp_connect_channel(),
i.e. pch->chan_net is NULL.

This is due to that a parallel ppp_unregister_channel()
could happen while we are in ppp_connect_channel(), during
which pch->chan_net set to NULL. Since we need a reference
to net per channel, it makes sense to sync the refcnt
with the life time of the channel, therefore we should
release this reference when we destroy it.

Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
Reported-by: Matt Bennett <Matt.Bennett@alliedtelesis.co.nz>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linux-ppp@vger.kernel.org
Cc: Guillaume Nault <g.nault@alphalink.fr>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/ppp/ppp_generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 8dedafa..a30ee42 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2601,8 +2601,6 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	spin_lock_bh(&pn->all_channels_lock);
 	list_del(&pch->list);
 	spin_unlock_bh(&pn->all_channels_lock);
-	put_net(pch->chan_net);
-	pch->chan_net = NULL;
 
 	pch->file.dead = 1;
 	wake_up_interruptible(&pch->file.rwait);
@@ -3136,6 +3134,9 @@ ppp_disconnect_channel(struct channel *pch)
  */
 static void ppp_destroy_channel(struct channel *pch)
 {
+	put_net(pch->chan_net);
+	pch->chan_net = NULL;
+
 	atomic_dec(&channel_count);
 
 	if (!pch->file.dead) {
-- 
1.8.4.5

Re: [Patch net] ppp: defer netns reference release for ppp channel

[Cyrill Gorcunov]

On Tue, Jul 05, 2016 at 10:12:36PM -0700, Cong Wang wrote:
> Matt reported that we have a NULL pointer dereference
> in ppp_pernet() from ppp_connect_channel(),
> i.e. pch->chan_net is NULL.
> 
> This is due to that a parallel ppp_unregister_channel()
> could happen while we are in ppp_connect_channel(), during
> which pch->chan_net set to NULL. Since we need a reference
> to net per channel, it makes sense to sync the refcnt
> with the life time of the channel, therefore we should
> release this reference when we destroy it.
> 
> Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
> Reported-by: Matt Bennett <Matt.Bennett@alliedtelesis.co.nz>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linux-ppp@vger.kernel.org
> Cc: Guillaume Nault <g.nault@alphalink.fr>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---

Hi Cong! I may be wrong, but this doesn't look right in general.
We take the net in ppp_register_channel->ppp_register_net_channel
and (name) context implies that ppp_unregister_channel does
the reverse. Maybe there some sync point missed? I'll review
in detail a bit later.

Re: [Patch net] ppp: defer netns reference release for ppp channel

[Cyrill Gorcunov]

On Wed, Jul 06, 2016 at 11:26:02AM +0300, Cyrill Gorcunov wrote:
> On Tue, Jul 05, 2016 at 10:12:36PM -0700, Cong Wang wrote:
> > Matt reported that we have a NULL pointer dereference
> > in ppp_pernet() from ppp_connect_channel(),
> > i.e. pch->chan_net is NULL.
> > 
> > This is due to that a parallel ppp_unregister_channel()
> > could happen while we are in ppp_connect_channel(), during
> > which pch->chan_net set to NULL. Since we need a reference
> > to net per channel, it makes sense to sync the refcnt
> > with the life time of the channel, therefore we should
> > release this reference when we destroy it.
> > 
> > Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
> > Reported-by: Matt Bennett <Matt.Bennett@alliedtelesis.co.nz>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: linux-ppp@vger.kernel.org
> > Cc: Guillaume Nault <g.nault@alphalink.fr>
> > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> 
> Hi Cong! I may be wrong, but this doesn't look right in general.
> We take the net in ppp_register_channel->ppp_register_net_channel
> and (name) context implies that ppp_unregister_channel does
> the reverse. Maybe there some sync point missed? I'll review
> in detail a bit later.

After staring more I think the patch should be fine as a fix
since implementing sync with ppp_[re|un]register_channel and
ppp_ioctl might need a way more work.

Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

Re: [Patch net] ppp: defer netns reference release for ppp channel

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue,  5 Jul 2016 22:12:36 -0700

> Matt reported that we have a NULL pointer dereference
> in ppp_pernet() from ppp_connect_channel(),
> i.e. pch->chan_net is NULL.
> 
> This is due to that a parallel ppp_unregister_channel()
> could happen while we are in ppp_connect_channel(), during
> which pch->chan_net set to NULL. Since we need a reference
> to net per channel, it makes sense to sync the refcnt
> with the life time of the channel, therefore we should
> release this reference when we destroy it.
> 
> Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
> Reported-by: Matt Bennett <Matt.Bennett@alliedtelesis.co.nz>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linux-ppp@vger.kernel.org
> Cc: Guillaume Nault <g.nault@alphalink.fr>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable.

Re: [Patch net] ppp: defer netns reference release for ppp channel

[Guillaume Nault]

On Wed, Jul 06, 2016 at 03:25:15PM +0300, Cyrill Gorcunov wrote:
> On Wed, Jul 06, 2016 at 11:26:02AM +0300, Cyrill Gorcunov wrote:
> > On Tue, Jul 05, 2016 at 10:12:36PM -0700, Cong Wang wrote:
> > > Matt reported that we have a NULL pointer dereference
> > > in ppp_pernet() from ppp_connect_channel(),
> > > i.e. pch->chan_net is NULL.
> > > 
> > > This is due to that a parallel ppp_unregister_channel()
> > > could happen while we are in ppp_connect_channel(), during
> > > which pch->chan_net set to NULL. Since we need a reference
> > > to net per channel, it makes sense to sync the refcnt
> > > with the life time of the channel, therefore we should
> > > release this reference when we destroy it.
> > > 
> > > Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
> > > Reported-by: Matt Bennett <Matt.Bennett@alliedtelesis.co.nz>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > > Cc: linux-ppp@vger.kernel.org
> > > Cc: Guillaume Nault <g.nault@alphalink.fr>
> > > Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > ---
> > 
> > Hi Cong! I may be wrong, but this doesn't look right in general.
> > We take the net in ppp_register_channel->ppp_register_net_channel
> > and (name) context implies that ppp_unregister_channel does
> > the reverse. Maybe there some sync point missed? I'll review
> > in detail a bit later.
> 
> After staring more I think the patch should be fine as a fix
> since implementing sync with ppp_[re|un]register_channel and
> ppp_ioctl might need a way more work.
> 

[Sorry for arriving so late in the game, I was offline the last 3 weeks]

I agree having some symmetry between the creation and deletion
processes would be nice and would make the code easier to reason about.
Actually, I released the channel netns in ppp_unregister_channel() for
exactly this reason (and failed to spot this race).

But the code is already quite asymmetric and it's certainly too late to
move away from this scheme now. So releasing the channel netns in
ppp_destroy_channel() is in line with ppp_generic's architecture. Other
data are handled this way: e.g. channel_count is incremented in
ppp_register_net_channel() and decremented in ppp_destroy_channel()).

Thank you all for testing and fixing this issue!

Guillaume

Re: [Patch net] ppp: defer netns reference release for ppp channel

[Cyrill Gorcunov]

On Thu, Jul 28, 2016 at 12:33:58PM +0200, Guillaume Nault wrote:
> > 
> > After staring more I think the patch should be fine as a fix
> > since implementing sync with ppp_[re|un]register_channel and
> > ppp_ioctl might need a way more work.
> > 
> 
> [Sorry for arriving so late in the game, I was offline the last 3 weeks]
> 
> I agree having some symmetry between the creation and deletion
> processes would be nice and would make the code easier to reason about.
> Actually, I released the channel netns in ppp_unregister_channel() for
> exactly this reason (and failed to spot this race).
> 
> But the code is already quite asymmetric and it's certainly too late to
> move away from this scheme now. So releasing the channel netns in

Yes, this module needs cleanup in general, so fix is fine, thanks!