[PATCH net] ppp: fix device unregistration upon netns deletion

[Guillaume Nault]

PPP devices may get automatically unregistered when their network
namespace is getting removed. This happens if the ppp control plane
daemon (e.g. pppd) exits while it is the last user of this namespace.

This leads to several races:

  * ppp_exit_net() may destroy the per namespace idr (pn-units_idr)
before all file descriptors were released. Successive ppp_release()
calls may then cleanup PPP devices with ppp_shutdown_interface() and
try to use the already destroyed idr.

  * Automatic device unregistration may also happen before the
ppp_release() call for that device gets executed. Once called on
the file owning the device, ppp_release() will then clean it up and
try to unregister it a second time.

To fix these issues, operations defined in ppp_shutdown_interface() are
moved to the PPP device's ndo_uninit() callback. This allows PPP
devices to be properly cleaned up by unregister_netdev() and friends.
So checking for ppp-owner is now an accurate test to decide if a PPP
device should be unregistered.

Setting ppp-owner is done in ppp_create_interface(), before device
registration, in order to avoid unprotected modification of this field.

Finally ppp_exit_net() now starts by unregistering all remaining PPP
devices to ensure that none will get unregistered after the call to
idr_destroy().

Signed-off-by: Guillaume Nault g.na...@alphalink.fr
---
 drivers/net/ppp/ppp_generic.c | 79 +++
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 9d15566..1dc478a 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -269,9 +269,9 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit, int *retp);
+static struct ppp *ppp_create_interface(struct net *net, int unit,
+   struct file *file, int *retp);
 static void init_ppp_file(struct ppp_file *pf, int kind);
-static void ppp_shutdown_interface(struct ppp *ppp);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
 static struct channel *ppp_find_channel(struct ppp_net *pn, int unit);
@@ -392,8 +392,10 @@ static int ppp_release(struct inode *unused, struct file 
*file)
file-private_data = NULL;
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_dec_and_test(pf-refcnt)) {
switch (pf-kind) {
@@ -593,8 +595,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
mutex_lock(ppp_mutex);
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_long_read(file-f_count)  2) {
ppp_release(NULL, file);
@@ -838,11 +842,10 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, err);
+   ppp = ppp_create_interface(net, unit, file, err);
if (!ppp)
break;
file-private_data = ppp-file;
-   ppp-owner = file;
err = -EFAULT;
if (put_user(ppp-file.index, p))
break;
@@ -916,6 +919,17 @@ static __net_init int ppp_init_net(struct net *net)
 static __net_exit void ppp_exit_net(struct net *net)
 {
struct ppp_net *pn = net_generic(net, ppp_net_id);
+   struct ppp *ppp;
+   LIST_HEAD(list);
+   int id;
+
+   rtnl_lock();
+   idr_for_each_entry(pn-units_idr, ppp, id) {
+   unregister_netdevice_queue(ppp-dev, list);
+   }
+
+   unregister_netdevice_many(list);
+   rtnl_unlock();
 
idr_destroy(pn-units_idr);
 }
@@ -1088,8 +1102,28 @@ static int ppp_dev_init(struct net_device *dev)
return 0;
 }
 
+static void ppp_dev_uninit(struct net_device *dev)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   struct ppp_net *pn = ppp_pernet(ppp-ppp_net

[PATCH net-next 2/2] ppp: implement x-netns support

[Guillaume Nault]

Let packets move from one netns to the other at PPP encapsulation and
decapsulation time.

PPP units and channels remain in the netns in which they were
originally created. Only the net_device may move to a different
namespace. Cross netns handling is thus transparent to lower PPP
layers (PPPoE, L2TP, etc.).

PPP devices are automatically unregistered when their netns gets
removed. So read() and poll() on the unit file descriptor will
respectively receive EOF and POLLHUP. Channels aren't affected.

Signed-off-by: Guillaume Nault g.na...@alphalink.fr
---
 drivers/net/ppp/ppp_generic.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1dc478a..bdde5d8 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -283,6 +283,8 @@ static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
 
+static const struct net_device_ops ppp_netdev_ops;
+
 static struct class *ppp_class;
 
 /* per net-namespace data */
@@ -919,13 +921,22 @@ static __net_init int ppp_init_net(struct net *net)
 static __net_exit void ppp_exit_net(struct net *net)
 {
struct ppp_net *pn = net_generic(net, ppp_net_id);
+   struct net_device *dev;
+   struct net_device *aux;
struct ppp *ppp;
LIST_HEAD(list);
int id;
 
rtnl_lock();
+   for_each_netdev_safe(net, dev, aux) {
+   if (dev-netdev_ops == ppp_netdev_ops)
+   unregister_netdevice_queue(dev, list);
+   }
+
idr_for_each_entry(pn-units_idr, ppp, id) {
-   unregister_netdevice_queue(ppp-dev, list);
+   /* Skip devices already unregistered by previous loop */
+   if (!net_eq(dev_net(ppp-dev), net))
+   unregister_netdevice_queue(ppp-dev, list);
}
 
unregister_netdevice_many(list);
@@ -1018,6 +1029,7 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device 
*dev)
proto = npindex_to_proto[npi];
put_unaligned_be16(proto, pp);
 
+   skb_scrub_packet(skb, !net_eq(ppp-ppp_net, dev_net(dev)));
skb_queue_tail(ppp-file.xq, skb);
ppp_xmit_process(ppp);
return NETDEV_TX_OK;
@@ -1138,7 +1150,6 @@ static void ppp_setup(struct net_device *dev)
dev-tx_queue_len = 3;
dev-type = ARPHRD_PPP;
dev-flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
-   dev-features |= NETIF_F_NETNS_LOCAL;
netif_keep_dst(dev);
 }
 
@@ -1901,6 +1912,8 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff 
*skb)
skb-dev = ppp-dev;
skb-protocol = htons(npindex_to_ethertype[npi]);
skb_reset_mac_header(skb);
+   skb_scrub_packet(skb, !net_eq(ppp-ppp_net,
+ dev_net(ppp-dev)));
netif_rx(skb);
}
}
-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 1/2] ppp: fix device unregistration upon netns deletion

[Guillaume Nault]

PPP devices may get automatically unregistered when their network
namespace is getting removed. This happens if the ppp control plane
daemon (e.g. pppd) exits while it is the last user of this namespace.

This leads to several races:

  * ppp_exit_net() may destroy the per namespace idr (pn-units_idr)
before all file descriptors were released. Successive ppp_release()
calls may then cleanup PPP devices with ppp_shutdown_interface() and
try to use the already destroyed idr.

  * Automatic device unregistration may also happen before the
ppp_release() call for that device gets executed. Once called on
the file owning the device, ppp_release() will then clean it up and
try to unregister it a second time.

To fix these issues, operations defined in ppp_shutdown_interface() are
moved to the PPP device's ndo_uninit() callback. This allows PPP
devices to be properly cleaned up by unregister_netdev() and friends.
So checking for ppp-owner is now an accurate test to decide if a PPP
device should be unregistered.

Setting ppp-owner is done in ppp_create_interface(), before device
registration, in order to avoid unprotected modification of this field.

Finally ppp_exit_net() now starts by unregistering all remaining PPP
devices to ensure that none will get unregistered after the call to
idr_destroy().

Signed-off-by: Guillaume Nault g.na...@alphalink.fr
---
 drivers/net/ppp/ppp_generic.c | 79 +++
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 9d15566..1dc478a 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -269,9 +269,9 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit, int *retp);
+static struct ppp *ppp_create_interface(struct net *net, int unit,
+   struct file *file, int *retp);
 static void init_ppp_file(struct ppp_file *pf, int kind);
-static void ppp_shutdown_interface(struct ppp *ppp);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
 static struct channel *ppp_find_channel(struct ppp_net *pn, int unit);
@@ -392,8 +392,10 @@ static int ppp_release(struct inode *unused, struct file 
*file)
file-private_data = NULL;
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_dec_and_test(pf-refcnt)) {
switch (pf-kind) {
@@ -593,8 +595,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
mutex_lock(ppp_mutex);
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_long_read(file-f_count)  2) {
ppp_release(NULL, file);
@@ -838,11 +842,10 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, err);
+   ppp = ppp_create_interface(net, unit, file, err);
if (!ppp)
break;
file-private_data = ppp-file;
-   ppp-owner = file;
err = -EFAULT;
if (put_user(ppp-file.index, p))
break;
@@ -916,6 +919,17 @@ static __net_init int ppp_init_net(struct net *net)
 static __net_exit void ppp_exit_net(struct net *net)
 {
struct ppp_net *pn = net_generic(net, ppp_net_id);
+   struct ppp *ppp;
+   LIST_HEAD(list);
+   int id;
+
+   rtnl_lock();
+   idr_for_each_entry(pn-units_idr, ppp, id) {
+   unregister_netdevice_queue(ppp-dev, list);
+   }
+
+   unregister_netdevice_many(list);
+   rtnl_unlock();
 
idr_destroy(pn-units_idr);
 }
@@ -1088,8 +1102,28 @@ static int ppp_dev_init(struct net_device *dev)
return 0;
 }
 
+static void ppp_dev_uninit(struct net_device *dev)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   struct ppp_net *pn = ppp_pernet(ppp-ppp_net

[PATCH net-next 0/2] ppp: implement x-netns support

[Guillaume Nault]

This series allows PPP devices to reside in a different netns from the
PPP unit/channels. Packets only cross netns boundaries when they're
transmitted between the net_device and the PPP unit (units and channels
always remain in their creation namespace).
So only PPP units need to handle cross namespace operations. Channels
and lower layer protocols aren't affected.

Patch #1 is a bug fix for an existing namespace deletion bug and has
been separetly sent to net.
Patch #2 is the actual x-netns implementation.


Guillaume Nault (2):
  ppp: fix device unregistration upon netns deletion
  ppp: implement x-netns support

 drivers/net/ppp/ppp_generic.c | 94 ++-
 1 file changed, 57 insertions(+), 37 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v2] ppp: implement x-netns support

[Guillaume Nault]

Let packets move from one netns to the other at PPP encapsulation and
decapsulation time.

PPP units and channels remain in the netns in which they were
originally created. Only the net_device may move to a different
namespace. Cross netns handling is thus transparent to lower PPP
layers (PPPoE, L2TP, etc.).

PPP devices are automatically unregistered when their netns gets
removed. So read() and poll() on the unit file descriptor will
respectively receive EOF and POLLHUP. Channels aren't affected.

Signed-off-by: Guillaume Nault g.na...@alphalink.fr
---
v2: rebase on top of net-next

 drivers/net/ppp/ppp_generic.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fa8f504..0481daf 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -283,6 +283,8 @@ static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
 
+static const struct net_device_ops ppp_netdev_ops;
+
 static struct class *ppp_class;
 
 /* per net-namespace data */
@@ -919,13 +921,22 @@ static __net_init int ppp_init_net(struct net *net)
 static __net_exit void ppp_exit_net(struct net *net)
 {
struct ppp_net *pn = net_generic(net, ppp_net_id);
+   struct net_device *dev;
+   struct net_device *aux;
struct ppp *ppp;
LIST_HEAD(list);
int id;
 
rtnl_lock();
+   for_each_netdev_safe(net, dev, aux) {
+   if (dev-netdev_ops == ppp_netdev_ops)
+   unregister_netdevice_queue(dev, list);
+   }
+
idr_for_each_entry(pn-units_idr, ppp, id)
-   unregister_netdevice_queue(ppp-dev, list);
+   /* Skip devices already unregistered by previous loop */
+   if (!net_eq(dev_net(ppp-dev), net))
+   unregister_netdevice_queue(ppp-dev, list);
 
unregister_netdevice_many(list);
rtnl_unlock();
@@ -1017,6 +1028,7 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device 
*dev)
proto = npindex_to_proto[npi];
put_unaligned_be16(proto, pp);
 
+   skb_scrub_packet(skb, !net_eq(ppp-ppp_net, dev_net(dev)));
skb_queue_tail(ppp-file.xq, skb);
ppp_xmit_process(ppp);
return NETDEV_TX_OK;
@@ -1137,7 +1149,6 @@ static void ppp_setup(struct net_device *dev)
dev-tx_queue_len = 3;
dev-type = ARPHRD_PPP;
dev-flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
-   dev-features |= NETIF_F_NETNS_LOCAL;
netif_keep_dst(dev);
 }
 
@@ -1900,6 +1911,8 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff 
*skb)
skb-dev = ppp-dev;
skb-protocol = htons(npindex_to_ethertype[npi]);
skb_reset_mac_header(skb);
+   skb_scrub_packet(skb, !net_eq(ppp-ppp_net,
+ dev_net(ppp-dev)));
netif_rx(skb);
}
}
-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net v2] ppp: fix device unregistration upon netns deletion

[Guillaume Nault]

PPP devices may get automatically unregistered when their network
namespace is getting removed. This happens if the ppp control plane
daemon (e.g. pppd) exits while it is the last user of this namespace.

This leads to several races:

  * ppp_exit_net() may destroy the per namespace idr (pn-units_idr)
before all file descriptors were released. Successive ppp_release()
calls may then cleanup PPP devices with ppp_shutdown_interface() and
try to use the already destroyed idr.

  * Automatic device unregistration may also happen before the
ppp_release() call for that device gets executed. Once called on
the file owning the device, ppp_release() will then clean it up and
try to unregister it a second time.

To fix these issues, operations defined in ppp_shutdown_interface() are
moved to the PPP device's ndo_uninit() callback. This allows PPP
devices to be properly cleaned up by unregister_netdev() and friends.
So checking for ppp-owner is now an accurate test to decide if a PPP
device should be unregistered.

Setting ppp-owner is done in ppp_create_interface(), before device
registration, in order to avoid unprotected modification of this field.

Finally, ppp_exit_net() now starts by unregistering all remaining PPP
devices to ensure that none will get unregistered after the call to
idr_destroy().

Signed-off-by: Guillaume Nault g.na...@alphalink.fr
---
v2: remove unnecessary curly braces in idr_for_each_entry()

 drivers/net/ppp/ppp_generic.c | 78 +++
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 9d15566..fa8f504 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -269,9 +269,9 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit, int *retp);
+static struct ppp *ppp_create_interface(struct net *net, int unit,
+   struct file *file, int *retp);
 static void init_ppp_file(struct ppp_file *pf, int kind);
-static void ppp_shutdown_interface(struct ppp *ppp);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
 static struct channel *ppp_find_channel(struct ppp_net *pn, int unit);
@@ -392,8 +392,10 @@ static int ppp_release(struct inode *unused, struct file 
*file)
file-private_data = NULL;
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_dec_and_test(pf-refcnt)) {
switch (pf-kind) {
@@ -593,8 +595,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
mutex_lock(ppp_mutex);
if (pf-kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
+   rtnl_lock();
if (file == ppp-owner)
-   ppp_shutdown_interface(ppp);
+   unregister_netdevice(ppp-dev);
+   rtnl_unlock();
}
if (atomic_long_read(file-f_count)  2) {
ppp_release(NULL, file);
@@ -838,11 +842,10 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, err);
+   ppp = ppp_create_interface(net, unit, file, err);
if (!ppp)
break;
file-private_data = ppp-file;
-   ppp-owner = file;
err = -EFAULT;
if (put_user(ppp-file.index, p))
break;
@@ -916,6 +919,16 @@ static __net_init int ppp_init_net(struct net *net)
 static __net_exit void ppp_exit_net(struct net *net)
 {
struct ppp_net *pn = net_generic(net, ppp_net_id);
+   struct ppp *ppp;
+   LIST_HEAD(list);
+   int id;
+
+   rtnl_lock();
+   idr_for_each_entry(pn-units_idr, ppp, id)
+   unregister_netdevice_queue(ppp-dev, list);
+
+   unregister_netdevice_many(list);
+   rtnl_unlock();
 
idr_destroy(pn-units_idr);
 }
@@ -1088,8 +1101,28 @@ static int ppp_dev_init(struct net_device *dev)
return 0;
 }
 
+static void ppp_dev_uninit(struct net_device *dev)
+{
+   struct ppp *ppp = netdev_priv(dev

Re: [PATCH net-next 0/2] ppp: implement x-netns support

[Guillaume Nault]

On Thu, Aug 13, 2015 at 09:20:04PM -0700, David Miller wrote:
 From: Guillaume Nault g.na...@alphalink.fr
 Date: Thu, 13 Aug 2015 15:28:02 +0200
 
  This series allows PPP devices to reside in a different netns from the
  PPP unit/channels. Packets only cross netns boundaries when they're
  transmitted between the net_device and the PPP unit (units and channels
  always remain in their creation namespace).
  So only PPP units need to handle cross namespace operations. Channels
  and lower layer protocols aren't affected.
  
  Patch #1 is a bug fix for an existing namespace deletion bug and has
  been separetly sent to net.
  Patch #2 is the actual x-netns implementation.
 
 Patch #1 needs to be respun with the change I requested.
 
Ok, done.

 And this is not the way to submit things that have dependencies
 upon bug fixes.
 
Will do. I was actually unsure about how to handle this case.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Thu, Oct 22, 2015 at 03:53:33AM +0300, Denys Fedoryshchenko wrote:
> On 2015-10-22 03:14, Matt Bennett wrote:
> >On Tue, 2015-10-13 at 05:13 +0300, Denys Fedoryshchenko wrote:
> >>On 2015-10-07 15:12, Guillaume Nault wrote:
> >>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> >>> index 2ed7506..5e0b432 100644
> >>> --- a/drivers/net/ppp/pppoe.c
> >>> +++ b/drivers/net/ppp/pppoe.c
> >>> @@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
> >>>
> >>>   po = pppox_sk(sk);
> >>>
> >>> - if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> >>> + if (po->pppoe_dev) {
> >>>   dev_put(po->pppoe_dev);
> >>>   po->pppoe_dev = NULL;
> >>>   }
> >>I just got OK to upgrade server yesterday, for now around 12 hours
> >>working fine. I need 1-2 more days, and maybe will upgrade few more
> >>servers to say for sure, if it is ok or not.
> >>Sorry for delay, just it is production servers and at current situation
> >>they cannot tolerate significant downtime.
> >>
> >Any update on whether this issue is fixed with the suggested patch?
> 
> As on server i am allowed to test - no crashed anymore, but i am unable to
> get permission yet to test
> on server where this crash was happening several times per day. But all i
> can say it is definitely better now.
> 
Good. It seems that more people are getting this problem, so I'm going
to submit the patch now.

I'm still interested in the result of your test on the second server
though.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] ppp: fix pppoe_dev deletion condition in pppoe_release()

[Guillaume Nault]

We can't rely on PPPOX_ZOMBIE to decide whether to clear po->pppoe_dev.
PPPOX_ZOMBIE can be set by pppoe_disc_rcv() even when po->pppoe_dev is
NULL. So we have no guarantee that (sk->sk_state & PPPOX_ZOMBIE) implies
(po->pppoe_dev != NULL).
Since we're releasing a PPPoE socket, we want to release the pppoe_dev
if it exists and reset sk_state to PPPOX_DEAD, no matter the previous
value of sk_state. So we can just check for po->pppoe_dev and avoid any
assumption on sk->sk_state.

Fixes: 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 2ed7506..5e0b432 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
 
po = pppox_sk(sk);
 
-   if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
+   if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fw: [Bug 107141] New: [1177853.192071] INFO: task accel-pppd:24263 blocked for more than 120 seconds.

[Guillaume Nault]

On Wed, Nov 11, 2015 at 08:57:15AM -0800, Stephen Hemminger wrote:
> 
> 
> Begin forwarded message:
> 
> Date: Wed, 4 Nov 2015 10:46:56 +
> From: "bugzilla-dae...@bugzilla.kernel.org" 
> 
> To: "shemmin...@linux-foundation.org" 
> Subject: [Bug 107141] New: [1177853.192071] INFO: task accel-pppd:24263 
> blocked for more than 120 seconds.
> 
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=107141
> 
> Bug ID: 107141
>Summary: [1177853.192071] INFO: task accel-pppd:24263 blocked
> for more than 120 seconds.
>Product: Networking
>Version: 2.5
> Kernel Version: 4.1.12 , 4.2.0 , 4.3
>   Hardware: All
> OS: Linux
>   Tree: Mainline
> Status: NEW
>   Severity: high
>   Priority: P1
>  Component: Other
>   Assignee: shemmin...@linux-foundation.org
>   Reporter: pstaszew...@artcom.pl
> Regression: No
> 
> Latest checked kernel where no problem exist:
> 3.13.0
> 
> 
> 
> On any kernel >4.0 I have always same problem as below:
> 
[...]

> [1177853.192196] INFO: task accel-pppd:10821 blocked for more than 120 
> seconds.
> [1177853.192196]   Not tainted 4.2.0 #1
> [1177853.192197] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> [1177853.192198] accel-pppd  D 88021fd14680 0 10821  1
> 0x
> [1177853.192199]  8800d771fd08 0082 8800d771fcd8
> 8802161c
> [1177853.192201]  8800d772 8801fe650bc0 8800d771fce8
> 8800d772
> [1177853.192202]  8801fe650bc0 81a9d264 81a9d268
> 
> [1177853.192203] Call Trace:
> [1177853.192205]  [] schedule+0x71/0x80
> [1177853.192207]  [] schedule_preempt_disabled+0x9/0xb
> [1177853.192208]  [] __mutex_lock_slowpath+0xa6/0x104
> [1177853.192210]  [] mutex_lock+0x13/0x24
> [1177853.192212]  [] rtnl_lock+0x10/0x12
> [1177853.192214]  [] register_netdev+0x11/0x27
> [1177853.192215]  [] ppp_ioctl+0x2ee/0xb9d
> [1177853.192218]  [] do_vfs_ioctl+0x418/0x460
> [1177853.192219]  [] ? _raw_spin_lock+0x9/0xb
> [1177853.192221]  [] ? __fget+0x2a/0x69
> [1177853.19]  [] SyS_ioctl+0x4e/0x7e
> [1177853.192224]  [] entry_SYSCALL_64_fastpath+0x12/0x6a

[...]

> [1177853.192306] INFO: task accel-pppd:6838 blocked for more than 120 seconds.
> [1177853.192307]   Not tainted 4.2.0 #1
> [1177853.192308] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> [1177853.192308] accel-pppd  D 88021fc14680 0  6838  1
> 0x
> [1177853.192310]  8800c5ca7d08 0082 000380ef
> 81a114c0
> [1177853.192311]  0f48ff3a 8800d76c3ac0 
> 8800c5ca8000
> [1177853.192313]  8800d76c3ac0 880215e3cdac 880215e3cdb0
> 
> [1177853.192314] Call Trace:
> [1177853.192316]  [] schedule+0x71/0x80
> [1177853.192317]  [] schedule_preempt_disabled+0x9/0xb
> [1177853.192319]  [] __mutex_lock_slowpath+0xa6/0x104
> [1177853.192320]  [] mutex_lock+0x13/0x24
> [1177853.192322]  [] ppp_dev_uninit+0x62/0xae
> [1177853.192324]  [] rollback_registered_many+0x19e/0x252
> [1177853.192325]  [] rollback_registered+0x29/0x38
> [1177853.192327]  [] unregister_netdevice_queue+0x6a/0x77
> [1177853.192328]  [] ppp_release+0x3f/0x73
> [1177853.192330]  [] __fput+0xdf/0x184
> [1177853.192332]  [] fput+0x9/0xb
> [1177853.192335]  [] task_work_run+0x7b/0x94
> [1177853.192336]  [] do_notify_resume+0x40/0x44
> [1177853.192338]  [] int_signal+0x12/0x17

This should be fixed by 58a89ecaca53 ("ppp: fix lockdep splat in
ppp_dev_uninit()").

Only Linux 4.2 is impacted (not counting -rcX versions). The fix has
been integrated into the -stable tree with Linux 4.2.3.

If lockups occur on Linux 4.1.12 or Linux 4.3, they most likely have
another origin.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Tue, Oct 06, 2015 at 09:12:18PM +, Matt Bennett wrote:
> On Tue, 2015-10-06 at 11:46 +0200, Guillaume Nault wrote:
> > On Tue, Oct 06, 2015 at 04:46:04AM +, Matt Bennett wrote:
> > > I don't know why the code isn't like the following anyway.
> > > 
> > > -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> > > +if (po->pppoe_dev) {
> > >   dev_put(po->pppoe_dev);
> > >   po->pppoe_dev = NULL;
> > > }
> > I was thinking about that same approach. pppoe_release() is the only
> > function making that assumption. Other parts of the code seem to only
> > require that PPPOX_CONNECTED => pppoe_dev != NULL.
> > 
> > But I think the original condition was valid. Adding PPPOX_ZOMBIE into
> > the test and resetting pppoe_dev upon reception of PADT have changed the
> > relationship between sk_state and pppoe_dev, which is where the problem
> > stands.
> Yes originally the condition was valid. But I think the issue is plain
> to see when you look at the comment beside PPPOX_ZOMBIE declared in the
> enum.
> 
>  PPPOX_ZOMBIE = 8,  /* dead, but still bound to ppp device */
> 
> We have seen in the situation we have described previously that we can
> be in this state without being bound to the ppp device.
> 
> In my opinion the entire logic around
> pppoe_disc_rcv()/pppoe_unbind_sock_work() looks wrong and I agree we
> should do what you suggested a few emails back.
> 
> i.e in pppoe_disc_rcv():
> 
> if (po) {
>   struct sock *sk = sk_pppox(po);
> 
> - bh_lock_sock(sk);
> -
> - /* If the user has locked the socket, just ignore
> -  * the packet.  With the way two rcv protocols hook into
> -  * one socket family type, we cannot (easily) distinguish
> -  * what kind of SKB it is during backlog rcv.
> -  */
> - if (sock_owned_by_user(sk) == 0) {
> - /* We're no longer connect at the PPPOE layer,
> -  * and must wait for ppp channel to disconnect us.
> -  */
> - sk->sk_state = PPPOX_ZOMBIE;
> - }
> -
> - bh_unlock_sock(sk);
>   if (!schedule_work(>proto.pppoe.padt_work))
>   sock_put(sk);
>   }
> 
Yes, with the introduction of pppoe_unbind_sock_work(), setting
PPPOX_ZOMBIE shouldn't be required anymore.

> Subsequently the PPPOX_ZOMBIE state can be completely removed?
> 
Yes, this is the last place where PPPOX_ZOMBIE can be set.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
>   if (po) {
>   struct sock *sk = sk_pppox(po);
>  
> - bh_lock_sock(sk);
> -
> - /* If the user has locked the socket, just ignore
> -  * the packet.  With the way two rcv protocols hook into
> -  * one socket family type, we cannot (easily) distinguish
> -  * what kind of SKB it is during backlog rcv.
> -  */
> - if (sock_owned_by_user(sk) == 0) {
> - /* We're no longer connect at the PPPOE layer,
> -  * and must wait for ppp channel to disconnect us.
> -  */
> - sk->sk_state = PPPOX_ZOMBIE;
> - }
> -
> - bh_unlock_sock(sk);
>   if (!schedule_work(>proto.pppoe.padt_work))
>   sock_put(sk);
>   }
> 
Finally, I think I'll keep this approach for net-next, to completely
remove PPPOX_ZOMBIE.
For now, let's just avoid any assumption about the relationship between
the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
Matt.

Denys, can you let me know if your issue goes away with the following
patch?
---
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 2ed7506..5e0b432 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
 
po = pppox_sk(sk);
 
-   if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
+   if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Tue, Oct 13, 2015 at 05:13:54AM +0300, Denys Fedoryshchenko wrote:
> On 2015-10-07 15:12, Guillaume Nault wrote:
> >On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
> >>if (po) {
> >>struct sock *sk = sk_pppox(po);
> >>
> >>-   bh_lock_sock(sk);
> >>-
> >>-   /* If the user has locked the socket, just ignore
> >>-* the packet.  With the way two rcv protocols hook into
> >>-* one socket family type, we cannot (easily) distinguish
> >>-* what kind of SKB it is during backlog rcv.
> >>-*/
> >>-   if (sock_owned_by_user(sk) == 0) {
> >>-   /* We're no longer connect at the PPPOE layer,
> >>-* and must wait for ppp channel to disconnect us.
> >>-*/
> >>-   sk->sk_state = PPPOX_ZOMBIE;
> >>-   }
> >>-
> >>-   bh_unlock_sock(sk);
> >>if (!schedule_work(>proto.pppoe.padt_work))
> >>sock_put(sk);
> >>}
> >>
> >Finally, I think I'll keep this approach for net-next, to completely
> >remove PPPOX_ZOMBIE.
> >For now, let's just avoid any assumption about the relationship between
> >the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
> >Matt.
> >
> >Denys, can you let me know if your issue goes away with the following
> >patch?
> >---
> >diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> >index 2ed7506..5e0b432 100644
> >--- a/drivers/net/ppp/pppoe.c
> >+++ b/drivers/net/ppp/pppoe.c
> >@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
> >
> > po = pppox_sk(sk);
> >
> >-if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> >+if (po->pppoe_dev) {
> > dev_put(po->pppoe_dev);
> > po->pppoe_dev = NULL;
> > }
> I just got OK to upgrade server yesterday, for now around 12 hours working
> fine. I need 1-2 more days, and maybe will upgrade few more servers to say
> for sure, if it is ok or not.
> Sorry for delay, just it is production servers and at current situation they
> cannot tolerate significant downtime.
> 
That's ok. I'll send an official patch when you consider the issue to
be definitely fixed.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Tue, Oct 06, 2015 at 12:26:20AM +, Matt Bennett wrote:
> On Mon, 2015-10-05 at 14:24 +0200, Guillaume Nault wrote:
> > On Mon, Oct 05, 2015 at 04:08:51AM +, Matt Bennett wrote:
> > > Hi, I am seeing this panic occur occasionally however I am unsure how to
> > > go about reproducing it. Is it enough to simply keep creating and
> > > tearing down the PPP interface? I can also test and/or investigate this
> > > issue if a suitable reproduction method is available.
> > > 
> > There are at least two issues resulting in similar Oops.
> > 
> > The first one goes with MTU/address/link state updates on the
> > underlying interface: any such update on an interface used by a
> > PPPoE connection will generally result in an Oops when releasing the
> > PPPoE connection. This is fixed by e6740165b8f7 ("ppp: don't override
> > sk->sk_state in pppoe_flush_dev()").
> 
> Without your patch ("ppp: don't override sk->sk_state in
> pppoe_flush_dev()") I can see the following function calls being made
> when changing the mtu on the underlying ethernet interface for the PPPoE
> connection:
> 
> 1. pppoe_flush_dev() - setting PPPOX_ZOMBIE
> 
> 2. pppoe_connect - setting PPPOX_NONE (shown below)
> 
> /* Delete the old binding */
>   if (stage_session(po->pppoe_pa.sid)) {
>   pppox_unbind_sock(sk);
>   pn = pppoe_pernet(sock_net(sk));
>   delete_item(pn, po->pppoe_pa.sid,
>   po->pppoe_pa.remote, po->pppoe_ifindex);
>   if (po->pppoe_dev) {
>   dev_put(po->pppoe_dev);
>   po->pppoe_dev = NULL;
>   }
> 
>   memset(sk_pppox(po) + 1, 0,
>  sizeof(struct pppox_sock) - sizeof(struct sock));
>   sk->sk_state = PPPOX_NONE;
>   }
> 
> 3. pppoe_release - No oops (since sk->sk_state is no longer in
> {PPPOX_CONNECTED,PPPOX_BOUND,PPPOX_ZOMBIE})
> 
> It doesn't look to me like the above functions can execute
> asynchronously but I'd have to look harder. I am using 3.16 by the way.
> 
Just drop the pppoe_connect() call. Right after the pppoe_flush_dev()
call, sk_state is PPPOX_ZOMBIE and pppoe_dev is NULL. This is enouhg to
make pppoe_release() crash.

The typical scenario e6740165b8f7 ("ppp: don't override sk->sk_state in
pppoe_flush_dev()") fixes is:

  Userspace process #1:   Userspace process #2:
  -   -
fd = socket(AF_PPPOX, PX_PROTO_OE, 0);
connect(fd, {AF_PPPOX, PX_PROTO_EO,
$sid, $mac_addr, $ifname},
sizeof(struct sockaddr_pppox));

... process_packets() ...   # ip link set $ifname mtu $mtu

close(fd); --> Kernel Oops
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Tue, Oct 06, 2015 at 04:46:04AM +, Matt Bennett wrote:
> > > The second one seems to be trickier. It looks like a race wrt. PADT
> > > message reception. Reproducing the bug will probably require to
> > > generate some PADT flooding to a host that creates and releases PPPoE
> > > connections.
> 
> Ok I think I can see the potential race here, specifically the PADT
> frame is received while the pppoe interface is being deleted. (I will
> have a go inducing this with msleep() in the code tomorrow)
> 
> 1. pppoe_flush_dev() - sk->sk_state = PPPOX_DEAD, po->pppoe_dev = NULL
> 
> 2. pppoe_connect() - sk->sk_state = PPPOX_NONE, po->pppoe_dev = NULL
> 
> 3. pppoe_disc_rcv() - sk->sk_state = PPPOX_ZOMBIE po->pppoe_dev = NULL
> 
> 4. pppoe_release() - dev_put(po->pppoe_dev) > Oops
> 
Again, I don't know why you introduce pppoe_connect() into the mix.
But anyway, you got the point. Note that pppoe_flush_dev() could be
replaced by other calls since we just need to reset po->pppoe_dev
(another pppoe_unbind_sock_work() call, due to duplicated PADT, would
also trigger the bug). Note also that pppoe_release() needs to be run
before pppoe_unbind_sock_work() gets scheduled (or at least before it
locks the socket).

> Either in pppoe_disc_rcv() we add the condition:
> 
> @@ -496,7 +499,8 @@ static int pppoe_disc_rcv(struct sk_buff *skb,
> struct net_device *dev,
> /* We're no longer connect at the PPPOE layer,
>  * and must wait for ppp channel to disconnect
> us.
>  */
> -   sk->sk_state = PPPOX_ZOMBIE;
> +   if (sk->sk_state & PPPOX_CONNECTED)
> +   sk->sk_state = PPPOX_ZOMBIE;
> }
> 
> Or perhaps we remove the assumption that the state PPPOX_ZOMBIE has a
> non-null pppoe_dev on it.
> 
I don't think adding complexity in the socket state management would be
a good think. Actually I event think about dropping the PPPOX_ZOMBIE
state altogether. But that's probably something for net-next.

> I don't know why the code isn't like the following anyway.
> 
> -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> +if (po->pppoe_dev) {
>   dev_put(po->pppoe_dev);
>   po->pppoe_dev = NULL;
> }
I was thinking about that same approach. pppoe_release() is the only
function making that assumption. Other parts of the code seem to only
require that PPPOX_CONNECTED => pppoe_dev != NULL.

But I think the original condition was valid. Adding PPPOX_ZOMBIE into
the test and resetting pppoe_dev upon reception of PADT have changed the
relationship between sk_state and pppoe_dev, which is where the problem
stands.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 4.1.0, kernel panic, pppoe_release

[Guillaume Nault]

On Fri, Jul 17, 2015 at 09:16:14PM +0300, Denys Fedoryshchenko wrote:
> Probably my knowledge of kernel is not sufficient, but i will try few
> approaches.
> One of them to add to pppoe_unbind_sock_work:
> 
> pppox_unbind_sock(sk);
> +/* Signal the death of the socket. */
> +sk->sk_state = PPPOX_DEAD;
>
I don't believe this will fix anything. pppox_unbind_sock() already
sets sk->sk_state when necessary.

> I will wait first, to make sure this patch was causing kernel panic (it
> needs 24h testing cycle), then i will try this fix.
> 
I suspect the problem goes with actions performed on the underlying
interface (MAC address, MTU or link state update). This triggers
pppoe_flush_dev(), which cleans up the device without announcing it
in sk->sk_state.

Can you pleas try the following patch?

---
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 3837ae3..2ed7506 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -313,7 +313,6 @@ static void pppoe_flush_dev(struct net_device *dev)
if (po->pppoe_dev == dev &&
sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | 
PPPOX_ZOMBIE)) {
pppox_unbind_sock(sk);
-   sk->sk_state = PPPOX_ZOMBIE;
sk->sk_state_change(sk);
po->pppoe_dev = NULL;
dev_put(dev);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel panic in pppoe_release

[Guillaume Nault]

On Thu, Sep 03, 2015 at 12:14:32PM -0700, Murat Sezgin wrote:
> I just wonder , if the
> proposed patch by Denys Fedoryshchenko, which is below, fixes this issue
> completely.
> 
>     pppox_unbind_sock(sk);
>     +/* Signal the death of the socket. */
>     +sk->sk_state = PPPOX_DEAD;
> 
> Do you have a conclusion on this bug? Is it safe to get this patch along
> with the other workqueue patches?
>
You might want to check my recent reply in the original thread:
https://marc.info/?l=linux-netdev=144190062507458=2
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

Since commit 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release"),
pppoe_release() calls dev_put(po->pppoe_dev) if sk is in the
PPPOX_ZOMBIE state. But pppoe_flush_dev() can set sk->sk_state to
PPPOX_ZOMBIE _and_ reset po->pppoe_dev to NULL. This leads to the
following oops:

[  570.140800] BUG: unable to handle kernel NULL pointer dereference at 
04e0
[  570.142931] IP: [] pppoe_release+0x50/0x101 [pppoe]
[  570.144601] PGD 3d119067 PUD 3dbc1067 PMD 0
[  570.144601] Oops:  [#1] SMP
[  570.144601] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core 
ip6_udp_tunnel udp_tunnel pppoe pppox ppp_generic slhc loop crc32c_intel 
ghash_clmulni_intel jitterentropy_rng sha256_generic hmac drbg ansi_cprng 
aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper acpi_cpufreq 
evdev serio_raw processor button ext4 crc16 mbcache jbd2 virtio_net virtio_blk 
virtio_pci virtio_ring virtio
[  570.144601] CPU: 1 PID: 15738 Comm: ppp-apitest Not tainted 4.2.0 #1
[  570.144601] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Debian-1.8.2-1 04/01/2014
[  570.144601] task: 88003d30d600 ti: 880036b6 task.ti: 
880036b6
[  570.144601] RIP: 0010:[]  [] 
pppoe_release+0x50/0x101 [pppoe]
[  570.144601] RSP: 0018:880036b63e08  EFLAGS: 00010202
[  570.144601] RAX:  RBX: 88003434 RCX: 0206
[  570.144601] RDX: 0006 RSI: 88003d30dd20 RDI: 88003d30dd20
[  570.144601] RBP: 880036b63e28 R08: 0001 R09: 
[  570.144601] R10: 7ffee9b50420 R11: 880034340078 R12: 8800387ec780
[  570.144601] R13: 8800387ec7b0 R14: 88003e222aa0 R15: 8800387ec7b0
[  570.144601] FS:  7f5672f48700() GS:88003fc8() 
knlGS:
[  570.144601] CS:  0010 DS:  ES:  CR0: 80050033
[  570.144601] CR2: 04e0 CR3: 37f7e000 CR4: 000406a0
[  570.144601] Stack:
[  570.144601]  a018f240 8800387ec780 a018f240 
8800387ec7b0
[  570.144601]  880036b63e48 812caabe 880039e4e000 
0008
[  570.144601]  880036b63e58 812cabad 880036b63ea8 
811347f5
[  570.144601] Call Trace:
[  570.144601]  [] sock_release+0x1a/0x75
[  570.144601]  [] sock_close+0xd/0x11
[  570.144601]  [] __fput+0xff/0x1a5
[  570.144601]  [] fput+0x9/0xb
[  570.144601]  [] task_work_run+0x66/0x90
[  570.144601]  [] prepare_exit_to_usermode+0x8c/0xa7
[  570.144601]  [] syscall_return_slowpath+0x16d/0x19b
[  570.144601]  [] int_ret_from_sys_call+0x25/0x9f
[  570.144601] Code: 48 8b 83 c8 01 00 00 a8 01 74 12 48 89 df e8 8b 27 14 e1 
b8 f7 ff ff ff e9 b7 00 00 00 8a 43 12 a8 0b 74 1c 48 8b 83 a8 04 00 00 <48> 8b 
80 e0 04 00 00 65 ff 08 48 c7 83 a8 04 00 00 00 00 00 00
[  570.144601] RIP  [] pppoe_release+0x50/0x101 [pppoe]
[  570.144601]  RSP 
[  570.144601] CR2: 04e0
[  570.200518] ---[ end trace 46956baf17349563 ]---

pppoe_flush_dev() has no reason to override sk->sk_state with
PPPOX_ZOMBIE. pppox_unbind_sock() already sets sk->sk_state to
PPPOX_DEAD, which is the correct state given that sk is unbound and
po->pppoe_dev is NULL.

Fixes: 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release")
Tested-by: Oleksii Berezhniak <c...@irc.lg.ua>
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 3837ae3..2ed7506 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -313,7 +313,6 @@ static void pppoe_flush_dev(struct net_device *dev)
if (po->pppoe_dev == dev &&
sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | 
PPPOX_ZOMBIE)) {
pppox_unbind_sock(sk);
-   sk->sk_state = PPPOX_ZOMBIE;
sk->sk_state_change(sk);
po->pppoe_dev = NULL;
dev_put(dev);
-- 
2.5.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Fri, Oct 02, 2015 at 11:01:45AM +0300, Denys Fedoryshchenko wrote:
> Here is similar panic after patch applied (it might be different bug), got
> over netconsole:
> 
>  [126348.617115] CPU: 0 PID: 5254 Comm: accel-pppd Not tainted
> 4.2.2-build-0087 #2
>  [126348.617632] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS
> SE5C600.86B.02.03.0003.041920141333 04/19/2014
>  [126348.618193] task: 8817cfbe ti: 8817c635 task.ti:
> 8817c635
>  [126348.618696] RIP: 0010:[]
>  [] pppoe_release+0x56/0x142 [pppoe]
>  [126348.619306] RSP: 0018:8817c6353e28  EFLAGS: 00010202
>  [126348.619601] RAX:  RBX: 8817a92b0400 RCX:
> 
>  [126348.620152] RDX: 0001 RSI: fe01 RDI:
> 8180c18a
>  [126348.620715] RBP: 8817c6353e68 R08:  R09:
> 
>  [126348.621254] R10: 88173c02b210 R11: 0293 R12:
> 8817b3c18000
>  [126348.621784] R13: 8817b3c18030 R14: 8817967f1140 R15:
> 8817d226c920
>  [126348.622330] FS:  7f9444db9700() GS:8817dee0()
> knlGS:
>  [126348.622876] CS:  0010 DS:  ES:  CR0: 80050033
>  [126348.623202] CR2: 0428 CR3: 0017c70b2000 CR4:
> 001406f0
>  [126348.623760] Stack:
>  [126348.624056]  000100200018
>  
>  0001
>  8817b3c18000
> 
>  [126348.624925]  a00ec280
>  8817b3c18030
>  8817967f1140
>  8817d226c920
> 
>  [126348.625736]  8817c6353e88
>  8180820a
>  88173c02b200
>  0008
> 
>  [126348.626533] Call Trace:
>  [126348.626873]  [] sock_release+0x1a/0x70
>  [126348.627183]  [] sock_close+0xd/0x11
>  [126348.627512]  [] __fput+0xdf/0x193
>  [126348.627845]  [] fput+0x9/0xb
>  [126348.628169]  [] task_work_run+0x78/0x8f
>  [126348.628517]  [] do_notify_resume+0x40/0x4e
>  [126348.628837]  [] int_signal+0x12/0x17

Ok, so there's another possibility for pppoe_release() to be called while
sk->sk_state is PPPOX_{CONNECTED,BOUND,ZOMBIE} but po->pppoe_dev is NULL.

I'll check the code to see if I can find any race wrt. po->pppoe_dev
and sk->sk_state settings.

In a previous message, you said you'd try reverting 287f3a943fef
("pppoe: Use workqueue to die properly when a PADT is received") and
related patches. I guess "related patches" means 665a6cd809f4 ("pppoe:
drop pppoe device in pppoe_unbind_sock_work"), right?.
Did these reverts give any successful result?

BTW, please don't top-post.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux 4.2-rc8+...v4.3-rc2] REGRESSION: ppp: circular locking dependency detected: [pppd] ppp_dev_uninit() | rtnl_lock()

[Guillaume Nault]

On Wed, Sep 23, 2015 at 08:06:16AM +0200, Sedat Dilek wrote:
> Without reverting the below culprit ppp patch...
> 
> commit/?id=8cb775bc0a34dc596837e7da03fd22c747be618b
> ("ppp: fix device unregistration upon netns deletion")
> 
> ...I have an unstable Internet connection via Network-Manager/ModemManager.
> 
> First I saw this with Linux v4.2.
> 

Thanks for reporting the issue. Can you test this patch ?

---
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 0481daf..ed00446 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2755,6 +2755,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
 */
dev_net_set(dev, net);
 
+   rtnl_lock();
mutex_lock(>all_ppp_mutex);
 
if (unit < 0) {
@@ -2785,7 +2786,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
ppp->file.index = unit;
sprintf(dev->name, "ppp%d", unit);
 
-   ret = register_netdev(dev);
+   ret = register_netdevice(dev);
if (ret != 0) {
unit_put(>units_idr, unit);
netdev_err(ppp->dev, "PPP: couldn't register device %s (%d)\n",
@@ -2797,6 +2798,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
 
atomic_inc(_unit_count);
mutex_unlock(>all_ppp_mutex);
+   rtnl_unlock();
 
*retp = 0;
return ppp;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] ppp: fix lockdep splat in ppp_dev_uninit()

[Guillaume Nault]

ppp_dev_uninit() locks all_ppp_mutex while under rtnl mutex protection.
ppp_create_interface() must then lock these mutexes in that same order
to avoid possible deadlock.

[  120.880011] ==
[  120.880011] [ INFO: possible circular locking dependency detected ]
[  120.880011] 4.2.0 #1 Not tainted
[  120.880011] ---
[  120.880011] ppp-apitest/15827 is trying to acquire lock:
[  120.880011]  (>all_ppp_mutex){+.+.+.}, at: [] 
ppp_dev_uninit+0x64/0xb0 [ppp_generic]
[  120.880011]
[  120.880011] but task is already holding lock:
[  120.880011]  (rtnl_mutex){+.+.+.}, at: [] 
rtnl_lock+0x12/0x14
[  120.880011]
[  120.880011] which lock already depends on the new lock.
[  120.880011]
[  120.880011]
[  120.880011] the existing dependency chain (in reverse order) is:
[  120.880011]
[  120.880011] -> #1 (rtnl_mutex){+.+.+.}:
[  120.880011][] lock_acquire+0xcf/0x10e
[  120.880011][] mutex_lock_nested+0x56/0x341
[  120.880011][] rtnl_lock+0x12/0x14
[  120.880011][] register_netdev+0x11/0x27
[  120.880011][] ppp_ioctl+0x289/0xc98 [ppp_generic]
[  120.880011][] do_vfs_ioctl+0x4ea/0x532
[  120.880011][] SyS_ioctl+0x4e/0x7d
[  120.880011][] entry_SYSCALL_64_fastpath+0x12/0x6f
[  120.880011]
[  120.880011] -> #0 (>all_ppp_mutex){+.+.+.}:
[  120.880011][] __lock_acquire+0xb07/0xe76
[  120.880011][] lock_acquire+0xcf/0x10e
[  120.880011][] mutex_lock_nested+0x56/0x341
[  120.880011][] ppp_dev_uninit+0x64/0xb0 
[ppp_generic]
[  120.880011][] rollback_registered_many+0x19e/0x252
[  120.880011][] rollback_registered+0x29/0x38
[  120.880011][] unregister_netdevice_queue+0x6a/0x77
[  120.880011][] ppp_release+0x42/0x79 [ppp_generic]
[  120.880011][] __fput+0xec/0x192
[  120.880011][] fput+0x9/0xb
[  120.880011][] task_work_run+0x66/0x80
[  120.880011][] prepare_exit_to_usermode+0x8c/0xa7
[  120.880011][] syscall_return_slowpath+0xe4/0x104
[  120.880011][] int_ret_from_sys_call+0x25/0x9f
[  120.880011]
[  120.880011] other info that might help us debug this:
[  120.880011]
[  120.880011]  Possible unsafe locking scenario:
[  120.880011]
[  120.880011]CPU0CPU1
[  120.880011]
[  120.880011]   lock(rtnl_mutex);
[  120.880011]lock(>all_ppp_mutex);
[  120.880011]lock(rtnl_mutex);
[  120.880011]   lock(>all_ppp_mutex);
[  120.880011]
[  120.880011]  *** DEADLOCK ***

Fixes: 8cb775bc0a34 ("ppp: fix device unregistration upon netns deletion")
Reported-by: Sedat Dilek <sedat.di...@gmail.com>
Tested-by: Sedat Dilek <sedat.di...@gmail.com>
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 0481daf..ed00446 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2755,6 +2755,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
 */
dev_net_set(dev, net);
 
+   rtnl_lock();
mutex_lock(>all_ppp_mutex);
 
if (unit < 0) {
@@ -2785,7 +2786,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
ppp->file.index = unit;
sprintf(dev->name, "ppp%d", unit);
 
-   ret = register_netdev(dev);
+   ret = register_netdevice(dev);
if (ret != 0) {
unit_put(>units_idr, unit);
netdev_err(ppp->dev, "PPP: couldn't register device %s (%d)\n",
@@ -2797,6 +2798,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
 
atomic_inc(_unit_count);
mutex_unlock(>all_ppp_mutex);
+   rtnl_unlock();
 
*retp = 0;
return ppp;
-- 
2.5.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux 4.2-rc8+...v4.3-rc2] REGRESSION: ppp: circular locking dependency detected: [pppd] ppp_dev_uninit() | rtnl_lock()

[Guillaume Nault]

On Wed, Sep 23, 2015 at 11:21:50PM +0200, Sedat Dilek wrote:
> On Wed, Sep 23, 2015 at 10:46 PM, Sedat Dilek <sedat.di...@gmail.com> wrote:
> > On Wed, Sep 23, 2015 at 12:38 PM, Guillaume Nault <g.na...@alphalink.fr> 
> > wrote:
> > Do you mind to send a proper patch with subject-line and commit-message?
> > Can you embed the Fixes-tag and give credits like Reported-by/Tested-by?
>
I've just sent the patch to netdev.

> Do not forget CC-stable Tag, too.
> 
>  Cc: sta...@vger.kernel.org# v4.2+
>
Well, that's not how it works for netdev. DaveM handles stable
submissions on his own.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 4.1.0, kernel panic, pppoe_release

[Guillaume Nault]

On Tue, Sep 22, 2015 at 04:47:48AM +0300, Denys Fedoryshchenko wrote:
> Hi,
> Sorry for late reply, was not able to push new kernel on pppoes without
> permissions (it's production servers), just got OK.
> 
> I am testing patch on another pppoe server with 9k users, for ~3 days, seems
> fine. I will test today
> also on server that was experiencing crashes within 1 day.
>
Thanks for the feedback. I'm about to submit a fix. Should I add a
Tested-by tag for you?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 4.1.0, kernel panic, pppoe_release

[Guillaume Nault]

On Fri, Sep 25, 2015 at 06:02:42PM +0300, Denys Fedoryshchenko wrote:
> On 2015-09-25 17:38, Guillaume Nault wrote:
> >On Tue, Sep 22, 2015 at 04:47:48AM +0300, Denys Fedoryshchenko wrote:
> >>Hi,
> >>Sorry for late reply, was not able to push new kernel on pppoes without
> >>permissions (it's production servers), just got OK.
> >>
> >>I am testing patch on another pppoe server with 9k users, for ~3 days,
> >>seems
> >>fine. I will test today
> >>also on server that was experiencing crashes within 1 day.
> >>
> >Thanks for the feedback. I'm about to submit a fix. Should I add a
> >Tested-by tag for you?
> On one of servers i got same crash as before, within hours. 9k users server
> also crashed after while, so it seems it doesn't help.
> I will do some more tests tomorrow.
Ok, this must be a different bug then. Do you have a trace of a crash
with the patched kernel?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

On Mon, Oct 05, 2015 at 04:08:51AM +, Matt Bennett wrote:
> Hi, I am seeing this panic occur occasionally however I am unsure how to
> go about reproducing it. Is it enough to simply keep creating and
> tearing down the PPP interface? I can also test and/or investigate this
> issue if a suitable reproduction method is available.
> 
There are at least two issues resulting in similar Oops.

The first one goes with MTU/address/link state updates on the
underlying interface: any such update on an interface used by a
PPPoE connection will generally result in an Oops when releasing the
PPPoE connection. This is fixed by e6740165b8f7 ("ppp: don't override
sk->sk_state in pppoe_flush_dev()").

The second one seems to be trickier. It looks like a race wrt. PADT
message reception. Reproducing the bug will probably require to
generate some PADT flooding to a host that creates and releases PPPoE
connections.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

[Guillaume Nault]

> I am doing just "dirty" patch like this, i cannot certainly remember if i
> was doing git reversal, because
> it was a while when i spotted this bug. After that pppoe server is not
> rebooting.
> 
> diff -Naur linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c
> linux-4.2.2-changed/drivers/net/ppp/pppoe.c
> --- linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c 2015-09-29
> 20:38:27.0 +0300
> +++ linux-4.2.2-changed/drivers/net/ppp/pppoe.c 2015-10-04
> 19:05:55.697732991 +0300
> @@ -519,7 +519,7 @@
> }
> 
> bh_unlock_sock(sk);
> -   if (!schedule_work(>proto.pppoe.padt_work))
> +// if (!schedule_work(>proto.pppoe.padt_work))
> sock_put(sk);
> }
> 
> @@ -633,7 +633,7 @@
> 
> lock_sock(sk);
> 
> -   INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
> +// INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
> 
> error = -EINVAL;
> if (sp->sa_protocol != PX_PROTO_OE)
> 
Ok, so this is clearly related with PADT message handling. Setting
sk->sk_state to PPPOX_ZOMBIE in pppoe_disc_rcv() looks wrong to me.
Furthurmore, at a first glance, it doesn't look necessary. If you're
feeling lucky, you can try the following diff (WARNING: not even
compile-tested!):

if (po) {
struct sock *sk = sk_pppox(po);
 
-   bh_lock_sock(sk);
-
-   /* If the user has locked the socket, just ignore
-* the packet.  With the way two rcv protocols hook into
-* one socket family type, we cannot (easily) distinguish
-* what kind of SKB it is during backlog rcv.
-*/
-   if (sock_owned_by_user(sk) == 0) {
-   /* We're no longer connect at the PPPOE layer,
-* and must wait for ppp channel to disconnect us.
-*/
-   sk->sk_state = PPPOX_ZOMBIE;
-   }
-
-   bh_unlock_sock(sk);
if (!schedule_work(>proto.pppoe.padt_work))
sock_put(sk);
}

I'll take a closer look and do proper testing during the week.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] pppox: use standard module auto-loading feature

[Guillaume Nault]

  * Register PF_PPPOX with pppox module rather than with pppoe,
so that pppoe doesn't get loaded for any PF_PPPOX socket.

  * Register PX_PROTO_* with standard MODULE_ALIAS_NET_PF_PROTO()
instead of using pppox's own naming scheme.

  * While there, add auto-loading feature for pptp.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c | 2 +-
 drivers/net/ppp/pppox.c | 3 ++-
 drivers/net/ppp/pptp.c  | 1 +
 net/l2tp/l2tp_ppp.c | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 277e682..b8da2ea 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -1202,4 +1202,4 @@ module_exit(pppoe_exit);
 MODULE_AUTHOR("Michal Ostrowski <mostr...@speakeasy.net>");
 MODULE_DESCRIPTION("PPP over Ethernet driver");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS_NETPROTO(PF_PPPOX);
+MODULE_ALIAS_NET_PF_PROTO(PF_PPPOX, PX_PROTO_OE);
diff --git a/drivers/net/ppp/pppox.c b/drivers/net/ppp/pppox.c
index 0200de7..b9c8be6 100644
--- a/drivers/net/ppp/pppox.c
+++ b/drivers/net/ppp/pppox.c
@@ -113,7 +113,7 @@ static int pppox_create(struct net *net, struct socket 
*sock, int protocol,
 
rc = -EPROTONOSUPPORT;
if (!pppox_protos[protocol])
-   request_module("pppox-proto-%d", protocol);
+   request_module("net-pf-%d-proto-%d", PF_PPPOX, protocol);
if (!pppox_protos[protocol] ||
!try_module_get(pppox_protos[protocol]->owner))
goto out;
@@ -147,3 +147,4 @@ module_exit(pppox_exit);
 MODULE_AUTHOR("Michal Ostrowski <mostr...@speakeasy.net>");
 MODULE_DESCRIPTION("PPP over Ethernet driver (generic socket layer)");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS_NETPROTO(PF_PPPOX);
diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
index fc69e41..e18e0980b 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -718,3 +718,4 @@ module_exit(pptp_exit_module);
 MODULE_DESCRIPTION("Point-to-Point Tunneling Protocol");
 MODULE_AUTHOR("D. Kozlov (x...@mail.ru)");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS_NET_PF_PROTO(PF_PPPOX, PX_PROTO_PPTP);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 1ad18c5..d93f113 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1862,5 +1862,5 @@ MODULE_AUTHOR("James Chapman <jchap...@katalix.com>");
 MODULE_DESCRIPTION("PPP over L2TP over UDP");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(PPPOL2TP_DRV_VERSION);
-MODULE_ALIAS("pppox-proto-" __stringify(PX_PROTO_OL2TP));
+MODULE_ALIAS_NET_PF_PROTO(PF_PPPOX, PX_PROTO_OL2TP);
 MODULE_ALIAS_L2TP_PWTYPE(11);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] pppox: use standard module auto-loading feature

[Guillaume Nault]

On Wed, Dec 02, 2015 at 04:27:39PM +0100, Guillaume Nault wrote:
>   * Register PF_PPPOX with pppox module rather than with pppoe,
> so that pppoe doesn't get loaded for any PF_PPPOX socket.
> 
>   * Register PX_PROTO_* with standard MODULE_ALIAS_NET_PF_PROTO()
> instead of using pppox's own naming scheme.
> 
>   * While there, add auto-loading feature for pptp.
> 
This is for net-next, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

On Mon, Nov 30, 2015 at 09:42:08PM +0100, Guillaume Nault wrote:
> On Mon, Nov 30, 2015 at 04:03:37PM +0100, Guillaume Nault wrote:
> > Yes, it most likely was introduced by 287f3a943fef ("pppoe: Use
> > workqueue to die properly when a PADT is received"). I still have to
> > figure out why.
> 
> I confirm the bug comes from this commit.
> 
> It happens if pppoe_connect() reinitialises po->proto.pppoe.padt_work
> after pppoe_disc_rcv() has added it to the system's work queue, and
> before that work got scheduled. Then when scheduling occurs, the worker
> thread tries to run a corrupted structure and crashes.
> 
> I'm going to work on a patch.

You can try the following. It's not yet a proper fix as there are still
a few things that bug me in pppoe_connect().

---
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 5e0b432..865b74d 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -568,6 +568,9 @@ static int pppoe_create(struct net *net, struct socket 
*sock, int kern)
sk->sk_family   = PF_PPPOX;
sk->sk_protocol = PX_PROTO_OE;
 
+   INIT_WORK(_sk(sk)->proto.pppoe.padt_work,
+ pppoe_unbind_sock_work);
+
return 0;
 }
 
@@ -632,8 +635,6 @@ static int pppoe_connect(struct socket *sock, struct 
sockaddr *uservaddr,
 
lock_sock(sk);
 
-   INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
-
error = -EINVAL;
if (sp->sa_protocol != PX_PROTO_OE)
goto end;
@@ -663,8 +664,6 @@ static int pppoe_connect(struct socket *sock, struct 
sockaddr *uservaddr,
po->pppoe_dev = NULL;
}
 
-   memset(sk_pppox(po) + 1, 0,
-  sizeof(struct pppox_sock) - sizeof(struct sock));
sk->sk_state = PPPOX_NONE;
}
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

On Wed, Dec 02, 2015 at 06:23:35PM +0100, Guillaume Nault wrote:
> 
> You can try the following. It's not yet a proper fix as there are still
> a few things that bug me in pppoe_connect().
> 
> ---
> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 5e0b432..865b74d 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -568,6 +568,9 @@ static int pppoe_create(struct net *net, struct socket 
> *sock, int kern)
>   sk->sk_family   = PF_PPPOX;
>   sk->sk_protocol = PX_PROTO_OE;
>  
> + INIT_WORK(_sk(sk)->proto.pppoe.padt_work,
> +   pppoe_unbind_sock_work);
> +
>   return 0;
>  }
>  
> @@ -632,8 +635,6 @@ static int pppoe_connect(struct socket *sock, struct 
> sockaddr *uservaddr,
>  
>   lock_sock(sk);
>  
> - INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
> -
>   error = -EINVAL;
>   if (sp->sa_protocol != PX_PROTO_OE)
>   goto end;
> @@ -663,8 +664,6 @@ static int pppoe_connect(struct socket *sock, struct 
> sockaddr *uservaddr,
>   po->pppoe_dev = NULL;
>   }
>  
> - memset(sk_pppox(po) + 1, 0,
> -sizeof(struct pppox_sock) - sizeof(struct sock));
>   sk->sk_state = PPPOX_NONE;
>   }
>  
Finally, I'm going to send something similar to -net and keep the rest
of pppoe_connect() modifications for net-next. This will ease
backporting to -stable.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] pppoe: fix memory corruption in padt work structure

[Guillaume Nault]

pppoe_connect() mustn't touch the padt_work field of pppoe sockets
because that work could be already pending.

[   21.473147] BUG: unable to handle kernel NULL pointer dereference at 0004
[   21.474523] IP: [] process_one_work+0x29/0x31c
[   21.475164] *pde = 
[   21.475513] Oops:  [#1] SMP
[   21.475910] Modules linked in: pppoe pppox ppp_generic slhc crc32c_intel 
aesni_intel virtio_net xts aes_i586 lrw gf128mul ablk_helper cryptd evdev 
acpi_cpufreq processor serio_raw button ext4 crc16 mbcache jbd2 virtio_blk 
virtio_pci virtio_ring virtio
[   21.476168] CPU: 2 PID: 164 Comm: kworker/2:2 Not tainted 4.4.0-rc1 #1
[   21.476168] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Debian-1.8.2-1 04/01/2014
[   21.476168] task: f5f83c00 ti: f5e28000 task.ti: f5e28000
[   21.476168] EIP: 0060:[] EFLAGS: 00010046 CPU: 2
[   21.476168] EIP is at process_one_work+0x29/0x31c
[   21.484082] EAX:  EBX: f678b2a0 ECX: 0004 EDX: 
[   21.484082] ESI: f6c69940 EDI: f5e29ef0 EBP: f5e29f0c ESP: f5e29edc
[   21.484082]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[   21.484082] CR0: 80050033 CR2: 00a4 CR3: 317ad000 CR4: 00040690
[   21.484082] Stack:
[   21.484082]   f6c69950  f6c69940 c0042338 f5e29f0c c1327945 

[   21.484082]  0008 f678b2a0 f6c69940 f678b2b8 f5e29f30 c1043984 f5f83c00 
f6c69970
[   21.484082]  f678b2a0 c10437d3 f6775e80 f678b2a0 c10437d3 f5e29fac c1047059 
f5e29f74
[   21.484082] Call Trace:
[   21.484082]  [] ? _raw_spin_lock_irq+0x28/0x30
[   21.484082]  [] worker_thread+0x1b1/0x244
[   21.484082]  [] ? rescuer_thread+0x229/0x229
[   21.484082]  [] ? rescuer_thread+0x229/0x229
[   21.484082]  [] kthread+0x8f/0x94
[   21.484082]  [] ? _raw_spin_unlock_irq+0x22/0x26
[   21.484082]  [] ret_from_kernel_thread+0x21/0x38
[   21.484082]  [] ? kthread_parkme+0x19/0x19
[   21.496082] Code: 5d c3 55 89 e5 57 56 53 89 c3 83 ec 24 89 d0 89 55 e0 8d 
7d e4 e8 6c d8 ff ff b9 04 00 00 00 89 45 d8 8b 43 24 89 45 dc 8b 45 d8 <8b> 40 
04 8b 80 e0 00 00 00 c1 e8 05 24 01 88 45 d7 8b 45 e0 8d
[   21.496082] EIP: [] process_one_work+0x29/0x31c SS:ESP 
0068:f5e29edc
[   21.496082] CR2: 0004
[   21.496082] ---[ end trace e362cc9cf10dae89 ]---

Reported-by: Andrew <ni...@seti.kr.ua>
Fixes: 287f3a943fef ("pppoe: Use workqueue to die properly when a PADT is 
received")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 5e0b432..0a37f84 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -568,6 +568,9 @@ static int pppoe_create(struct net *net, struct socket 
*sock, int kern)
sk->sk_family   = PF_PPPOX;
sk->sk_protocol = PX_PROTO_OE;
 
+   INIT_WORK(_sk(sk)->proto.pppoe.padt_work,
+ pppoe_unbind_sock_work);
+
return 0;
 }
 
@@ -632,8 +635,6 @@ static int pppoe_connect(struct socket *sock, struct 
sockaddr *uservaddr,
 
lock_sock(sk);
 
-   INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
-
error = -EINVAL;
if (sp->sa_protocol != PX_PROTO_OE)
goto end;
@@ -663,8 +664,13 @@ static int pppoe_connect(struct socket *sock, struct 
sockaddr *uservaddr,
po->pppoe_dev = NULL;
}
 
-   memset(sk_pppox(po) + 1, 0,
-  sizeof(struct pppox_sock) - sizeof(struct sock));
+   po->pppoe_ifindex = 0;
+   memset(>pppoe_pa, 0, sizeof(po->pppoe_pa));
+   memset(>pppoe_relay, 0, sizeof(po->pppoe_relay));
+   memset(>chan, 0, sizeof(po->chan));
+   po->next = NULL;
+   po->num = 0;
+
sk->sk_state = PPPOX_NONE;
}
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next] pppoe: optional deactivation of PADT packet handling

[Guillaume Nault]

Kernel space shouldn't handle PADT packets since PADT belongs to
PPPoE's control plane.
With "handle_padt" module option, user space can now decide to avoid
kernel interpretation of PADT packets and be responsible for session
disconnection.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
The two open-source PPPoE programs I know of are accel-ppp and pppd
(with the rp-pppoe plugin). Accel-ppp already handles PADT, while
pppd + rp-pppoe doesn't (seems like it doesn't even listen to its
PPPoE Discovery socket once session is established).


 drivers/net/ppp/pppoe.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index b8da2ea..1846c3a 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -109,6 +109,18 @@ struct pppoe_net {
rwlock_t hash_lock;
 };
 
+/* PADT packets belong to PPPoE's discovery stage (i.e. control plane)
+ * and should be handled by user space. Unfortunately, some programs
+ * don't listen out for PADT packets and rely on pppoe module for
+ * closing session after reception of PADT.
+ *
+ * This option can be deactivated when PPPoE discovery is handled by
+ * a PADT aware program.
+ */
+static bool handle_padt __read_mostly = true;
+module_param(handle_padt, bool, 0444);
+MODULE_PARM_DESC(handle_padt, "Let kernel interpret incoming PADT");
+
 /*
  * PPPoE could be in the following stages:
  * 1) Discovery stage (to obtain remote MAC and Session ID)
@@ -1173,7 +1185,8 @@ static int __init pppoe_init(void)
goto out_unregister_pppoe_proto;
 
dev_add_pack(_ptype);
-   dev_add_pack(_ptype);
+   if (handle_padt)
+   dev_add_pack(_ptype);
register_netdevice_notifier(_notifier);
 
return 0;
@@ -1189,7 +1202,8 @@ out:
 static void __exit pppoe_exit(void)
 {
unregister_netdevice_notifier(_notifier);
-   dev_remove_pack(_ptype);
+   if (handle_padt)
+   dev_remove_pack(_ptype);
dev_remove_pack(_ptype);
unregister_pppox_proto(PX_PROTO_OE);
proto_unregister(_sk_proto);
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Another pppoe-related crash

[Guillaume Nault]

On Fri, Dec 11, 2015 at 05:07:54PM +0200, Andrew wrote:
> I've got another pppoe-related crash on one PPPoE BRAS.
> 
> Kernel is 4.1.13 with patch "pppoe: fix memory corruption in padt work
> structure"
> 
Commit 1acea4f6ce1b ("ppp: fix pppoe_dev deletion condition in
pppoe_release()") is missing from 4.1.13. Can you try with 4.1.14 (or at
least manually apply this patch)?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] pppoe: optional deactivation of PADT packet handling

[Guillaume Nault]

On Tue, Dec 15, 2015 at 10:50:05AM -0600, Dan Williams wrote:
> On Tue, 2015-12-15 at 17:06 +0100, Guillaume Nault wrote:
> > Kernel space shouldn't handle PADT packets since PADT belongs to
> > PPPoE's control plane.
> > With "handle_padt" module option, user space can now decide to avoid
> > kernel interpretation of PADT packets and be responsible for session
> > disconnection.
> 
> In general, module options like this kinda suck.  How about a new PPPoE
> ioctl so the option can be toggled for a given socket/session instead? 
>  Then at least you're not locked into a specific PPPoE implementation
> for all sessions on the machine.
> 
I've considered this approach too, but by using per session option,
we still have to register pppoed_ptype in pppoe_init() and thus call
pppoe_disc_recv() for any ETH_P_PPP_DISC frame, just in case a session
needs kernel space PADT handling. This makes the approach much less
interesting IMO, hence the original choice for module wide behaviour.
On the other hand, I understand the need for minimising module options.

Thinking a bit more about it, we could drop dev_add_pack() from
pppoe_init() and let pppoe_connect() register ETH_P_PPP_DISC when
necessary. We could even do it per device, as pppoe_connect() knows the
lower device.
The problem is to know when unregistering ETH_P_PPP_DISC. We could
probably use a reference counter, but I'd prefer to avoid complicating
pppoe's connection and disconnection code given that recent history
has shown its relative fragility.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 1/2] ppp: define "ppp" device type

[Guillaume Nault]

Let PPP devices be identified as such in /sys/class/net//uevent.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 9a863c6..1cd7651 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1138,9 +1138,15 @@ static const struct net_device_ops ppp_netdev_ops = {
.ndo_get_stats64 = ppp_get_stats64,
 };
 
+static struct device_type ppp_type = {
+   .name = "ppp",
+};
+
 static void ppp_setup(struct net_device *dev)
 {
dev->netdev_ops = _netdev_ops;
+   SET_NETDEV_DEVTYPE(dev, _type);
+
dev->hard_header_len = PPP_HDRLEN;
dev->mtu = PPP_MRU;
dev->addr_len = 0;
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 0/2] Minor PPP devices improvements

[Guillaume Nault]

Let PPP devices be friendlier to user space by registering their device
type and reporting their interface naming scheme.

Guillaume Nault (2):
  ppp: define "ppp" device type
  ppp: declare ppp devices as enumerated interfaces

 drivers/net/ppp/ppp_generic.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 2/2] ppp: declare ppp devices as enumerated interfaces

[Guillaume Nault]

Let user space be aware of the naming scheme used by ppp interfaces
(visible in /sys/class/net//name_assign_type).

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1cd7651..fc8ad00 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2726,8 +2726,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
int ret = -ENOMEM;
int i;
 
-   dev = alloc_netdev(sizeof(struct ppp), "", NET_NAME_UNKNOWN,
-  ppp_setup);
+   dev = alloc_netdev(sizeof(struct ppp), "", NET_NAME_ENUM, ppp_setup);
if (!dev)
goto out1;
 
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-v4.4-LTS] ppp: Backport of rtnetlink device handling

[Guillaume Nault]

On Mon, Jan 04, 2016 at 08:47:30AM +0100, Sedat Dilek wrote:
> Hi Guillaume,
> 
> which patches do I need to backport "ppp: rtnetlink device handling"
> to Linux v4.4 which will be a LongTerm-Supported (LTS) Linux-kernel
> [0]?
> 
Quite frankly, backporting this series doesn't look like a good idea.
It only provides a new ABI for creating ppp devices and your control
plane most likely hasn't been updated to use it. So it won't bring any
benefit.

> I tried [1] and [2] on top of recent net-next Git tree which will be
> in Linux v4.5.
> Currently, your patches are not included in net-next.git#master.
> 
Indeed, and that's why no control plane should rely on them (yet).

> In the thread "[net-next] ppp: rtnetlink device handling" [4] you
> explained the benefits and use-case etc.
> 
> Checking with git-log shows me these commits...
> 
> $ git log --oneline --no-merges v4.4-rc8.. drivers/net/ppp net/l2tp
> 19e8c5713e78 l2tp: rely on ppp layer for skb scrubbing
> 645eee4eba45 ppp: implement rtnetlink device handling
> 3a9bce0ae138 ppp: define reusable device creation functions
> 69d9728d00c7 ppp: declare ppp devices as enumerated interfaces
> 94dbffe16eb1 ppp: define "ppp" device type
> 681b4d88ad8e pppox: use standard module auto-loading feature
> a8acce6aa584 ppp: remove PPPOX_ZOMBIE socket state
> 8734e485fed5 ppp: don't set sk_state to PPPOX_ZOMBIE in pppoe_disc_rcv()
> 
> ...is that sufficient for a backport?
> 
Applying the series directly on v4.4-rc8 should work (with a few
conflicts, but other patches are unrelated). But still, you probably
don't want to maintain backports unless strictly required. BTW, this
one has no chance to hit any -stable tree anyway.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ppp: add netlink support

[Guillaume Nault]

On Tue, Jan 05, 2016 at 02:15:34PM -0500, David Miller wrote:
> From: Guillaume Nault <g.na...@alphalink.fr>
> Date: Tue, 5 Jan 2016 19:10:20 +0100
> 
> > On Wed, Dec 23, 2015 at 09:04:46PM +0100, Guillaume Nault wrote:
> >> This series adds netlink support for creating PPP devices.
> >> 
> > Any feedback on this series? I can see that it has been marked
> > "Deferred" in patchwork, so I'm unsure about what to do with this patch
> > set now.
> > Should I repost later (e.g. after the end of the merge window)? Are
> > there any concern/issues with the series itself? Or should the idea of
> > implementing netlink handlers for ppp devices be entirely dropped?
> 
> Repost it again later, I want more people to see and review this
> series and the holidays is not a good time for that...

Sure. Thanks for the feedback.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ppp: add netlink support

[Guillaume Nault]

On Wed, Dec 23, 2015 at 09:04:46PM +0100, Guillaume Nault wrote:
> This series adds netlink support for creating PPP devices.
> 
Any feedback on this series? I can see that it has been marked
"Deferred" in patchwork, so I'm unsure about what to do with this patch
set now.
Should I repost later (e.g. after the end of the merge window)? Are
there any concern/issues with the series itself? Or should the idea of
implementing netlink handlers for ppp devices be entirely dropped?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next] ppp: rtnetlink device handling

[Guillaume Nault]

On Thu, Dec 31, 2015 at 12:01:40PM +0100, Sedat Dilek wrote:
> On Thu, Dec 31, 2015 at 11:41 AM, Guillaume Nault <g.na...@alphalink.fr> 
> wrote:
> > On Thu, Dec 31, 2015 at 08:46:59AM +0100, Sedat Dilek wrote:
> >> Hi Guillaume,
> >>
> >> can you explain why you moved ppp to rtnetlink device handling?
> >> Benefits, etc.?
> >>
> > The objective is to bring all the flexibility of rtnetlink device
> > creation to ppp interfaces. This is particularly useful for Network
> > Access Servers which need to define device properties at creation
> > time. My use case includes setting device name and netns, which is
> > impossible with the ioctl-based interface without generating transient
> > effects.
> >
> 
> What do you mean by "my use-case"?
> Example?
> 
A NAS that terminates connections of PPP clients (usually transported
over PPPoE or L2TP). Clients may have different routing needs so each
PPP device is created in the appropriate network namespace. Without
rtnl the PPP device has to be created in the PPPoE/L2TP server's
namespace and then moved to the client's one. With rtnl, the PPP device
can be directly created in the right namespace.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next] ppp: rtnetlink device handling

[Guillaume Nault]

On Thu, Dec 31, 2015 at 08:46:59AM +0100, Sedat Dilek wrote:
> Hi Guillaume,
> 
> can you explain why you moved ppp to rtnetlink device handling?
> Benefits, etc.?
> 
The objective is to bring all the flexibility of rtnetlink device
creation to ppp interfaces. This is particularly useful for Network
Access Servers which need to define device properties at creation
time. My use case includes setting device name and netns, which is
impossible with the ioctl-based interface without generating transient
effects.

> Does anything change when using NetworkManager/ModemManager/pppd for
> my network setup/handling (here: Ubuntu/precise AMD64)?
> 
No. The ioctl-based ABI remains unchanged, so programs using it won't
see any difference. The rtnl ABI only brings a new way to create PPP
devices.

Whether a ppp device is created with the ioctl or rtnl ABI doesn't
change its behaviour. In particular ppp devices created with rtnl can
still be configured with the old ioctls. The only difference I can
think of, is that managing ppp devices with rtnl allows a user to
remove it with "ip link del", while this is not possible with ppp
devices created with ioctl.

> Thanks in advance.
> 
You're welcome.

Guillaume
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ppp: add netlink support

[Guillaume Nault]

On Wed, Dec 23, 2015 at 09:04:46PM +0100, Guillaume Nault wrote:
> This series adds netlink support for creating PPP devices.
> 
And I forgot to mention that it applies to net-next. Sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ppp: implement rtnetlink device handling

[Guillaume Nault]

Define PPP device handler for use with rtnetlink.

The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
contains the file descriptor of the associated /dev/ppp instance (the
file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
the ioctl-based API).

PPP devices created with the rtnetlink API behave like the ones created
with ioctl(PPPIOCNEWUNIT). In particular existing ioctls still apply to
PPP units created with the rtnetlink API.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 109 --
 include/uapi/linux/if_link.h  |   8 
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index bc24537..19476d6 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -46,6 +46,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -185,7 +186,9 @@ struct channel {
 
 struct ppp_config {
struct file *file;
+   s32 fd;
s32 unit;
+   bool ifname_is_set;
 };
 
 /*
@@ -286,6 +289,7 @@ static int unit_get(struct idr *p, void *ptr);
 static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
+static void ppp_setup(struct net_device *dev);
 
 static const struct net_device_ops ppp_netdev_ops;
 
@@ -954,7 +958,7 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
-static int ppp_unit_register(struct ppp *ppp, int unit)
+static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
 {
struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
int ret;
@@ -984,7 +988,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
 
ppp->file.index = ret;
 
-   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+   if (!ifname_is_set)
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
 
ret = register_netdevice(ppp->dev);
if (ret)
@@ -1012,7 +1017,24 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
int indx;
int err;
 
-   file = conf->file;
+   if (conf->fd >= 0) {
+   file = fget(conf->fd);
+   if (file) {
+   if (file->f_op != _device_fops) {
+   fput(file);
+   return -EBADF;
+   }
+
+   /* Don't hold reference on file: ppp_release() is
+* responsible for safely freeing the associated
+* resources upon release. So file won't go away
+* from under us.
+*/
+   fput(file);
+   }
+   } else {
+   file = conf->file;
+   }
if (!file)
return -EBADF;
if (file->private_data)
@@ -1040,7 +1062,7 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
ppp->mru = PPP_MRU;
ppp->ppp_net = src_net;
 
-   err = ppp_unit_register(ppp, conf->unit);
+   err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
if (err < 0)
return err;
 
@@ -1049,6 +1071,73 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
return 0;
 }
 
+static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
+   [IFLA_PPP_DEV_FD]   = { .type = NLA_S32 },
+};
+
+static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+   if (!data)
+   return -EINVAL;
+
+   if (!data[IFLA_PPP_DEV_FD])
+   return -EINVAL;
+   if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
+   return -EBADF;
+
+   return 0;
+}
+
+static int ppp_nl_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+   struct ppp_config conf = {
+   .file = NULL,
+   .unit = -1,
+   .ifname_is_set = true,
+   };
+
+   conf.fd = nla_get_s32(data[IFLA_PPP_DEV_FD]);
+
+   return ppp_dev_configure(src_net, dev, );
+}
+
+static void ppp_nl_dellink(struct net_device *dev, struct list_head *head)
+{
+   unregister_netdevice_queue(dev, head);
+}
+
+static size_t ppp_nl_get_size(const struct net_device *dev)
+{
+   return 0;
+}
+
+static int ppp_nl_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+   return 0;
+}
+
+static struct net *ppp_nl_get_link_net(const struct net_device *dev)
+{
+   struct ppp *ppp = netdev_priv(dev);
+
+   return ppp->ppp_net;
+}
+
+static struct rtnl_link_ops ppp_link_ops __read_mostly = {
+   .kind 

[PATCH 0/2] ppp: add netlink support

[Guillaume Nault]

This series adds netlink support for creating PPP devices.

It doesn't implement any PPP specific attribute beyond IFLA_PPP_DEV_FD,
which is necessary in the PPP device model for passing control plane
packets to user space.

Guillaume Nault (2):
  ppp: define reusable device creation functions
  ppp: implement rtnetlink device handling

 drivers/net/ppp/ppp_generic.c | 319 ++
 include/uapi/linux/if_link.h  |   8 ++
 2 files changed, 237 insertions(+), 90 deletions(-)

-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ppp: define reusable device creation functions

[Guillaume Nault]

Move PPP device initialisation and registration out of
ppp_create_interface().
This prepares code for device registration with rtnetlink.

While there, cleanup ppp_create_interface() prototype to return an
error code rather than the ppp structure. The unit parameter is now
read/write so that caller can retrieve the unit identifier without
accessing the new ppp structure.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 216 +-
 1 file changed, 127 insertions(+), 89 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..bc24537 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -183,6 +183,11 @@ struct channel {
 #endif /* CONFIG_PPP_MULTILINK */
 };
 
+struct ppp_config {
+   struct file *file;
+   s32 unit;
+};
+
 /*
  * SMP locking issues:
  * Both the ppp.rlock and ppp.wlock locks protect the ppp.channels
@@ -269,8 +274,7 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit,
-   struct file *file, int *retp);
+static int ppp_create_interface(struct net *net, int *unit, struct file *file);
 static void init_ppp_file(struct ppp_file *pf, int kind);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
@@ -842,12 +846,13 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, file, );
-   if (!ppp)
+
+   err = ppp_create_interface(net, , file);
+   if (err)
break;
-   file->private_data = >file;
+
err = -EFAULT;
-   if (put_user(ppp->file.index, p))
+   if (put_user(unit, p))
break;
err = 0;
break;
@@ -949,6 +954,101 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
+static int ppp_unit_register(struct ppp *ppp, int unit)
+{
+   struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
+   int ret;
+
+   mutex_lock(>all_ppp_mutex);
+
+   if (unit < 0) {
+   ret = unit_get(>units_idr, ppp);
+   if (ret < 0)
+   goto err;
+   } else {
+   /* Caller asked for a specific unit number. Fail with -EEXIST
+* if unavailable. For backward compatibility, return -EEXIST
+* too if idr allocation fails; this makes pppd retry without
+* requesting a specific unit number.
+*/
+   if (unit_find(>units_idr, unit)) {
+   ret = -EEXIST;
+   goto err;
+   }
+   ret = unit_set(>units_idr, ppp, unit);
+   if (ret < 0) {
+   ret = -EEXIST; /* rewrite error for backward compat. */
+   goto err;
+   }
+   }
+
+   ppp->file.index = ret;
+
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+
+   ret = register_netdevice(ppp->dev);
+   if (ret)
+   goto err_unit;
+
+   atomic_inc(_unit_count);
+
+   mutex_unlock(>all_ppp_mutex);
+
+   return 0;
+
+err_unit:
+   unit_put(>units_idr, ppp->file.index);
+err:
+   mutex_unlock(>all_ppp_mutex);
+
+   return ret;
+}
+
+static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
+const struct ppp_config *conf)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   struct file *file;
+   int indx;
+   int err;
+
+   file = conf->file;
+   if (!file)
+   return -EBADF;
+   if (file->private_data)
+   return -EBUSY;
+
+   init_ppp_file(>file, INTERFACE);
+   ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
+   ppp->owner = file;
+
+   for (indx = 0; indx < NUM_NP; ++indx)
+   ppp->npmode[indx] = NPMODE_PASS;
+   INIT_LIST_HEAD(>channels);
+   spin_lock_init(>rlock);
+   spin_lock_init(>wlock);
+#ifdef CONFIG_PPP_MULTILINK
+   ppp->minseq = -1;
+   skb_queue_head_init(>mrq);
+#endif /* CONFIG_PPP_MULTILINK */
+#ifdef CONFIG_PPP_FILTER
+   ppp->pass_filter = NULL;
+   ppp->active_filter = NULL;
+#endif /* CONFIG_PPP_FILTER */
+
+   ppp->dev = dev;
+   pp

[PATCH net-next] l2tp: rely on ppp layer for skb scrubbing

[Guillaume Nault]

Since 79c441ae505c ("ppp: implement x-netns support"), the PPP layer
calls skb_scrub_packet() whenever the skb is received on the PPP
device. Manually resetting packet meta-data in the L2TP layer is thus
redundant.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 net/l2tp/l2tp_ppp.c | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index d93f113..652c250 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -230,26 +230,11 @@ static void pppol2tp_recv(struct l2tp_session *session, 
struct sk_buff *skb, int
 
if (sk->sk_state & PPPOX_BOUND) {
struct pppox_sock *po;
+
l2tp_dbg(session, PPPOL2TP_MSG_DATA,
 "%s: recv %d byte data frame, passing to ppp\n",
 session->name, data_len);
 
-   /* We need to forget all info related to the L2TP packet
-* gathered in the skb as we are going to reuse the same
-* skb for the inner packet.
-* Namely we need to:
-* - reset xfrm (IPSec) information as it applies to
-*   the outer L2TP packet and not to the inner one
-* - release the dst to force a route lookup on the inner
-*   IP packet since skb->dst currently points to the dst
-*   of the UDP tunnel
-* - reset netfilter information as it doesn't apply
-*   to the inner packet either
-*/
-   secpath_reset(skb);
-   skb_dst_drop(skb);
-   nf_reset(skb);
-
po = pppox_sk(sk);
ppp_input(>chan, skb);
} else {
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

On Wed, Nov 25, 2015 at 12:59:52AM +0200, Andrew wrote:
> Hi.
> 
> I tried to reproduce errors in virtual environment (some VMs on my
> notebook).
> 
> I've tried to create 1000 client PPPoE sessions from this box via script:
> for i in `seq 1 1000`; do pppd plugin rp-pppoe.so user test password test
> nodefaultroute maxfail 0 persist nodefaultroute holdoff 1 noauth eth0; done
> 
I've tried to reproduce the bug with your script, but couldn't get
anything to crash (VM is Debian Jessie i386 running on KVM with upstream
kernel 4.1.12). Does the crash happen before all sessions get
established?

Can you reliably reproduce the bug? If so can you please try with 4.3?
It contains ppp fixes not included in 4.1.12.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

On Mon, Nov 30, 2015 at 12:05:13AM +0200, Andrew wrote:
> 26.11.2015 18:44, Guillaume Nault пишет:
> >On Wed, Nov 25, 2015 at 04:58:54PM +0200, Andrew wrote:
> >>25.11.2015 16:10, Guillaume Nault пишет:
> >>>On Wed, Nov 25, 2015 at 12:59:52AM +0200, Andrew wrote:
> >>>>Hi.
> >>>>
> >>>>I tried to reproduce errors in virtual environment (some VMs on my
> >>>>notebook).
> >>>>
> >>>>I've tried to create 1000 client PPPoE sessions from this box via script:
> >>>>for i in `seq 1 1000`; do pppd plugin rp-pppoe.so user test password test
> >>>>nodefaultroute maxfail 0 persist nodefaultroute holdoff 1 noauth eth0; 
> >>>>done
> >>>>
> >>>I've tried to reproduce the bug with your script, but couldn't get
> >>>anything to crash (VM is Debian Jessie i386 running on KVM with upstream
> >>>kernel 4.1.12). Does the crash happen before all sessions get
> >>>established?
> >>Yes, crash happens even before all daemon instances are started. Sessions
> >>don't get established because BRAS configured to reject sessions (so a lot
> >>of concurrent connection retries happens) - I still didn't created account
> >>for test user on it.
> >>
> >Ok, I got the crash too. In fact I had misunderstood your previous
> >message, crash happens when PPP sessions don't get established
> >(authentication failures in my case).
> >
> >I'll investigate on that and let you know.
> 
> It seems like bug appears on mass ppp devices removing (I planned to use
> this test environment to reproduce BRAS periodical crashes, but suddenly
> I've got crashes on test client).
> 
> I've checked it with some kernels - it's present in 4.3.0, but it isn't
> present in 3.10.57. I'll try to build 3.14/3.18 kernels to look how they
> will work in this case.

Yes, it most likely was introduced by 287f3a943fef ("pppoe: Use
workqueue to die properly when a PADT is received"). I still have to
figure out why.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

[Adding Simon to the discussion]

On Mon, Nov 30, 2015 at 04:03:37PM +0100, Guillaume Nault wrote:
> On Mon, Nov 30, 2015 at 12:05:13AM +0200, Andrew wrote:
> > 26.11.2015 18:44, Guillaume Nault пишет:
> > >On Wed, Nov 25, 2015 at 04:58:54PM +0200, Andrew wrote:
> > >>25.11.2015 16:10, Guillaume Nault пишет:
> > >>>On Wed, Nov 25, 2015 at 12:59:52AM +0200, Andrew wrote:
> > >>>>Hi.
> > >>>>
> > >>>>I tried to reproduce errors in virtual environment (some VMs on my
> > >>>>notebook).
> > >>>>
> > >>>>I've tried to create 1000 client PPPoE sessions from this box via 
> > >>>>script:
> > >>>>for i in `seq 1 1000`; do pppd plugin rp-pppoe.so user test password 
> > >>>>test
> > >>>>nodefaultroute maxfail 0 persist nodefaultroute holdoff 1 noauth eth0; 
> > >>>>done
> > >>>>
> > >>>I've tried to reproduce the bug with your script, but couldn't get
> > >>>anything to crash (VM is Debian Jessie i386 running on KVM with upstream
> > >>>kernel 4.1.12). Does the crash happen before all sessions get
> > >>>established?
> > >>Yes, crash happens even before all daemon instances are started. Sessions
> > >>don't get established because BRAS configured to reject sessions (so a lot
> > >>of concurrent connection retries happens) - I still didn't created account
> > >>for test user on it.
> > >>
> > >Ok, I got the crash too. In fact I had misunderstood your previous
> > >message, crash happens when PPP sessions don't get established
> > >(authentication failures in my case).
> > >
> > >I'll investigate on that and let you know.
> > 
> > It seems like bug appears on mass ppp devices removing (I planned to use
> > this test environment to reproduce BRAS periodical crashes, but suddenly
> > I've got crashes on test client).
> > 
> > I've checked it with some kernels - it's present in 4.3.0, but it isn't
> > present in 3.10.57. I'll try to build 3.14/3.18 kernels to look how they
> > will work in this case.
> 
> Yes, it most likely was introduced by 287f3a943fef ("pppoe: Use
> workqueue to die properly when a PADT is received"). I still have to
> figure out why.

I confirm the bug comes from this commit.

It happens if pppoe_connect() reinitialises po->proto.pppoe.padt_work
after pppoe_disc_rcv() has added it to the system's work queue, and
before that work got scheduled. Then when scheduling occurs, the worker
thread tries to run a corrupted structure and crashes.

I'm going to work on a patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel 4.1.12 crash

[Guillaume Nault]

On Wed, Nov 25, 2015 at 04:58:54PM +0200, Andrew wrote:
> 25.11.2015 16:10, Guillaume Nault пишет:
> >On Wed, Nov 25, 2015 at 12:59:52AM +0200, Andrew wrote:
> >>Hi.
> >>
> >>I tried to reproduce errors in virtual environment (some VMs on my
> >>notebook).
> >>
> >>I've tried to create 1000 client PPPoE sessions from this box via script:
> >>for i in `seq 1 1000`; do pppd plugin rp-pppoe.so user test password test
> >>nodefaultroute maxfail 0 persist nodefaultroute holdoff 1 noauth eth0; done
> >>
> >I've tried to reproduce the bug with your script, but couldn't get
> >anything to crash (VM is Debian Jessie i386 running on KVM with upstream
> >kernel 4.1.12). Does the crash happen before all sessions get
> >established?
> Yes, crash happens even before all daemon instances are started. Sessions
> don't get established because BRAS configured to reject sessions (so a lot
> of concurrent connection retries happens) - I still didn't created account
> for test user on it.
> 
Ok, I got the crash too. In fact I had misunderstood your previous
message, crash happens when PPP sessions don't get established
(authentication failures in my case).

I'll investigate on that and let you know.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 1/2] ppp: don't set sk_state to PPPOX_ZOMBIE in pppoe_disc_rcv()

[Guillaume Nault]

Since 287f3a943fef ("pppoe: Use workqueue to die properly when a PADT
is received"), pppoe_disc_rcv() disconnects the socket by scheduling
pppoe_unbind_sock_work(). This is enough to stop socket transmission
and makes the PPPOX_ZOMBIE state uncessary.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c | 22 ++
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 5e0b432..1dedfbf 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -500,27 +500,9 @@ static int pppoe_disc_rcv(struct sk_buff *skb, struct 
net_device *dev,
 
pn = pppoe_pernet(dev_net(dev));
po = get_item(pn, ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
-   if (po) {
-   struct sock *sk = sk_pppox(po);
-
-   bh_lock_sock(sk);
-
-   /* If the user has locked the socket, just ignore
-* the packet.  With the way two rcv protocols hook into
-* one socket family type, we cannot (easily) distinguish
-* what kind of SKB it is during backlog rcv.
-*/
-   if (sock_owned_by_user(sk) == 0) {
-   /* We're no longer connect at the PPPOE layer,
-* and must wait for ppp channel to disconnect us.
-*/
-   sk->sk_state = PPPOX_ZOMBIE;
-   }
-
-   bh_unlock_sock(sk);
+   if (po)
if (!schedule_work(>proto.pppoe.padt_work))
-   sock_put(sk);
-   }
+   sock_put(sk_pppox(po));
 
 abort:
kfree_skb(skb);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 2/2] ppp: remove PPPOX_ZOMBIE socket state

[Guillaume Nault]

PPPOX_ZOMBIE is never set anymore.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/pppoe.c  | 4 ++--
 drivers/net/ppp/pppox.c  | 2 +-
 include/linux/if_pppox.h | 1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 1dedfbf..277e682 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -311,7 +311,7 @@ static void pppoe_flush_dev(struct net_device *dev)
lock_sock(sk);
 
if (po->pppoe_dev == dev &&
-   sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | 
PPPOX_ZOMBIE)) {
+   sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND)) {
pppox_unbind_sock(sk);
sk->sk_state_change(sk);
po->pppoe_dev = NULL;
@@ -775,7 +775,7 @@ static int pppoe_ioctl(struct socket *sock, unsigned int 
cmd,
struct pppox_sock *relay_po;
 
err = -EBUSY;
-   if (sk->sk_state & (PPPOX_BOUND | PPPOX_ZOMBIE | PPPOX_DEAD))
+   if (sk->sk_state & (PPPOX_BOUND | PPPOX_DEAD))
break;
 
err = -ENOTCONN;
diff --git a/drivers/net/ppp/pppox.c b/drivers/net/ppp/pppox.c
index 0e1b306..0200de7 100644
--- a/drivers/net/ppp/pppox.c
+++ b/drivers/net/ppp/pppox.c
@@ -58,7 +58,7 @@ void pppox_unbind_sock(struct sock *sk)
 {
/* Clear connection to ppp device, if attached. */
 
-   if (sk->sk_state & (PPPOX_BOUND | PPPOX_CONNECTED | PPPOX_ZOMBIE)) {
+   if (sk->sk_state & (PPPOX_BOUND | PPPOX_CONNECTED)) {
ppp_unregister_channel(_sk(sk)->chan);
sk->sk_state = PPPOX_DEAD;
}
diff --git a/include/linux/if_pppox.h b/include/linux/if_pppox.h
index b49cf92..ba7a9b0 100644
--- a/include/linux/if_pppox.h
+++ b/include/linux/if_pppox.h
@@ -91,7 +91,6 @@ enum {
 PPPOX_CONNECTED= 1,  /* connection established ==TCP_ESTABLISHED */
 PPPOX_BOUND= 2,  /* bound to ppp device */
 PPPOX_RELAY= 4,  /* forwarding is enabled */
-PPPOX_ZOMBIE   = 8,  /* dead, but still bound to ppp device */
 PPPOX_DEAD = 16  /* dead, useless, please clean me up!*/
 };
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 0/2] ppp: Remove PPPOX_ZOMBIE socket state

[Guillaume Nault]

Several issues have been found lately wrt. the PPPOX_ZOMBIE socket
state. This state is now only set upon reception of a PADT to stop
further transmissions. However this is redundant with the PADT
workqueue mechanism introduced by 287f3a943fef ("pppoe: Use workqueue
to die properly when a PADT is received").

We can thus simplify pppox socket state handling by getting rid of
PPPOX_ZOMBIE entirely.


Guillaume Nault (2):
  ppp: don't set sk_state to PPPOX_ZOMBIE in pppoe_disc_rcv()
  ppp: remove PPPOX_ZOMBIE socket state

 drivers/net/ppp/pppoe.c  | 26 --
 drivers/net/ppp/pppox.c  |  2 +-
 include/linux/if_pppox.h |  1 -
 3 files changed, 5 insertions(+), 24 deletions(-)

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 net] l2tp: fix configuration passed to setup_udp_tunnel_sock()

[Guillaume Nault]

Unused fields of udp_cfg must be all zeros. Otherwise
setup_udp_tunnel_sock() fills ->gro_receive and ->gro_complete
callbacks with garbage, eventually resulting in panic when used by
udp_gro_receive().

[   72.694123] BUG: unable to handle kernel paging request at 880033f87d78
[   72.695518] IP: [] 0x880033f87d78
[   72.696530] PGD 26e2067 PUD 26e3067 PMD 342ed063 PTE 800033f87163
[   72.696530] Oops: 0011 [#1] SMP KASAN
[   72.696530] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core 
ip6_udp_tunnel udp_tunnel pptp gre pppox ppp_generic slhc crc32c_intel 
ghash_clmulni_intel jitterentropy_rng sha256_generic hmac drbg ansi_cprng 
aesni_intel evdev aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper 
serio_raw acpi_cpufreq button proc\
essor ext4 crc16 jbd2 mbcache virtio_blk virtio_net virtio_pci virtio_ring 
virtio
[   72.696530] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.7.0-rc1 #1
[   72.696530] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Debian-1.8.2-1 04/01/2014
[   72.696530] task: 880035b59700 ti: 880035b7 task.ti: 
880035b7
[   72.696530] RIP: 0010:[]  [] 
0x880033f87d78
[   72.696530] RSP: 0018:880035f87bc0  EFLAGS: 00010246
[   72.696530] RAX: ed000698f996 RBX: 88003326b840 RCX: 814cc823
[   72.696530] RDX: 88003326b840 RSI: 880033e48038 RDI: 880034c7c780
[   72.696530] RBP: 880035f87c18 R08: a506 R09: 
[   72.696530] R10: 880035f87b38 R11: 880034b9344d R12: ebfea715
[   72.696530] R13:  R14: 880034c7c780 R15: 
[   72.696530] FS:  () GS:880035f8() 
knlGS:
[   72.696530] CS:  0010 DS:  ES:  CR0: 80050033
[   72.696530] CR2: 880033f87d78 CR3: 33c98000 CR4: 000406a0
[   72.696530] Stack:
[   72.696530]  814cc834 880034b93468 001481416818 
88003326b874
[   72.696530]  880034c7ccb0 880033e48038 88003326b840 
880034b93462
[   72.696530]  88003326b88a 88003326b88c 880034b93468 
880035f87c70
[   72.696530] Call Trace:
[   72.696530]  
[   72.696530]  [] ? udp_gro_receive+0x1c6/0x1f9
[   72.696530]  [] udp4_gro_receive+0x2b5/0x310
[   72.696530]  [] inet_gro_receive+0x4a3/0x4cd
[   72.696530]  [] dev_gro_receive+0x584/0x7a3
[   72.696530]  [] ? __lock_is_held+0x29/0x64
[   72.696530]  [] napi_gro_receive+0x124/0x21d
[   72.696530]  [] virtnet_receive+0x8df/0x8f6 [virtio_net]
[   72.696530]  [] virtnet_poll+0x1d/0x8d [virtio_net]
[   72.696530]  [] net_rx_action+0x15b/0x3b9
[   72.696530]  [] __do_softirq+0x216/0x546
[   72.696530]  [] irq_exit+0x49/0xb6
[   72.696530]  [] do_IRQ+0xe2/0xfa
[   72.696530]  [] common_interrupt+0x89/0x89
[   72.696530]  
[   72.696530]  [] ? trace_hardirqs_on_caller+0x229/0x270
[   72.696530]  [] ? default_idle+0x1c/0x2d
[   72.696530]  [] ? default_idle+0x1a/0x2d
[   72.696530]  [] arch_cpu_idle+0xa/0xc
[   72.696530]  [] default_idle_call+0x1a/0x1c
[   72.696530]  [] cpu_startup_entry+0x15b/0x20f
[   72.696530]  [] start_secondary+0x12c/0x133
[   72.696530] Code: ff ff ff ff ff ff ff ff ff ff 7f ff ff ff ff ff ff ff 7f 
00 7e f8 33 00 88 ff ff 6d 61 58 81 ff ff ff ff 5e de 0a 81 ff ff ff ff <00> 5c 
e2 34 00 88 ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00
[   72.696530] RIP  [] 0x880033f87d78
[   72.696530]  RSP 
[   72.696530] CR2: 880033f87d78
[   72.696530] ---[ end trace ad7758b9a1dccf99 ]---
[   72.696530] Kernel panic - not syncing: Fatal exception in interrupt
[   72.696530] Kernel Offset: disabled
[   72.696530] ---[ end Kernel panic - not syncing: Fatal exception in interrupt

v2: use empty initialiser instead of "{ NULL }" to avoid relying on
first field's type.

Fixes: 38fd2af24fcf ("udp: Add socket based GRO and config")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 net/l2tp/l2tp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6edfa99..1e40dac 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1581,7 +1581,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int 
version, u32 tunnel_id, u32
/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
tunnel->encap = encap;
if (encap == L2TP_ENCAPTYPE_UDP) {
-   struct udp_tunnel_sock_cfg udp_cfg;
+   struct udp_tunnel_sock_cfg udp_cfg = { };
 
udp_cfg.sk_user_data = tunnel;
udp_cfg.encap_type = UDP_ENCAP_L2TPINUDP;
-- 
2.8.1

Re: [PATCH net] l2tp: fix configuration passed to setup_udp_tunnel_sock()

[Guillaume Nault]

On Tue, Jun 07, 2016 at 04:20:17PM -0700, David Miller wrote:
> From: Guillaume Nault <g.na...@alphalink.fr>
> Date: Mon, 6 Jun 2016 13:48:02 +0200
> 
> > @@ -1581,7 +1581,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int 
> > version, u32 tunnel_id, u32
> > /* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
> > tunnel->encap = encap;
> > if (encap == L2TP_ENCAPTYPE_UDP) {
> > -   struct udp_tunnel_sock_cfg udp_cfg;
> > +   struct udp_tunnel_sock_cfg udp_cfg = { NULL };
> 
> This assumes that the first member of udp_tunnel_sock_cfg is, and will always
> be, a pointer.
> 
> You could use a named initializer to make this better, but the cleanest way to
> deal with this is to use an empty initializer "{ }", or just plain memset()
> the thing to zero.
> 
Indeed. I'll repost using "{ }" then.

[PATCH net] l2tp: fix configuration passed to setup_udp_tunnel_sock()

[Guillaume Nault]

Unused fields of udp_cfg must be all zeros. Otherwise
setup_udp_tunnel_sock() fills ->gro_receive and ->gro_complete
callbacks with garbage, eventually resulting in panic when used by
udp_gro_receive().

[   72.694123] BUG: unable to handle kernel paging request at 880033f87d78
[   72.695518] IP: [] 0x880033f87d78
[   72.696530] PGD 26e2067 PUD 26e3067 PMD 342ed063 PTE 800033f87163
[   72.696530] Oops: 0011 [#1] SMP KASAN
[   72.696530] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core 
ip6_udp_tunnel udp_tunnel pptp gre pppox ppp_generic slhc crc32c_intel 
ghash_clmulni_intel jitterentropy_rng sha256_generic hmac drbg ansi_cprng 
aesni_intel evdev aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper 
serio_raw acpi_cpufreq button proc\
essor ext4 crc16 jbd2 mbcache virtio_blk virtio_net virtio_pci virtio_ring 
virtio
[   72.696530] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.7.0-rc1 #1
[   72.696530] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Debian-1.8.2-1 04/01/2014
[   72.696530] task: 880035b59700 ti: 880035b7 task.ti: 
880035b7
[   72.696530] RIP: 0010:[]  [] 
0x880033f87d78
[   72.696530] RSP: 0018:880035f87bc0  EFLAGS: 00010246
[   72.696530] RAX: ed000698f996 RBX: 88003326b840 RCX: 814cc823
[   72.696530] RDX: 88003326b840 RSI: 880033e48038 RDI: 880034c7c780
[   72.696530] RBP: 880035f87c18 R08: a506 R09: 
[   72.696530] R10: 880035f87b38 R11: 880034b9344d R12: ebfea715
[   72.696530] R13:  R14: 880034c7c780 R15: 
[   72.696530] FS:  () GS:880035f8() 
knlGS:
[   72.696530] CS:  0010 DS:  ES:  CR0: 80050033
[   72.696530] CR2: 880033f87d78 CR3: 33c98000 CR4: 000406a0
[   72.696530] Stack:
[   72.696530]  814cc834 880034b93468 001481416818 
88003326b874
[   72.696530]  880034c7ccb0 880033e48038 88003326b840 
880034b93462
[   72.696530]  88003326b88a 88003326b88c 880034b93468 
880035f87c70
[   72.696530] Call Trace:
[   72.696530]  
[   72.696530]  [] ? udp_gro_receive+0x1c6/0x1f9
[   72.696530]  [] udp4_gro_receive+0x2b5/0x310
[   72.696530]  [] inet_gro_receive+0x4a3/0x4cd
[   72.696530]  [] dev_gro_receive+0x584/0x7a3
[   72.696530]  [] ? __lock_is_held+0x29/0x64
[   72.696530]  [] napi_gro_receive+0x124/0x21d
[   72.696530]  [] virtnet_receive+0x8df/0x8f6 [virtio_net]
[   72.696530]  [] virtnet_poll+0x1d/0x8d [virtio_net]
[   72.696530]  [] net_rx_action+0x15b/0x3b9
[   72.696530]  [] __do_softirq+0x216/0x546
[   72.696530]  [] irq_exit+0x49/0xb6
[   72.696530]  [] do_IRQ+0xe2/0xfa
[   72.696530]  [] common_interrupt+0x89/0x89
[   72.696530]  
[   72.696530]  [] ? trace_hardirqs_on_caller+0x229/0x270
[   72.696530]  [] ? default_idle+0x1c/0x2d
[   72.696530]  [] ? default_idle+0x1a/0x2d
[   72.696530]  [] arch_cpu_idle+0xa/0xc
[   72.696530]  [] default_idle_call+0x1a/0x1c
[   72.696530]  [] cpu_startup_entry+0x15b/0x20f
[   72.696530]  [] start_secondary+0x12c/0x133
[   72.696530] Code: ff ff ff ff ff ff ff ff ff ff 7f ff ff ff ff ff ff ff 7f 
00 7e f8 33 00 88 ff ff 6d 61 58 81 ff ff ff ff 5e de 0a 81 ff ff ff ff <00> 5c 
e2 34 00 88 ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00
[   72.696530] RIP  [] 0x880033f87d78
[   72.696530]  RSP 
[   72.696530] CR2: 880033f87d78
[   72.696530] ---[ end trace ad7758b9a1dccf99 ]---
[   72.696530] Kernel panic - not syncing: Fatal exception in interrupt
[   72.696530] Kernel Offset: disabled
[   72.696530] ---[ end Kernel panic - not syncing: Fatal exception in interrupt

Fixes: 38fd2af24fcf ("udp: Add socket based GRO and config")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 net/l2tp/l2tp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6edfa99..1c39228 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1581,7 +1581,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int 
version, u32 tunnel_id, u32
/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
tunnel->encap = encap;
if (encap == L2TP_ENCAPTYPE_UDP) {
-   struct udp_tunnel_sock_cfg udp_cfg;
+   struct udp_tunnel_sock_cfg udp_cfg = { NULL };
 
udp_cfg.sk_user_data = tunnel;
udp_cfg.encap_type = UDP_ENCAP_L2TPINUDP;
-- 
2.8.1

Re: Fw: [Bug 111771] New: deadlock in ppp/l2tp

[Guillaume Nault]

On Thu, Feb 04, 2016 at 02:32:48AM +0100, Sorin Manolache wrote:
> On 2016-02-03 18:14, Guillaume Nault wrote:
> > 
> >Sorin, it seems like one of your L2TP tunnels is routed to one of its upper 
> >PPP
> >devices. Most likely, the peer address of the PPP device is also the address 
> >of
> >the remote L2TP tunnel endpoint. So L2TP packets are sent back to the upper 
> >PPP
> >device, instead of leaving through the physical interface.
> 
> Thank you. You are right. There's a host route to the peer over the ppp0
> interface in the routing table. I don't know how it gets there. I've checked
> the source code of pppd and no such route is added for kernels newer than
> 2.1.16. I've grepped /etc for "route" in order to detect a "post-up" script
> that would add that route. Nothing. I've double-checked by executing strace
> on xl2tpd and its children (i.e. pppd and the initialisation scripts) and I
> couldn't find any ioctl SIOCADDRT. So it's a total mystery for me where that
> route comes from. Could it come from the kernel?
> 
If that's a /32 IPv4 route to the peer address of the PPP link and has
the "proto kernel" attribute, then yes, that's most likely the one
generated by the kernel.

Re: Fw: [Bug 111771] New: deadlock in ppp/l2tp

[Guillaume Nault]

On Wed, Feb 03, 2016 at 11:04:31AM +1100, Stephen Hemminger wrote:
> Please excuse URL mangling, my bugzilla address appears to route through
> stupid corporate firewall.
> 
> Begin forwarded message:
> 
> Date: Tue, 2 Feb 2016 18:38:41 +
> From: "bugzilla-dae...@bugzilla.kernel.org" 
> 
> To: "shemmin...@linux-foundation.org" 
> Subject: [Bug 111771] New: deadlock in ppp/l2tp
> 
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.kernel.org_show-5Fbug.cgi-3Fid-3D111771=CwICaQ=IL_XqQWOjubgfqINi2jTzg=q_lvUiVm1uM6QEw9TPH-6jiV__hsrE6xXUAtATPE9x0=QRVzJYt9nD-EOW0XdrPpw2-kYmZu0sg62aaPeiiLI_Q=l3HC8fgAgyPVwSgMaX2Hjr8GL3P5j2fL1kDXhEW-v9w=
>  
> 
> Bug ID: 111771
>Summary: deadlock in ppp/l2tp
>Product: Networking
>Version: 2.5
> Kernel Version: 4.3.2
>   Hardware: x86-64
> OS: Linux
>   Tree: Mainline
> Status: NEW
>   Severity: normal
>   Priority: P1
>  Component: Other
>   Assignee: shemmin...@linux-foundation.org
>   Reporter: sor...@gmail.com
> Regression: No
> 
> Created attachment 202771
>   --> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.kernel.org_attachment.cgi-3Fid-3D202771-26action-3Dedit=CwICaQ=IL_XqQWOjubgfqINi2jTzg=q_lvUiVm1uM6QEw9TPH-6jiV__hsrE6xXUAtATPE9x0=QRVzJYt9nD-EOW0XdrPpw2-kYmZu0sg62aaPeiiLI_Q=2a_KCXgtoQ6NLxDon6_3flSUMpb7Tjj8WhGPZ09E8Vo=
>  
> kernel.log, config, cpuinfo
> 
> I'm getting a deadlock and the computer is unresponsive shortly after setting
> up a xl2tpd/ipsec vpn connection. The deadlock occurs every time, it is very
> reproducible on my computer.
> 
> It is the mainline kernel, version 4.3.2.
> 

Sorin, it seems like one of your L2TP tunnels is routed to one of its upper PPP
devices. Most likely, the peer address of the PPP device is also the address of
the remote L2TP tunnel endpoint. So L2TP packets are sent back to the upper PPP
device, instead of leaving through the physical interface.

For easier reference, here the trace extracted from bugzilla:

Feb  2 19:02:06 leo-naphta kernel: [  329.888935] 
==
Feb  2 19:02:06 leo-naphta kernel: [  329.888939] [ INFO: possible circular 
locking dependency detected ]
Feb  2 19:02:06 leo-naphta kernel: [  329.888945] 4.3.2 #5 Not tainted
Feb  2 19:02:06 leo-naphta kernel: [  329.888948] 
---
Feb  2 19:02:06 leo-naphta kernel: [  329.888952] pppd/4034 is trying to 
acquire lock:
Feb  2 19:02:06 leo-naphta kernel: [  329.888956]  (_xmit_PPP#2){+.}, at: 
[] sch_direct_xmit+0xf3/0x1f0
Feb  2 19:02:06 leo-naphta kernel: [  329.888979] 
Feb  2 19:02:06 leo-naphta kernel: [  329.888979] but task is already holding 
lock:
Feb  2 19:02:06 leo-naphta kernel: [  329.888982]  (l2tp_sock){+.-...}, at: 
[] l2tp_xmit_skb+0x117/0x3d0 [l2tp_core]
Feb  2 19:02:06 leo-naphta kernel: [  329.888997] 
Feb  2 19:02:06 leo-naphta kernel: [  329.888997] which lock already depends on 
the new lock.
Feb  2 19:02:06 leo-naphta kernel: [  329.888997] 
Feb  2 19:02:06 leo-naphta kernel: [  329.889002] 
Feb  2 19:02:06 leo-naphta kernel: [  329.889002] the existing dependency chain 
(in reverse order) is:
Feb  2 19:02:06 leo-naphta kernel: [  329.889006] 
Feb  2 19:02:06 leo-naphta kernel: [  329.889006] -> #3 (l2tp_sock){+.-...}:
Feb  2 19:02:06 leo-naphta kernel: [  329.889015][] 
lock_acquire+0x60/0x80
Feb  2 19:02:06 leo-naphta kernel: [  329.889023][] 
_raw_spin_lock+0x33/0x50
Feb  2 19:02:06 leo-naphta kernel: [  329.889033][] 
l2tp_xmit_skb+0x117/0x3d0 [l2tp_core]
Feb  2 19:02:06 leo-naphta kernel: [  329.889041][] 
pppol2tp_xmit+0x143/0x1f3 [l2tp_ppp]
Feb  2 19:02:06 leo-naphta kernel: [  329.889049][] 
ppp_channel_push+0x4b/0xc0 [ppp_generic]
Feb  2 19:02:06 leo-naphta kernel: [  329.889057][] 
ppp_write+0xc0/0xf0 [ppp_generic]
Feb  2 19:02:06 leo-naphta kernel: [  329.889065][] 
__vfs_write+0x23/0xe0
Feb  2 19:02:06 leo-naphta kernel: [  329.889074][] 
vfs_write+0x9d/0x180
Feb  2 19:02:06 leo-naphta kernel: [  329.889080][] 
SyS_write+0x44/0xa0
Feb  2 19:02:06 leo-naphta kernel: [  329.889086][] 
entry_SYSCALL_64_fastpath+0x16/0x6f
Feb  2 19:02:06 leo-naphta kernel: [  329.889094] 
Feb  2 19:02:06 leo-naphta kernel: [  329.889094] -> #2 
(&(>downl)->rlock){+.}:
Feb  2 19:02:06 leo-naphta kernel: [  329.889102][] 
lock_acquire+0x60/0x80
Feb  2 19:02:06 leo-naphta kernel: [  329.889107][] 
_raw_spin_lock_bh+0x3a/0x50
Feb  2 19:02:06 leo-naphta kernel: [  329.889114][] 
ppp_push+0x10a/0x5b0 [ppp_generic]
Feb  2 19:02:06 leo-naphta kernel: [  329.889133][] 
ppp_xmit_process+0x3f5/0x5d0 [ppp_generic]
Feb  2 19:02:06 leo-naphta kernel: [  329.889141][] 
ppp_write+0xca/0xf0 

[PATCH net 3/5] ppp: protect ppp->debug in ppp_ioctl()

[Guillaume Nault]

ppp->debug is read in the Tx and Rx paths while under protection of
ppp_xmit_lock() and ppp_recv_lock() respectively.
So ppp_ioctl() must hold both locks before concurrently updating it.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
Locking is not strictly necessary for PPPIOCGDEBUG, because ppp->debug
can only be modified by ioctl(PPPIOCSDEBUG) which is guaranteed not to
run concurrently thanks to ppp_mutex. I've added the locking in
PPPIOCGDEBUG in order to respect the general locking semantic of
ppp->debug and to avoid relying on ppp_mutex.

 drivers/net/ppp/ppp_generic.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4af548b..183d89c 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -708,12 +708,19 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
case PPPIOCSDEBUG:
if (get_user(val, p))
break;
+   ppp_lock(ppp);
ppp->debug = val;
+   ppp_unlock(ppp);
+
err = 0;
break;
 
case PPPIOCGDEBUG:
-   if (put_user(ppp->debug, p))
+   ppp_lock(ppp);
+   val = ppp->debug;
+   ppp_unlock(ppp);
+
+   if (put_user(val, p))
break;
err = 0;
break;
-- 
2.7.0

[PATCH net 2/5] ppp: fix unprotected accesses to ppp->flags and ppp->n_channels

[Guillaume Nault]

Reading ppp->flags has to be done under protection of ppp_recv_lock()
or ppp_xmit_lock() (for Rx and Tx paths respectively). Therefore, both
locks have to be held before writing.

This patch adds missing locking in ppp_read(), ppp_poll() and
ppp_ioctl(). As a side effect, it fixes unprotected access to
ppp->n_channels in ppp_read() and ppp_poll(), which has the same
locking requirements as ppp->flags.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4d342ae..4af548b 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user 
*buf,
 * network traffic (demand mode).
 */
struct ppp *ppp = PF_TO_PPP(pf);
+
+   ppp_recv_lock(ppp);
if (ppp->n_channels == 0 &&
-   (ppp->flags & SC_LOOP_TRAFFIC) == 0)
+   (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
+   ppp_recv_unlock(ppp);
break;
+   }
+   ppp_recv_unlock(ppp);
}
ret = -EAGAIN;
if (file->f_flags & O_NONBLOCK)
@@ -532,9 +537,12 @@ static unsigned int ppp_poll(struct file *file, poll_table 
*wait)
else if (pf->kind == INTERFACE) {
/* see comment in ppp_read */
struct ppp *ppp = PF_TO_PPP(pf);
+
+   ppp_recv_lock(ppp);
if (ppp->n_channels == 0 &&
(ppp->flags & SC_LOOP_TRAFFIC) == 0)
mask |= POLLIN | POLLRDNORM;
+   ppp_recv_unlock(ppp);
}
 
return mask;
@@ -678,7 +686,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
break;
 
case PPPIOCGFLAGS:
+   ppp_lock(ppp);
val = ppp->flags | ppp->xstate | ppp->rstate;
+   ppp_unlock(ppp);
+
if (put_user(val, p))
break;
err = 0;
-- 
2.7.0

[PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
and ppp_recv_lock() respectively.
Therefore ppp_ioctl() must hold the xmit and recv locks before
concurrently updating ppp->mru.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..4d342ae 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
case PPPIOCSMRU:
if (get_user(val, p))
break;
+   ppp_lock(ppp);
ppp->mru = val;
+   ppp_unlock(ppp);
+
err = 0;
break;
 
-- 
2.7.0

[PATCH net 5/5] ppp: protect ppp->npmode

[Guillaume Nault]

ppp->npmode is read by ppp_ioctl(), ppp_receive_nonmp_frame() and
ppp_start_xmit(). But it is only modified by ppp_ioctl().
However, the only protected access is done by ppp_receive_nonmp_frame()
which runs under ppp_recv_lock() protection.

We could protect ppp->npmode with ppp_recv_lock() in ppp_start_xmit()
too, but taking the recv lock in the xmit path would look strange. So
this patch takes the xmit lock instead and holds both locks before
writing in ppp_ioctl().

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 24777f4..2eb39cd 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -767,11 +767,18 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
i = err;
if (cmd == PPPIOCGNPMODE) {
err = -EFAULT;
+
+   ppp_lock(ppp);
npi.mode = ppp->npmode[i];
+   ppp_unlock(ppp);
+
if (copy_to_user(argp, , sizeof(npi)))
break;
} else {
+   ppp_lock(ppp);
ppp->npmode[i] = npi.mode;
+   ppp_unlock(ppp);
+
/* we may be able to transmit more packets now (??) */
netif_wake_queue(ppp->dev);
}
@@ -1023,13 +1030,18 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device 
*dev)
struct ppp *ppp = netdev_priv(dev);
int npi, proto;
unsigned char *pp;
+   enum NPmode npmode;
 
npi = ethertype_to_npindex(ntohs(skb->protocol));
if (npi < 0)
goto outf;
 
+   ppp_xmit_lock(ppp);
+   npmode = ppp->npmode[npi];
+   ppp_xmit_unlock(ppp);
+
/* Drop, accept or reject the packet */
-   switch (ppp->npmode[npi]) {
+   switch (npmode) {
case NPMODE_PASS:
break;
case NPMODE_QUEUE:
-- 
2.7.0

[PATCH net 4/5] ppp: protect access to ppp->last_{xmit,recv} in ppp_ioctl()

[Guillaume Nault]

ppp->last_xmit is witten to by ppp_send_frame() while protected by
ppp_xmit_lock().
Likewise, ppp->last_recv is written to by ppp_receive_nonmp_frame()
while protected by ppp_recv_lock().

Holding both locks is therefore necessary before ppp_ioctl() can safely
read these fields.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 183d89c..24777f4 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -726,8 +726,11 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
break;
 
case PPPIOCGIDLE:
+   ppp_lock(ppp);
idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
idle.recv_idle = (jiffies - ppp->last_recv) / HZ;
+   ppp_unlock(ppp);
+
if (copy_to_user(argp, , sizeof(idle)))
break;
err = 0;
-- 
2.7.0

[PATCH net 0/5] ppp: fix locking issues related to ppp_ioctl()

[Guillaume Nault]

This series fixes unprotected accesses to several struct ppp fields.
Only fields used in ppp_ioctl() have been considered, though.

Locking of the xstate and rstate fields remains incomplete: although a
side effect of patch #2 provides protection in ppp_ioctl(), xstate and
rstate can still be modified without appropriate locking by
ppp_ccp_peek(). Taking the missing locks in ppp_ccp_peek() isn't
possible as this would lead to lock inversion (when protecting xstate
with ppp_xmit_lock() while ppp_ccp_peek() is called in the Rx path).

Using a workqueue to run ppp_ccp_peek() might be a solution, but this
is left for another series.

Guillaume Nault (5):
  ppp: lock ppp structure before modifying mru in ppp_ioctl()
  ppp: fix unprotected accesses to ppp->flags and ppp->n_channels
  ppp: protect ppp->debug in ppp_ioctl()
  ppp: protect access to ppp->last{xmit,recv} in ppp_ioctl()
  ppp: protect ppp->npmode

 drivers/net/ppp/ppp_generic.c | 42 +++---
 1 file changed, 39 insertions(+), 3 deletions(-)

-- 
2.7.0

[PATCH net-next] ppp: clarify parsing of user supplied data in ppp_set_compress()

[Guillaume Nault]

  * Split big conditional statement.
  * Check (data.length <= CCP_MAX_OPTION_LENGTH) only once.
  * Don't read ccp_option[1] if not initialised.

Reading uninitialised ccp_option[1] was harmless, because this could
only happen when data.length was 0 or 1. So even then, we couldn't pass
the (ccp_option[1] < 2 || ccp_option[1] > data.length) test anyway.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..04f4eb3 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2429,13 +2429,15 @@ ppp_set_compress(struct ppp *ppp, unsigned long arg)
unsigned char ccp_option[CCP_MAX_OPTION_LENGTH];
 
err = -EFAULT;
-   if (copy_from_user(, (void __user *) arg, sizeof(data)) ||
-   (data.length <= CCP_MAX_OPTION_LENGTH &&
-copy_from_user(ccp_option, (void __user *) data.ptr, data.length)))
+   if (copy_from_user(, (void __user *) arg, sizeof(data)))
goto out;
+   if (data.length > CCP_MAX_OPTION_LENGTH)
+   goto out;
+   if (copy_from_user(ccp_option, (void __user *) data.ptr, data.length))
+   goto out;
+
err = -EINVAL;
-   if (data.length > CCP_MAX_OPTION_LENGTH ||
-   ccp_option[1] < 2 || ccp_option[1] > data.length)
+   if (data.length < 2 || ccp_option[1] < 2 || ccp_option[1] > data.length)
goto out;
 
cp = try_then_request_module(
-- 
2.7.0

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

On Wed, Feb 24, 2016 at 03:32:02PM -0500, David Miller wrote:
> From: Guillaume Nault <g.na...@alphalink.fr>
> Date: Mon, 22 Feb 2016 20:47:13 +0100
> 
> > PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> > and ppp_recv_lock() respectively.
> > Therefore ppp_ioctl() must hold the xmit and recv locks before
> > concurrently updating ppp->mru.
> > 
> > Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
>  ...
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index fc8ad00..4d342ae 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int 
> > cmd, unsigned long arg)
> > case PPPIOCSMRU:
> > if (get_user(val, p))
> > break;
> > +   ppp_lock(ppp);
> > ppp->mru = val;
> > +   ppp_unlock(ppp);
> > +
> 
> I see no bug here at all.
> 
> The store here is atomic, and all of those mentioned code paths only
> read the MRU once and then use that value for the duration of the
> rest of the processing of that PPP frame.
> 
Ok, I didn't think we could assume atomic stores for int on all arch.

> No possible corruptions or misbehavior can occur and I therefore think
> the lack of locking here is completely legitimate.
> 
Then this is also legitimate for most of the other fields considered in
this series. I'll drop the patches.

One exception is the n_channels and flags fields (patch #2). The update
side is done with read-modify-write instructions ('ppp->flags &= ~XXX'
in ppp_ccp_closed(), '++ppp->n_channels' in ppp_connect_channel()). So
locking should be required. I haven't succeeded in triggering any
misbehaviour from userspace though.

> You absolutely must demonstrate a case of corruption or misbehavior
> when you want to add supposedly "missing locking".  Otherwise I'll have
> a hard time accepting your changes.  This is especially for a subsystem
> that as been around as long as PPP.
Understood. Just to be sure, does patch #2 falls under lack of
demonstration? Or should I repost it separately?

Re: [PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[Guillaume Nault]

On Mon, Feb 29, 2016 at 10:18:38AM +, David Laight wrote:
> From: Guillaume Nault
> > Sent: 26 February 2016 17:46
> > 
> > ppp_read() and ppp_poll() can be called concurrently with ppp_ioctl().
> > In this case, ppp_ioctl() might call ppp_ccp_closed(), which may update
> > ppp->flags while ppp_read() or ppp_poll() is reading it.
> > The update done by ppp_ccp_closed() isn't atomic due to the bit mask
> > operation ('ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)'), so concurrent
> > readers might get transient values.
> > Reading incorrect ppp->flags may disturb the 'ppp->flags & SC_LOOP_TRAFFIC'
> > test in ppp_read() and ppp_poll(), which in turn can lead to improper
> > decision on whether the PPP unit file is ready for reading or not.
> > 
> > Since ppp_ccp_closed() is protected by the Rx and Tx locks (with
> > ppp_lock()), taking the Rx lock is enough for ppp_read() and ppp_poll()
> > to guarantee that ppp_ccp_closed() won't update ppp->flags
> > concurrently.
> 
> This is all splurious.
> The 'concurrent' calls cannot be distinguished from calls just prior to,
> or just after the ppp_ccp_closed() call.
> 
If the ppp->flags update is guaranteed to be atomic from a reader's
point of view, then yes, concurrent runs of ppp_{read,poll}() and
ppp_ccp_closed() aren't an issue.

Here I assume that 'ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)' isn't
atomic, and that a concurrent reader may get a value different from
the original or final value of ppp->flags. If ppp_read() or ppp_poll()
reads such a transient value that affects the SC_LOOP_TRAFFIC bit, the
code may take the wrong decision as to whether or not the file
descriptor is available for reading.

ppp_ioctl()
   |
   \_ ppp_ccp_closed()
|
\_ (1) Start Read-Modify-Write operation on ppp->flags
   |
   |
   | <-- (2) ppp_read() or ppp_poll() reads transient value
   | of ppp->flags here
   |
   (3) End Read-Modify-Write operation on ppp->flags

If we assume that the RMW operation can't generate transient values, or
that transient values can't affect the SC_LOOP_TRAFFIC bit, then this
patch is not needed. However I'm not aware of such guarantees (unless
the atomic stores of ints described by DaveM in my original series
also applies here). At least I haven't seen other parts of the stack
implicitely relying in atomic properties of RMW operations performed on
plain integers.

If my analysis is too far fetched, I'll happily drop the patch.
I'm just trying to fix the possible issues I saw while working on the
PPP code, before resubmitting the PPP rtnetlink handler support.

[PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[Guillaume Nault]

ppp_read() and ppp_poll() can be called concurrently with ppp_ioctl().
In this case, ppp_ioctl() might call ppp_ccp_closed(), which may update
ppp->flags while ppp_read() or ppp_poll() is reading it.
The update done by ppp_ccp_closed() isn't atomic due to the bit mask
operation ('ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)'), so concurrent
readers might get transient values.
Reading incorrect ppp->flags may disturb the 'ppp->flags & SC_LOOP_TRAFFIC'
test in ppp_read() and ppp_poll(), which in turn can lead to improper
decision on whether the PPP unit file is ready for reading or not.

Since ppp_ccp_closed() is protected by the Rx and Tx locks (with
ppp_lock()), taking the Rx lock is enough for ppp_read() and ppp_poll()
to guarantee that ppp_ccp_closed() won't update ppp->flags
concurrently.

The same reasoning applies to ppp->n_channels. The 'n_channels' field
can also be written to concurrently by ppp_ioctl() (through
ppp_connect_channel() or ppp_disconnect_channel()). These writes aren't
atomic (simple increment/decrement), but are protected by both the Rx
and Tx locks (like in the ppp->flags case). So holding the Rx lock
before reading ppp->n_channels also prevents concurrent writes.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---

This was patch #2 of the 'ppp: fix locking issues related to ppp_ioctl()'
series. I haven't kept the extra locking of ppp->flags in
ppp_ioctl(PPPIOCGFLAGS), which was added in the original series,
because the ppp_mutex lock ensures we can't enter the PPPIOCSFLAGS case
concurrently.
This is still quite theoretical issue as I've never observed the error
in practice.

 drivers/net/ppp/ppp_generic.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..e8a5936 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user 
*buf,
 * network traffic (demand mode).
 */
struct ppp *ppp = PF_TO_PPP(pf);
+
+   ppp_recv_lock(ppp);
if (ppp->n_channels == 0 &&
-   (ppp->flags & SC_LOOP_TRAFFIC) == 0)
+   (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
+   ppp_recv_unlock(ppp);
break;
+   }
+   ppp_recv_unlock(ppp);
}
ret = -EAGAIN;
if (file->f_flags & O_NONBLOCK)
@@ -532,9 +537,12 @@ static unsigned int ppp_poll(struct file *file, poll_table 
*wait)
else if (pf->kind == INTERFACE) {
/* see comment in ppp_read */
struct ppp *ppp = PF_TO_PPP(pf);
+
+   ppp_recv_lock(ppp);
if (ppp->n_channels == 0 &&
(ppp->flags & SC_LOOP_TRAFFIC) == 0)
mask |= POLLIN | POLLRDNORM;
+   ppp_recv_unlock(ppp);
}
 
return mask;
-- 
2.7.0

Re: [PATCH 2/2] ppp: implement rtnetlink device handling

[Guillaume Nault]

On Mon, Jan 25, 2016 at 12:09:34PM +0100, walter harms wrote:
> 
> 
> Am 23.12.2015 21:04, schrieb Guillaume Nault:
> > @@ -1012,7 +1017,24 @@ static int ppp_dev_configure(struct net *src_net, 
> > struct net_device *dev,
> > int indx;
> > int err;
> >  
> > -   file = conf->file;
> > +   if (conf->fd >= 0) {
> > +   file = fget(conf->fd);
> > +   if (file) {
> > +   if (file->f_op != _device_fops) {
> > +   fput(file);
> > +   return -EBADF;
> > +   }
> > +
> > +   /* Don't hold reference on file: ppp_release() is
> > +* responsible for safely freeing the associated
> > +* resources upon release. So file won't go away
> > +* from under us.
> > +*/
> > +   fput(file);
> > +   }
> > +   } else {
> > +   file = conf->file;
> > +   }
> > if (!file)
> > return -EBADF;
> 
> 
> I would write that a bid different to reduce indent
> und improve readability
> 
> (note: totaly untested just reviewing)
> 
> if (conf->fd < 0) {
>   file = conf->file;
>   if (!file)
>   return -EBADF;
> }
> else
> {
> file = fget(conf->fd);
> if (!file)
>   return -EBADF;
> 
Early return on fget() failure looks indeed simpler.

> fput(file);
> if (file->f_op != _device_fops) { 
>   return -EBADF;
>   }
> 
But this is wrong: we can't act on file after fput(). So we have to
place fput() after the test.

Thanks for your review.
--
To unsubscribe from this list: send the line "unsubscribe linux-ppp" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] pppoe: fix reference counting in PPPoE proxy

[Guillaume Nault]

Drop reference on the relay_po socket when __pppoe_xmit() succeeds.
This is already handled correctly in the error path.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
No 'Fixes' tag, since this issue seems to predate git's history.

 drivers/net/ppp/pppoe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index f3c6302..4ddae81 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -395,6 +395,8 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff 
*skb)
 
if (!__pppoe_xmit(sk_pppox(relay_po), skb))
goto abort_put;
+
+   sock_put(sk_pppox(relay_po));
} else {
if (sock_queue_rcv_skb(sk, skb))
goto abort_kfree;
-- 
2.7.0

Re: [PATCH net] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

On Fri, Mar 11, 2016 at 02:42:16PM -0500, David Miller wrote:
> From: Guillaume Nault <g.na...@alphalink.fr>
> Date: Tue, 8 Mar 2016 20:14:30 +0100
> 
> > Lock ppp_mutex and check that file->private_data is NULL before
> > executing any action in ppp_unattached_ioctl().
> > The test done by ppp_ioctl() can't be relied upon, because
> > file->private_data may have been updated meanwhile. In which case
> > ppp_unattached_ioctl() will override file->private_data and mess up
> > reference counters or loose pointer to previously allocated PPP unit.
> > 
> > In case the test fails, -ENOTTY is returned, just like if ppp_ioctl()
> > had rejected the ioctl in the first place.
> > 
> > Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
> 
> If this thing can disappear on us, then we need to make the entirety
> of ppp_ioctl() run with the mutex held to fix this properly.
> 
> Otherwise ->private_data could go NULL on us meanwhile as well.
> 
> We should hold the mutex, to stabilize the value of ->private_data.

Actually, only ppp_release() can reset ->private_data to NULL. Beyond
closing the file's last reference, the only way to trigger it is
to run the PPPIOCDETACH ioctl. But even then, ppp_release() isn't
called if the file has more than one reference.
So ->private_data should never go NULL from under another user.

As for setting ->private_data to non-NULL value, this is exclusively
handled by ppp_unattached_ioctl(). Since the ppp_mutex is held at the
beginning of the function, calls are serialised, but one may still
overwrite ->private_data and leak the memory previously pointed to.
By testing ->private_data with ppp_mutex held, this patch fixes this
issue, and ->private_data is now guaranteed to remain constant after
it's been set.

Testing ->private_data without lock in ppp_ioctl() before calling
ppp_unattached_ioctl() is fine, because either ->private_data is
not NULL and thus is stable, or it is and ppp_unattached_ioctl()
takes care of not overriding ->private_data, should its value get
modified before taking the mutex.


I considered moving ppp_mutex up to cover the entirety of ppp_ioctl()
too, but finally choosed to handle everything in ppp_unattached_ioctl()
because that's where the problem really stands.
ppp_ioctl() takes the mutex for historical reasons (semi-automatic BKL
removal) and there are several places where holding ppp_mutex seems
unnecessary (e.g. for PPPIOCDETACH). So I felt the right direction was
to move ppp_mutex further down rather than moving it up to cover the
entirety of ppp_ioctl().

In particular, with regard to adding rtnetlink handlers for PPP (which
is the objective that lead to those PPP fixes), holding ppp_mutex for
too long is a problem. An rtnetlink handler would run under protection
of the rtnl mutex, and would need to grab ppp_mutex too (unless we
don't associate the PPP unit fd to the net device in the .newlink
callback).
But currently the PPPIOCNEWUNIT ioctl holds ppp_mutex before taking the
rtnl mutex (in ppp_create_interface()). In this context moving
ppp_mutex up to ppp_ioctl() makes things more difficult because what's
required is, on the contrary, moving it further down so that it gets
held after the rtnl mutex.
However I'd agree that such consideration shouldn't come into play for
fixes on net. It weighted a bit in my decision to not push ppp_mutex
up though.

[PATCH net v2] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

Locking ppp_mutex must be done before dereferencing file->private_data,
otherwise it could be modified before ppp_unattached_ioctl() takes the
lock. This could lead ppp_unattached_ioctl() to override ->private_data,
thus leaking reference to the ppp_file previously pointed to.

v2: lock all ppp_ioctl() instead of just checking private_data in
ppp_unattached_ioctl(), to avoid ambiguous behaviour.

Fixes: f3ff8a4d80e8 ("ppp: push BKL down into the driver")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index d61da9ec..8c8eedb 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -575,7 +575,7 @@ static int get_filter(void __user *arg, struct sock_filter 
**p)
 
 static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-   struct ppp_file *pf = file->private_data;
+   struct ppp_file *pf;
struct ppp *ppp;
int err = -EFAULT, val, val2, i;
struct ppp_idle idle;
@@ -585,9 +585,14 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
void __user *argp = (void __user *)arg;
int __user *p = argp;
 
-   if (!pf)
-   return ppp_unattached_ioctl(current->nsproxy->net_ns,
-   pf, file, cmd, arg);
+   mutex_lock(_mutex);
+
+   pf = file->private_data;
+   if (!pf) {
+   err = ppp_unattached_ioctl(current->nsproxy->net_ns,
+  pf, file, cmd, arg);
+   goto out;
+   }
 
if (cmd == PPPIOCDETACH) {
/*
@@ -602,7 +607,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
 * this fd and reopening /dev/ppp.
 */
err = -EINVAL;
-   mutex_lock(_mutex);
if (pf->kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
rtnl_lock();
@@ -616,15 +620,13 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
} else
pr_warn("PPPIOCDETACH file->f_count=%ld\n",
atomic_long_read(>f_count));
-   mutex_unlock(_mutex);
-   return err;
+   goto out;
}
 
if (pf->kind == CHANNEL) {
struct channel *pch;
struct ppp_channel *chan;
 
-   mutex_lock(_mutex);
pch = PF_TO_CHANNEL(pf);
 
switch (cmd) {
@@ -646,17 +648,16 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
err = chan->ops->ioctl(chan, cmd, arg);
up_read(>chan_sem);
}
-   mutex_unlock(_mutex);
-   return err;
+   goto out;
}
 
if (pf->kind != INTERFACE) {
/* can't happen */
pr_err("PPP: not interface or channel??\n");
-   return -EINVAL;
+   err = -EINVAL;
+   goto out;
}
 
-   mutex_lock(_mutex);
ppp = PF_TO_PPP(pf);
switch (cmd) {
case PPPIOCSMRU:
@@ -831,7 +832,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
default:
err = -ENOTTY;
}
+
+out:
mutex_unlock(_mutex);
+
return err;
 }
 
@@ -844,7 +848,6 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
struct ppp_net *pn;
int __user *p = (int __user *)arg;
 
-   mutex_lock(_mutex);
switch (cmd) {
case PPPIOCNEWUNIT:
/* Create a new ppp unit */
@@ -894,7 +897,7 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
default:
err = -ENOTTY;
}
-   mutex_unlock(_mutex);
+
return err;
 }
 
-- 
2.7.0

Re: [RFC PATCH 0/6] ppp: add rtnetlink support

[Guillaume Nault]

.On Tue, Apr 05, 2016 at 08:27:45AM -0700, Stephen Hemminger wrote:
> On Tue, 5 Apr 2016 02:56:17 +0200
> Guillaume Nault <g.na...@alphalink.fr> wrote:
> 
> > The rtnetlink handlers implemented in this series are minimal, and can
> > only replace the PPPIOCNEWUNIT ioctl. The rest of PPP ioctls remains
> > necessary for any other operation on channels and units.
> > It is perfectly to possible to mix PPP devices created by rtnl
> > and by ioctl(PPPIOCNEWUNIT). Devices will behave in the same way,
> > except for a few specific cases (as detailed in patch #6).
> 
> What blocks PPP from being fully netlink (use attributes),
> 
I just didn't implement other netlink attributes because I wanted to
get the foundations validated first. Implementing PPP unit ioctls with
rtnetlink attributes shouldn't be a problem because there's a 1:1
mapping between units and netdevices. So we could have some kind of
feature parity (I'm not sure if all ioctls are worth a netlink
attribute though).

But there's the problem of getting the unit identifier of a PPP device.
If that device was created with kernel assigned name and index, then
the user space daemon has no ifindex or ifname for building an
RTM_GETLINK message. So the ability to retrieve the unit identifer with
rtnetlink wouldn't be enough to fully replace ioctls on unit.

If by "fully netlink", you also meant implementing a netlink
replacement for all supported ioctls, then that's going to be even
trickier. A genetlink API would probably need to be created for
handling generic operations on PPP channels. But that wouldn't be
enough since unknown ioctls on channels are passed to the
chan->ops->ioctl() callback. So netlink support would also have to be
added to the channel handlers (pptp, pppoatm, sync_ppp, irda...).

> and work with same API set independent of how device was created.
> Special cases are nuisance and source of bugs.
> 
It looks like handling rtnetlink messages in ioctl based PPP devices is
just a matter of assigning ->rtnl_link_ops in ppp_create_interface().
I'll consider that for v3.

> > I'm sending the series only as RFC this time, because there are a few
> > points I'm unsatisfied with.
> > 
> > First, I'm not fond of passing file descriptors as netlink attributes,
> > as done with IFLA_PPP_DEV_FD (which is filled with a /dev/ppp fd). But
> > given how PPP units work, we have to associate a /dev/ppp fd somehow.
> > 
> > More importantly, the locking constraints of PPP are quite problematic.
> > The rtnetlink handler has to associate the new PPP unit with the
> > /dev/ppp file descriptor passed as parameter. This requires holding the
> > ppp_mutex (see e8e56ffd9d29 "ppp: ensure file->private_data can't be
> > overridden"), while the rtnetlink callback is already protected by
> > rtnl_lock(). Since other parts of the module take these locks in
> > reverse order, most of this series deals with preparing the code for
> > inverting the dependency between rtnl_lock and ppp_mutex. Some more
> > work is needed on that part (see patch #4 for details), but I wanted
> > to be sure that approach it worth it before spending some more time on
> > it.
> 
> One other way to handle the locking is to use trylock. Yes it justs
> pushs the problem back to userspace, but that is how lock reordering was
> handled in sysfs.
>
If that's considered a valid approach, then I'll use it for v3. That'd
simplify things nicely.

Re: [RFC PATCH 5/6] ppp: define reusable device creation functions

[Guillaume Nault]

On Tue, Apr 05, 2016 at 08:28:32AM -0700, Stephen Hemminger wrote:
> On Tue, 5 Apr 2016 02:56:29 +0200
> Guillaume Nault <g.na...@alphalink.fr> wrote:
> 
> > Move PPP device initialisation and registration out of
> > ppp_create_interface().
> > This prepares code for device registration with rtnetlink.
> > 
> 
> Does PPP module autoload correctly based on the netlink attributes?
> 
Patch #6 has MODULE_ALIAS_RTNL_LINK("ppp"). This works fine for
auto-loading ppp_generic when creating a PPP device with rtnetlink.
Is there anything else required?

Re: [RFC PATCH 6/6] ppp: add rtnetlink device creation support

[Guillaume Nault]

On Tue, Apr 05, 2016 at 07:18:14PM +0200, walter harms wrote:
> 
> 
> Am 05.04.2016 02:56, schrieb Guillaume Nault:
> > @@ -1043,12 +1048,39 @@ static int ppp_dev_configure(struct net *src_net, 
> > struct net_device *dev,
> >  const struct ppp_config *conf)
> >  {
> > struct ppp *ppp = netdev_priv(dev);
> > +   struct file *file;
> > int indx;
> > +   int err;
> > +
> > +   if (conf->fd < 0) {
> > +   file = conf->file;
> > +   if (!file) {
> > +   err = -EBADF;
> > +   goto out;
> 
> why not just return -EBADF;
> 
> > +   }
> > +   } else {
> > +   file = fget(conf->fd);
> > +   if (!file) {
> > +   err = -EBADF;
> > +   goto out;
>   
> why not just return -EBADF;
> 
Just because the 'out' label is declared anyway and because this
centralises the return point. But I agree returning -EBADF directly
could be more readable. I don't have strong opinion.

Re: [RFC PATCH 6/6] ppp: add rtnetlink device creation support

[Guillaume Nault]

On Wed, Apr 06, 2016 at 10:02:56AM +0200, walter harms wrote:
> 
> 
> Am 05.04.2016 23:22, schrieb Guillaume Nault:
> > On Tue, Apr 05, 2016 at 07:18:14PM +0200, walter harms wrote:
> >>
> >>
> >> Am 05.04.2016 02:56, schrieb Guillaume Nault:
> >>> @@ -1043,12 +1048,39 @@ static int ppp_dev_configure(struct net *src_net, 
> >>> struct net_device *dev,
> >>>const struct ppp_config *conf)
> >>>  {
> >>>   struct ppp *ppp = netdev_priv(dev);
> >>> + struct file *file;
> >>>   int indx;
> >>> + int err;
> >>> +
> >>> + if (conf->fd < 0) {
> >>> + file = conf->file;
> >>> + if (!file) {
> >>> + err = -EBADF;
> >>> + goto out;
> >>
> >> why not just return -EBADF;
> >>
> >>> + }
> >>> + } else {
> >>> + file = fget(conf->fd);
> >>> + if (!file) {
> >>> + err = -EBADF;
> >>> + goto out;
> >>
> >> why not just return -EBADF;
> >>
> > Just because the 'out' label is declared anyway and because this
> > centralises the return point. But I agree returning -EBADF directly
> > could be more readable. I don't have strong opinion.
> 
> in this special case i would go for readable. People tend to miss these
> if
>  if
>   if
> constructs.
> 
Ok, I'll do that in v3.

Re: net/ppp: use-after-free in ppp_unregister_channel

[Guillaume Nault]

On Wed, Mar 16, 2016 at 11:14:42PM +0800, Baozeng Ding wrote:
> Dear all,
> I've got the following use-after-free report while running syzkaller
> fuzzer. Unfortunately no reproducer. It was found in the Linux kernel
> version(4.4, on commit 9638685e32af961943b679fcb72d4ddd458eb18f).
> 
> ==
> BUG: KASAN: use-after-free in ppp_unregister_channel+0x372/0x3a0 at
> addr 880064e217e0
> Read of size 8 by task syz-executor/11581
> =
> BUG net_namespace (Not tainted): kasan: bad access detected
> -
> 
> Disabling lock debugging due to kernel taint
> INFO: Allocated in copy_net_ns+0x6b/0x1a0 age=92569 cpu=3 pid=6906
> [<  none  >] ___slab_alloc+0x4c7/0x500 kernel/mm/slub.c:2440
> [<  none  >] __slab_alloc+0x4c/0x90 kernel/mm/slub.c:2469
> [< inline >] slab_alloc_node kernel/mm/slub.c:2532
> [< inline >] slab_alloc kernel/mm/slub.c:2574
> [<  none  >] kmem_cache_alloc+0x23a/0x2b0 kernel/mm/slub.c:2579
> [< inline >] kmem_cache_zalloc kernel/include/linux/slab.h:597
> [< inline >] net_alloc kernel/net/core/net_namespace.c:325
> [<  none  >] copy_net_ns+0x6b/0x1a0 
> kernel/net/core/net_namespace.c:360
> [<  none  >] create_new_namespaces+0x2f6/0x610 
> kernel/kernel/nsproxy.c:95
> [<  none  >] copy_namespaces+0x297/0x320 kernel/kernel/nsproxy.c:150
> [<  none  >] copy_process.part.35+0x1bf4/0x5760 
> kernel/kernel/fork.c:1451
> [< inline >] copy_process kernel/kernel/fork.c:1274
> [<  none  >] _do_fork+0x1bc/0xcb0 kernel/kernel/fork.c:1723
> [< inline >] SYSC_clone kernel/kernel/fork.c:1832
> [<  none  >] SyS_clone+0x37/0x50 kernel/kernel/fork.c:1826
> [<  none  >] entry_SYSCALL_64_fastpath+0x16/0x7a 
> kernel/arch/x86/entry/entry_64.S:185
> 
> INFO: Freed in net_drop_ns+0x67/0x80 age=575 cpu=2 pid=2631
> [<  none  >] __slab_free+0x1fc/0x320 kernel/mm/slub.c:2650
> [< inline >] slab_free kernel/mm/slub.c:2805
> [<  none  >] kmem_cache_free+0x2a0/0x330 kernel/mm/slub.c:2814
> [< inline >] net_free kernel/net/core/net_namespace.c:341
> [<  none  >] net_drop_ns+0x67/0x80 kernel/net/core/net_namespace.c:348
> [<  none  >] cleanup_net+0x4e5/0x600 
> kernel/net/core/net_namespace.c:448
> [<  none  >] process_one_work+0x794/0x1440 
> kernel/kernel/workqueue.c:2036
> [<  none  >] worker_thread+0xdb/0xfc0 kernel/kernel/workqueue.c:2170
> [<  none  >] kthread+0x23f/0x2d0 
> kernel/drivers/block/aoe/aoecmd.c:1303
> [<  none  >] ret_from_fork+0x3f/0x70 
> kernel/arch/x86/entry/entry_64.S:468
> INFO: Slab 0xea0001938800 objects=3 used=0 fp=0x880064e2
> flags=0x5fffc004080
> INFO: Object 0x880064e2 @offset=0 fp=0x880064e24200
> 
> CPU: 1 PID: 11581 Comm: syz-executor Tainted: GB   4.4.0+
> #5
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
>   8800662c7790 8292049d 88003e36a300
>  880064e2 880064e2 8800662c77c0 816f2054
>  88003e36a300 ea0001938800 880064e2 
> Call Trace:
>  [< inline >] __dump_stack kernel/lib/dump_stack.c:15
>  [] dump_stack+0x6f/0xa2 kernel/lib/dump_stack.c:50
>  [] print_trailer+0xf4/0x150 kernel/mm/slub.c:654
>  [] object_err+0x2f/0x40 kernel/mm/slub.c:661
>  [< inline >] print_address_description kernel/mm/kasan/report.c:138
>  [] kasan_report_error+0x215/0x530 
> kernel/mm/kasan/report.c:236
>  [< inline >] kasan_report kernel/mm/kasan/report.c:259
>  [] __asan_report_load8_noabort+0x3e/0x40 
> kernel/mm/kasan/report.c:280
>  [< inline >] ? ppp_pernet kernel/include/linux/compiler.h:218
>  [] ? ppp_unregister_channel+0x372/0x3a0 
> kernel/drivers/net/ppp/ppp_generic.c:2392
>  [< inline >] ppp_pernet kernel/include/linux/compiler.h:218
>  [] ppp_unregister_channel+0x372/0x3a0 
> kernel/drivers/net/ppp/ppp_generic.c:2392
>  [< inline >] ? ppp_pernet kernel/drivers/net/ppp/ppp_generic.c:293
>  [] ? ppp_unregister_channel+0xe6/0x3a0 
> kernel/drivers/net/ppp/ppp_generic.c:2392
>  [] ppp_asynctty_close+0xa3/0x130 
> kernel/drivers/net/ppp/ppp_async.c:241
>  [] ? async_lcp_peek+0x5b0/0x5b0 
> kernel/drivers/net/ppp/ppp_async.c:1000
>  [] tty_ldisc_close.isra.1+0x99/0xe0 
> kernel/drivers/tty/tty_ldisc.c:478
>  [] tty_ldisc_kill+0x40/0x170 
> kernel/drivers/tty/tty_ldisc.c:744
>  [] tty_ldisc_release+0x1b3/0x260 
> kernel/drivers/tty/tty_ldisc.c:772
>  [] tty_release+0xac1/0x13e0 
> kernel/drivers/tty/tty_io.c:1901
>  [] ? release_tty+0x320/0x320 
> kernel/drivers/tty/tty_io.c:1688
>  [] __fput+0x236/0x780 kernel/fs/file_table.c:208
>  [] 

[PATCH net] ppp: take reference on channels netns

[Guillaume Nault]

/fs/file_table.c:244
 [] task_work_run+0x16b/0x200 kernel/kernel/task_work.c:115
 [< inline >] exit_task_work kernel/include/linux/task_work.h:21
 [] do_exit+0x8b5/0x2c60 kernel/kernel/exit.c:750
 [] ? debug_check_no_locks_freed+0x290/0x290 
kernel/kernel/locking/lockdep.c:4123
 [] ? mm_update_next_owner+0x6f0/0x6f0 
kernel/kernel/exit.c:357
 [] ? __dequeue_signal+0x136/0x470 kernel/kernel/signal.c:550
 [] ? recalc_sigpending_tsk+0x13b/0x180 
kernel/kernel/signal.c:145
 [] do_group_exit+0x108/0x330 kernel/kernel/exit.c:880
 [] get_signal+0x5e4/0x14f0 kernel/kernel/signal.c:2307
 [< inline >] ? kretprobe_table_lock kernel/kernel/kprobes.c:1113
 [] ? kprobe_flush_task+0xb5/0x450 
kernel/kernel/kprobes.c:1158
 [] do_signal+0x83/0x1c90 kernel/arch/x86/kernel/signal.c:712
 [] ? recycle_rp_inst+0x310/0x310 
kernel/include/linux/list.h:655
 [] ? setup_sigcontext+0x780/0x780 
kernel/arch/x86/kernel/signal.c:165
 [] ? finish_task_switch+0x424/0x5f0 
kernel/kernel/sched/core.c:2692
 [< inline >] ? finish_lock_switch kernel/kernel/sched/sched.h:1099
 [] ? finish_task_switch+0x120/0x5f0 
kernel/kernel/sched/core.c:2678
 [< inline >] ? context_switch kernel/kernel/sched/core.c:2807
 [] ? __schedule+0x919/0x1bd0 kernel/kernel/sched/core.c:3283
 [] exit_to_usermode_loop+0xf1/0x1a0 
kernel/arch/x86/entry/common.c:247
 [< inline >] prepare_exit_to_usermode 
kernel/arch/x86/entry/common.c:282
 [] syscall_return_slowpath+0x19f/0x210 
kernel/arch/x86/entry/common.c:344
 [] int_ret_from_sys_call+0x25/0x9f 
kernel/arch/x86/entry/entry_64.S:281
Memory state around the buggy address:
 880064e21680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 880064e21700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>880064e21780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
   ^
 880064e21800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 880064e21880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==

Fixes: 273ec51dd7ce ("net: ppp_generic - introduce net-namespace functionality 
v2")
Reported-by: Baozeng Ding <splovi...@gmail.com>
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4fd8610..f572b31 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2307,7 +2307,7 @@ int ppp_register_net_channel(struct net *net, struct 
ppp_channel *chan)
 
pch->ppp = NULL;
pch->chan = chan;
-   pch->chan_net = net;
+   pch->chan_net = get_net(net);
chan->ppp = pch;
init_ppp_file(>file, CHANNEL);
pch->file.hdrlen = chan->hdrlen;
@@ -2404,6 +2404,8 @@ ppp_unregister_channel(struct ppp_channel *chan)
spin_lock_bh(>all_channels_lock);
list_del(>list);
spin_unlock_bh(>all_channels_lock);
+   put_net(pch->chan_net);
+   pch->chan_net = NULL;
 
pch->file.dead = 1;
wake_up_interruptible(>file.rwait);
-- 
2.8.0.rc3

[PATCH net] ppp: release rtnl mutex when interface creation fails

[Guillaume Nault]

Add missing rtnl_unlock() in the error path of ppp_create_interface().

Fixes: 58a89ecaca53 ("ppp: fix lockdep splat in ppp_dev_uninit()")
Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index e8a5936..d61da9ec 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2816,6 +2816,7 @@ static struct ppp *ppp_create_interface(struct net *net, 
int unit,
 
 out2:
mutex_unlock(>all_ppp_mutex);
+   rtnl_unlock();
free_netdev(dev);
 out1:
*retp = ret;
-- 
2.7.0

[PATCH net] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

Lock ppp_mutex and check that file->private_data is NULL before
executing any action in ppp_unattached_ioctl().
The test done by ppp_ioctl() can't be relied upon, because
file->private_data may have been updated meanwhile. In which case
ppp_unattached_ioctl() will override file->private_data and mess up
reference counters or loose pointer to previously allocated PPP unit.

In case the test fails, -ENOTTY is returned, just like if ppp_ioctl()
had rejected the ioctl in the first place.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
This seems to originate in BKL removal :
f3ff8a4d80e8 ("ppp: push BKL down into the driver") moved the test on
file->private_data (in ppp_ioctl()) out of BKL protection. BKL was then
replaced by ppp_mutex in 15fd0cd9a2ad ("net: autoconvert trivial BKL
users to private mutex").

 drivers/net/ppp/ppp_generic.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index d61da9ec..d1dbcb6 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -845,6 +845,11 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
int __user *p = (int __user *)arg;
 
mutex_lock(_mutex);
+   if (file->private_data) {
+   mutex_unlock(_mutex);
+   return -ENOTTY;
+   }
+
switch (cmd) {
case PPPIOCNEWUNIT:
/* Create a new ppp unit */
-- 
2.7.0

[RFC PATCH 6/6] ppp: add rtnetlink device creation support

[Guillaume Nault]

Define PPP device handlers for use with rtnetlink.
The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
contains the file descriptor of the associated /dev/ppp instance (the
file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
the ioctl-based API). The PPP device is removed when this file
descriptor is released (same behaviour as with ioctl based PPP
devices).

PPP devices created with the rtnetlink API behave like the ones created
with ioctl(PPPIOCNEWUNIT). In particular existing ioctls work the same
way, no matter how the PPP device was created.

However, there are a few differences between rtnl and ioctl based PPP
devices. Rtnl based PPP devices can be removed with RTM_DELLINK
messages (e.g. with "ip link del"), while the ones created with
ioctl(PPPIOCNEWUNIT) can't.
The interface name is also built differently: the number following the
"ppp" prefix corresponds to the PPP unit number for ioctl based
devices, while it is just an unrelated incrementing index for rtnl
ones.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 143 +-
 include/uapi/linux/if_link.h  |   8 +++
 2 files changed, 136 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 516f8dc..ae40368 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -46,6 +46,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -185,7 +186,9 @@ struct channel {
 
 struct ppp_config {
struct file *file;
+   s32 fd;
s32 unit;
+   bool ifname_is_set;
 };
 
 /*
@@ -286,6 +289,7 @@ static int unit_get(struct idr *p, void *ptr);
 static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
+static void ppp_setup(struct net_device *dev);
 
 static const struct net_device_ops ppp_netdev_ops;
 
@@ -989,7 +993,7 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
-static int ppp_unit_register(struct ppp *ppp, int unit)
+static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
 {
struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
int ret;
@@ -1019,7 +1023,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
}
ppp->file.index = ret;
 
-   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+   if (!ifname_is_set)
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
 
ret = register_netdevice(ppp->dev);
if (ret < 0)
@@ -1043,12 +1048,39 @@ static int ppp_dev_configure(struct net *src_net, 
struct net_device *dev,
 const struct ppp_config *conf)
 {
struct ppp *ppp = netdev_priv(dev);
+   struct file *file;
int indx;
+   int err;
+
+   if (conf->fd < 0) {
+   file = conf->file;
+   if (!file) {
+   err = -EBADF;
+   goto out;
+   }
+   } else {
+   file = fget(conf->fd);
+   if (!file) {
+   err = -EBADF;
+   goto out;
+   }
+
+   if (file->f_op != _device_fops) {
+   err = -EBADF;
+   goto out;
+   }
+   }
+
+   mutex_lock(_mutex);
+   if (file->private_data) {
+   err = -ENOTTY;
+   goto out_mutex;
+   }
 
ppp->dev = dev;
ppp->mru = PPP_MRU;
ppp->ppp_net = src_net;
-   ppp->owner = conf->file;
+   ppp->owner = file;
 
init_ppp_file(>file, INTERFACE);
ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
@@ -1067,9 +1099,88 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
ppp->active_filter = NULL;
 #endif /* CONFIG_PPP_FILTER */
 
-   return ppp_unit_register(ppp, conf->unit);
+   err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
+   if (err < 0)
+   goto out_mutex;
+
+   file->private_data = >file;
+
+out_mutex:
+   mutex_unlock(_mutex);
+out:
+   if (conf->fd >= 0 && file)
+   fput(file);
+
+   return err;
 }
 
+static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
+   [IFLA_PPP_DEV_FD]   = { .type = NLA_S32 },
+};
+
+static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+   if (!data)
+   return -EINVAL;
+
+   if (!data[IFLA_PPP_DEV_FD])
+   return -EINVAL;
+   if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
+   return -EBADF;
+
+   return 0;
+}
+
+static int pp

[RFC PATCH 1/6] ppp: simplify usage of ppp_create_interface()

[Guillaume Nault]

Set file->private_data inside ppp_create_interface() to avoid the need
for returning the new ppp structure to the caller.
Also, make the unit parameter read/write so that caller can still
retrieve the PPP unit number assigned to the interface.

Avoiding setting ->private_data in the caller will allow for pushing
ppp_mutex down when handling the PPPIOCNEWUNIT ioctl (as locking
ppp_mutex is required before setting ->private_data).

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 47 +--
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f572b31..ec83b83 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -269,8 +269,7 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit,
-   struct file *file, int *retp);
+static int ppp_create_interface(struct net *net, struct file *file, int *unit);
 static void init_ppp_file(struct ppp_file *pf, int kind);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
@@ -853,12 +852,13 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, file, );
-   if (!ppp)
+
+   err = ppp_create_interface(net, file, );
+   if (err < 0)
break;
-   file->private_data = >file;
+
err = -EFAULT;
-   if (put_user(ppp->file.index, p))
+   if (put_user(unit, p))
break;
err = 0;
break;
@@ -2732,8 +2732,7 @@ ppp_get_stats(struct ppp *ppp, struct ppp_stats *st)
  * or if there is already a unit with the requested number.
  * unit == -1 means allocate a new number.
  */
-static struct ppp *ppp_create_interface(struct net *net, int unit,
-   struct file *file, int *retp)
+static int ppp_create_interface(struct net *net, struct file *file, int *unit)
 {
struct ppp *ppp;
struct ppp_net *pn;
@@ -2776,15 +2775,13 @@ static struct ppp *ppp_create_interface(struct net 
*net, int unit,
rtnl_lock();
mutex_lock(>all_ppp_mutex);
 
-   if (unit < 0) {
-   unit = unit_get(>units_idr, ppp);
-   if (unit < 0) {
-   ret = unit;
+   if (*unit < 0) {
+   ret = unit_get(>units_idr, ppp);
+   if (ret < 0)
goto out2;
-   }
} else {
ret = -EEXIST;
-   if (unit_find(>units_idr, unit))
+   if (unit_find(>units_idr, *unit))
goto out2; /* unit already exists */
/*
 * if caller need a specified unit number
@@ -2795,39 +2792,41 @@ static struct ppp *ppp_create_interface(struct net 
*net, int unit,
 * fair but at least pppd will ask us to allocate
 * new unit in this case so user is happy :)
 */
-   unit = unit_set(>units_idr, ppp, unit);
-   if (unit < 0)
+   ret = unit_set(>units_idr, ppp, *unit);
+   if (ret < 0) {
+   ret = -EEXIST;
goto out2;
+   }
}
 
/* Initialize the new ppp unit */
-   ppp->file.index = unit;
-   sprintf(dev->name, "ppp%d", unit);
+   ppp->file.index = ret;
+   sprintf(dev->name, "ppp%d", ret);
 
ret = register_netdevice(dev);
if (ret != 0) {
-   unit_put(>units_idr, unit);
+   unit_put(>units_idr, ppp->file.index);
netdev_err(ppp->dev, "PPP: couldn't register device %s (%d)\n",
   dev->name, ret);
goto out2;
}
 
ppp->ppp_net = net;
-
+   file->private_data = >file;
+   *unit = ppp->file.index;
atomic_inc(_unit_count);
+
mutex_unlock(>all_ppp_mutex);
rtnl_unlock();
 
-   *retp = 0;
-   return ppp;
+   return 0;
 
 out2:
mutex_unlock(>all_ppp_mutex);
rtnl_unlock();
free_netdev(dev);
 out1:
-   *retp = ret;
-   return NULL;
+   return ret;
 }
 
 /*
-- 
2.8.0.rc3

[RFC PATCH 3/6] ppp: don't lock ppp_mutex while handling PPPIOCDETACH

[Guillaume Nault]

Once set, file->private_data remains constant. So it's safe to access
it without holding ppp_mutex.
The PPP unit fields accessed while handling PPPIOCDETACH (pf->kind and
ppp->owner) are also constant and have been set before
file->private_data got assigned. So these too can be read without
holding ppp_mutex.
Finally, ppp_release() is called only if we're the only user of the
unit.

Therefore, we can avoid locking ppp_mutex completely for handling
PPPIOCDETACH. This removes locking dependency between ppp_mutex and
rtnl_mutex, which will allow holding ppp_mutex from an rtnetlink
context.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 7329c72..c81e257 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -592,15 +592,11 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
cmd, arg);
}
 
-   mutex_lock(_mutex);
-
-   pf = file->private_data;
-   if (!pf) {
-   err = -ENOTTY;
-   goto out;
-   }
-
if (cmd == PPPIOCDETACH) {
+   pf = file->private_data;
+   if (!pf)
+   return -ENOTTY;
+
/*
 * We have to be careful here... if the file descriptor
 * has been dup'd, we could have another process in the
@@ -626,6 +622,15 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
} else
pr_warn("PPPIOCDETACH file->f_count=%ld\n",
atomic_long_read(>f_count));
+
+   return err;
+   }
+
+   mutex_lock(_mutex);
+
+   pf = file->private_data;
+   if (!pf) {
+   err = -ENOTTY;
goto out;
}
 
-- 
2.8.0.rc3

[RFC PATCH 4/6] ppp: invert lock ordering between ppp_mutex and rtnl_lock

[Guillaume Nault]

Since ppp_create_interface() is the only place[1] holding both ppp_mutex
and rtnl_lock(), we can reverse lock ordering to allow taking ppp_mutex
when rtnl_lock is already held.
This will be necessary for creating PPP interfaces from an rtnetlink
callback.

[1]: There may actually be exceptions. When operating on PPP channels,
ppp_ioctl() can call lower level ioctl callbacks (chan->ops->ioctl())
with ppp_mutex locked. More review would be necessary to ensure no such
callback would ever call rtnl_lock(). OTOH, with proper locking inside
the ->ioctl() callbacks, ppp_mutex might be removed entirely from this
part of the code.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index c81e257..8aaedb8 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2796,8 +2796,8 @@ static int ppp_create_interface(struct net *net, struct 
file *file, int *unit)
 */
dev_net_set(dev, net);
 
-   mutex_lock(_mutex);
rtnl_lock();
+   mutex_lock(_mutex);
mutex_lock(>all_ppp_mutex);
 
if (file->private_data) {
@@ -2847,15 +2847,15 @@ static int ppp_create_interface(struct net *net, struct 
file *file, int *unit)
atomic_inc(_unit_count);
 
mutex_unlock(>all_ppp_mutex);
-   rtnl_unlock();
mutex_unlock(_mutex);
+   rtnl_unlock();
 
return 0;
 
 out2:
mutex_unlock(>all_ppp_mutex);
-   rtnl_unlock();
mutex_unlock(_mutex);
+   rtnl_unlock();
free_netdev(dev);
 out1:
return ret;
-- 
2.8.0.rc3

[RFC PATCH 2/6] ppp: don't hold ppp_mutex before calling ppp_unattached_ioctl()

[Guillaume Nault]

Let ppp_unattached_ioctl() lock ppp_mutex only when needed.
For PPPIOCATTACH and PPPIOCATTCHAN, we just need to lock ppp_mutex
before all_ppp_mutex/all_channels_lock (to keep lock ordering) and to
take care of the -ENOTTY error code when file->private_data is not
NULL.
For PPPIOCNEWUNIT, ppp_mutex is pushed further down in
ppp_create_interface() and is held right before rtnl_lock. The goal is
to eventually lock ppp_mutex after rtnl_lock, so that PPP device
creation can be done inside a rtnetlink function handler.

While there, remove unused ppp_file argument from ppp_unattached_ioctl()
prototype.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 51 +--
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index ec83b83..7329c72 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -245,8 +245,8 @@ struct ppp_net {
 #define seq_after(a, b)((s32)((a) - (b)) > 0)
 
 /* Prototypes. */
-static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
-   struct file *file, unsigned int cmd, unsigned long arg);
+static int ppp_unattached_ioctl(struct net *net, struct file *file,
+   unsigned int cmd, unsigned long arg);
 static void ppp_xmit_process(struct ppp *ppp);
 static void ppp_send_frame(struct ppp *ppp, struct sk_buff *skb);
 static void ppp_push(struct ppp *ppp);
@@ -584,12 +584,19 @@ static long ppp_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
void __user *argp = (void __user *)arg;
int __user *p = argp;
 
+   switch (cmd) {
+   case PPPIOCNEWUNIT:
+   case PPPIOCATTACH:
+   case PPPIOCATTCHAN:
+   return ppp_unattached_ioctl(current->nsproxy->net_ns, file,
+   cmd, arg);
+   }
+
mutex_lock(_mutex);
 
pf = file->private_data;
if (!pf) {
-   err = ppp_unattached_ioctl(current->nsproxy->net_ns,
-  pf, file, cmd, arg);
+   err = -ENOTTY;
goto out;
}
 
@@ -838,8 +845,8 @@ out:
return err;
 }
 
-static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
-   struct file *file, unsigned int cmd, unsigned long arg)
+static int ppp_unattached_ioctl(struct net *net, struct file *file,
+   unsigned int cmd, unsigned long arg)
 {
int unit, err = -EFAULT;
struct ppp *ppp;
@@ -867,31 +874,43 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Attach to an existing ppp unit */
if (get_user(unit, p))
break;
+
err = -ENXIO;
pn = ppp_pernet(net);
+   mutex_lock(_mutex);
mutex_lock(>all_ppp_mutex);
ppp = ppp_find_unit(pn, unit);
if (ppp) {
-   atomic_inc(>file.refcnt);
-   file->private_data = >file;
-   err = 0;
+   err = -ENOTTY;
+   if (!file->private_data) {
+   atomic_inc(>file.refcnt);
+   file->private_data = >file;
+   err = 0;
+   }
}
mutex_unlock(>all_ppp_mutex);
+   mutex_unlock(_mutex);
break;
 
case PPPIOCATTCHAN:
if (get_user(unit, p))
break;
+
err = -ENXIO;
pn = ppp_pernet(net);
+   mutex_lock(_mutex);
spin_lock_bh(>all_channels_lock);
chan = ppp_find_channel(pn, unit);
if (chan) {
-   atomic_inc(>file.refcnt);
-   file->private_data = >file;
-   err = 0;
+   err = -ENOTTY;
+   if (!file->private_data) {
+   atomic_inc(>file.refcnt);
+   file->private_data = >file;
+   err = 0;
+   }
}
spin_unlock_bh(>all_channels_lock);
+   mutex_unlock(_mutex);
break;
 
default:
@@ -2772,9 +2791,15 @@ static int ppp_create_interface(struct net *net, struct 
file *file, int *unit)
 */
dev_net_set(dev, net);
 
+   mutex_lock(_mutex);
rtnl_lock();
mutex_lock(>all_ppp_mutex);
 
+   if (file->private_data) {
+   ret = -ENOTTY;
+   goto out2;
+   }
+
if (*unit < 0) {
re

[RFC PATCH 5/6] ppp: define reusable device creation functions

[Guillaume Nault]

Move PPP device initialisation and registration out of
ppp_create_interface().
This prepares code for device registration with rtnetlink.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 185 --
 1 file changed, 106 insertions(+), 79 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 8aaedb8..516f8dc 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -183,6 +183,11 @@ struct channel {
 #endif /* CONFIG_PPP_MULTILINK */
 };
 
+struct ppp_config {
+   struct file *file;
+   s32 unit;
+};
+
 /*
  * SMP locking issues:
  * Both the ppp.rlock and ppp.wlock locks protect the ppp.channels
@@ -984,6 +989,87 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
+static int ppp_unit_register(struct ppp *ppp, int unit)
+{
+   struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
+   int ret;
+
+   mutex_lock(>all_ppp_mutex);
+
+   if (unit < 0) {
+   ret = unit_get(>units_idr, ppp);
+   if (ret < 0)
+   goto err;
+   } else {
+   /* Caller asked for a specific unit number. Fail with -EEXIST
+* if unavailable. For backward compatibility, return -EEXIST
+* too if idr allocation fails; this makes pppd retry without
+* requesting a specific unit number.
+*/
+   if (unit_find(>units_idr, unit)) {
+   ret = -EEXIST;
+   goto err;
+   }
+   ret = unit_set(>units_idr, ppp, unit);
+   if (ret < 0) {
+   /* Rewrite error for backward compatibility */
+   ret = -EEXIST;
+   goto err;
+   }
+   }
+   ppp->file.index = ret;
+
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+
+   ret = register_netdevice(ppp->dev);
+   if (ret < 0)
+   goto err_unit;
+
+   atomic_inc(_unit_count);
+
+   mutex_unlock(>all_ppp_mutex);
+
+   return 0;
+
+err_unit:
+   unit_put(>units_idr, ppp->file.index);
+err:
+   mutex_unlock(>all_ppp_mutex);
+
+   return ret;
+}
+
+static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
+const struct ppp_config *conf)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   int indx;
+
+   ppp->dev = dev;
+   ppp->mru = PPP_MRU;
+   ppp->ppp_net = src_net;
+   ppp->owner = conf->file;
+
+   init_ppp_file(>file, INTERFACE);
+   ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
+
+   for (indx = 0; indx < NUM_NP; ++indx)
+   ppp->npmode[indx] = NPMODE_PASS;
+   INIT_LIST_HEAD(>channels);
+   spin_lock_init(>rlock);
+   spin_lock_init(>wlock);
+#ifdef CONFIG_PPP_MULTILINK
+   ppp->minseq = -1;
+   skb_queue_head_init(>mrq);
+#endif /* CONFIG_PPP_MULTILINK */
+#ifdef CONFIG_PPP_FILTER
+   ppp->pass_filter = NULL;
+   ppp->active_filter = NULL;
+#endif /* CONFIG_PPP_FILTER */
+
+   return ppp_unit_register(ppp, conf->unit);
+}
+
 #define PPP_MAJOR  108
 
 /* Called at boot time if ppp is compiled into the kernel,
@@ -2758,107 +2844,48 @@ ppp_get_stats(struct ppp *ppp, struct ppp_stats *st)
  */
 static int ppp_create_interface(struct net *net, struct file *file, int *unit)
 {
+   struct ppp_config conf = {
+   .file = file,
+   .unit = *unit,
+   };
+   struct net_device *dev;
struct ppp *ppp;
-   struct ppp_net *pn;
-   struct net_device *dev = NULL;
-   int ret = -ENOMEM;
-   int i;
+   int err;
 
dev = alloc_netdev(sizeof(struct ppp), "", NET_NAME_ENUM, ppp_setup);
-   if (!dev)
-   goto out1;
-
-   pn = ppp_pernet(net);
-
-   ppp = netdev_priv(dev);
-   ppp->dev = dev;
-   ppp->mru = PPP_MRU;
-   init_ppp_file(>file, INTERFACE);
-   ppp->file.hdrlen = PPP_HDRLEN - 2;  /* don't count proto bytes */
-   ppp->owner = file;
-   for (i = 0; i < NUM_NP; ++i)
-   ppp->npmode[i] = NPMODE_PASS;
-   INIT_LIST_HEAD(>channels);
-   spin_lock_init(>rlock);
-   spin_lock_init(>wlock);
-#ifdef CONFIG_PPP_MULTILINK
-   ppp->minseq = -1;
-   skb_queue_head_init(>mrq);
-#endif /* CONFIG_PPP_MULTILINK */
-#ifdef CONFIG_PPP_FILTER
-   ppp->pass_filter = NULL;
-   ppp->active_filter = NULL;
-#endif /* CONFIG_PPP_FILTER */
+   if (!dev) {
+   err = -ENOMEM;
+   goto err;
+   }
 
-   /*
-* drum roll: don't forget to set
-* the net device is

[RFC PATCH 0/6] ppp: add rtnetlink support

[Guillaume Nault]

PPP devices lack the ability to be customised at creation time. In
particular they can't be created in a given netns or with a particular
name. Moving or renaming the device after creation is possible, but
creates undesirable transient effects on servers where PPP devices are
constantly created and removed, as users connect and disconnect.
Implementing rtnetlink support solves this problem.

The rtnetlink handlers implemented in this series are minimal, and can
only replace the PPPIOCNEWUNIT ioctl. The rest of PPP ioctls remains
necessary for any other operation on channels and units.
It is perfectly to possible to mix PPP devices created by rtnl
and by ioctl(PPPIOCNEWUNIT). Devices will behave in the same way,
except for a few specific cases (as detailed in patch #6).


I'm sending the series only as RFC this time, because there are a few
points I'm unsatisfied with.

First, I'm not fond of passing file descriptors as netlink attributes,
as done with IFLA_PPP_DEV_FD (which is filled with a /dev/ppp fd). But
given how PPP units work, we have to associate a /dev/ppp fd somehow.

More importantly, the locking constraints of PPP are quite problematic.
The rtnetlink handler has to associate the new PPP unit with the
/dev/ppp file descriptor passed as parameter. This requires holding the
ppp_mutex (see e8e56ffd9d29 "ppp: ensure file->private_data can't be
overridden"), while the rtnetlink callback is already protected by
rtnl_lock(). Since other parts of the module take these locks in
reverse order, most of this series deals with preparing the code for
inverting the dependency between rtnl_lock and ppp_mutex. Some more
work is needed on that part (see patch #4 for details), but I wanted
to be sure that approach it worth it before spending some more time on
it.


Other approach

I've considered another approach where no /dev/ppp file descriptor
is associated to the PPP unit at creation time. This removes all the
problems described above. The PPP interface that is created behaves
mostly like a dummy device until it gets associated with a /dev/ppp
file descriptor (using the PPPIOCATTACH ioctl).
The problem here is that, AFAIK, we can't return the unit identifier of
the new PPP device to the user space program having issued the
RTM_NEWLINK message. This identifier is required for the
ioctl(PPPIOCATTACH) call. Of course we could return such information
in an RTM_GETLINK message, but the user would need to query the device
name that was created. This would only work for users that can set the
IFLA_IFNAME attribute in their original RTM_NEWLINK message.


Patch series

Patches 1 to 3 prepare the code for inverting lock ordering between
ppp_mutex and rtnl_lock. Patch #4 does the lock inversion.
The actual infrastructure is implemented in patches #5 and #6.


Changes since v1:
  - Rebase on net-next.
  - Invert locking order wrt. ppp_mutex and rtnl_lock and protect
file->private_data with ppp_mutex.


Guillaume Nault (6):
  ppp: simplify usage of ppp_create_interface()
  ppp: don't hold ppp_mutex before calling ppp_unattached_ioctl()
  ppp: don't lock ppp_mutex while handling PPPIOCDETACH
  ppp: invert lock ordering between ppp_mutex and rtnl_lock
  ppp: define reusable device creation functions
  ppp: add rtnetlink device creation support

 drivers/net/ppp/ppp_generic.c | 385 ++
 include/uapi/linux/if_link.h  |   8 +
 2 files changed, 286 insertions(+), 107 deletions(-)

-- 
2.8.0.rc3

[RFC PATCH v3 0/2] ppp: add rtnetlink support

[Guillaume Nault]

PPP devices lack the ability to be customised at creation time. In
particular they can't be created in a given netns or with a particular
name. Moving or renaming the device after creation is possible, but
creates undesirable transient effects on servers where PPP devices are
constantly created and removed, as users connect and disconnect.
Implementing rtnetlink support solves this problem.

The rtnetlink handlers implemented in this series are minimal, and can
only replace the PPPIOCNEWUNIT ioctl. The rest of PPP ioctls remains
necessary for any other operation on channels and units.
It is perfectly possible to mix PPP devices created by rtnl
and by ioctl(PPPIOCNEWUNIT). Devices will behave in the same way.

If necessary, rtnetlink support could be extended to provide some of
the functionalities brought by ppp_net_ioctl() and ppp_ioctl(). This
would let external programs, like "ip link", set or retrieve PPP device
information. However, I haven't made my mind on the usefulness of this
approach, so this isn't implemented in this series.


This series doesn't try to invert lock ordering between ppp_mutex and
rtnl_lock. mutex_trylock() is used instead, which greatly simplifies
things.
A user visible difference brought by this series is that old PPP
interfaces (those created with ioctl(PPPIOCNEWUNIT)), can now be
removed by "ip link del", just like new rtnl based PPP devices.


Changes since v2:
  - Define ->rtnl_link_ops for ioctl based PPP devices, so they can
handle rtnl messages just like rtnl based ones (suggested by
Stephen Hemminger).
  - Move back to original lock ordering between ppp_mutex and rtnl_lock
to simplify patch series. Handle lock inversion issue using
mutex_trylock() (suggested by Stephen Hemminger).
  - Do file descriptor lookup directly in ppp_nl_newlink(), to simplify
ppp_dev_configure().

Changes since v1:
  - Rebase on net-next.
  - Invert locking order wrt. ppp_mutex and rtnl_lock and protect
file->private_data with ppp_mutex.


Guillaume Nault (2):
  ppp: define reusable device creation functions
  ppp: add rtnetlink device creation support

 drivers/net/ppp/ppp_generic.c | 315 ++
 include/uapi/linux/if_link.h  |   8 ++
 2 files changed, 235 insertions(+), 88 deletions(-)

-- 
2.8.1

[RFC PATCH v3 2/2] ppp: add rtnetlink device creation support

[Guillaume Nault]

Define PPP device handler for use with rtnetlink.
The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
contains the file descriptor of the associated /dev/ppp instance (the
file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
the ioctl-based API). The PPP device is removed when this file
descriptor is released (same behaviour as with ioctl based PPP
devices).

PPP devices created with the rtnetlink API behave like the ones created
with ioctl(PPPIOCNEWUNIT). In particular existing ioctls work the same
way, no matter how the PPP device was created.
The rtnl callbacks are also assigned to ioctl based PPP devices. This
way, rtnl messages have the same effect on any PPP devices.
The immediate effect is that all PPP devices, even ioctl-based
ones, can now be removed with "ip link del".

A minor difference still exists between ioctl and rtnl based PPP
interfaces: in the device name, the number following the "ppp" prefix
corresponds to the PPP unit number for ioctl based devices, while it is
just an unrelated incrementing index for rtnl ones.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 115 --
 include/uapi/linux/if_link.h  |   8 +++
 2 files changed, 120 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f581a77..8b3db3a 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -46,6 +46,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -186,6 +187,7 @@ struct channel {
 struct ppp_config {
struct file *file;
s32 unit;
+   bool ifname_is_set;
 };
 
 /*
@@ -286,6 +288,7 @@ static int unit_get(struct idr *p, void *ptr);
 static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
+static void ppp_setup(struct net_device *dev);
 
 static const struct net_device_ops ppp_netdev_ops;
 
@@ -964,7 +967,7 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
-static int ppp_unit_register(struct ppp *ppp, int unit)
+static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
 {
struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
int ret;
@@ -994,7 +997,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
}
ppp->file.index = ret;
 
-   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+   if (!ifname_is_set)
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
 
ret = register_netdevice(ppp->dev);
if (ret < 0)
@@ -1043,7 +1047,7 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
ppp->active_filter = NULL;
 #endif /* CONFIG_PPP_FILTER */
 
-   err = ppp_unit_register(ppp, conf->unit);
+   err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
if (err < 0)
return err;
 
@@ -1052,6 +1056,99 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
return 0;
 }
 
+static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
+   [IFLA_PPP_DEV_FD]   = { .type = NLA_S32 },
+};
+
+static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+   if (!data)
+   return -EINVAL;
+
+   if (!data[IFLA_PPP_DEV_FD])
+   return -EINVAL;
+   if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
+   return -EBADF;
+
+   return 0;
+}
+
+static int ppp_nl_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+   struct ppp_config conf = {
+   .unit = -1,
+   .ifname_is_set = true,
+   };
+   struct file *file;
+   int err;
+
+   file = fget(nla_get_s32(data[IFLA_PPP_DEV_FD]));
+   if (!file)
+   return -EBADF;
+
+   /* rtnl_lock is already held here, but ppp_create_interface() locks
+* ppp_mutex before holding rtnl_lock. Using mutex_trylock() avoids
+* possible deadlock due to lock order inversion, at the cost of
+* pushing the problem back to userspace.
+*/
+   if (!mutex_trylock(_mutex)) {
+   err = -EBUSY;
+   goto out;
+   }
+
+   if (file->f_op != _device_fops || file->private_data) {
+   err = -EBADF;
+   goto out_unlock;
+   }
+
+   conf.file = file;
+   err = ppp_dev_configure(src_net, dev, );
+
+out_unlock:
+   mutex_unlock(_mutex);
+out:
+   fput(file);
+
+   return err;
+}
+
+static void ppp_nl_dellink(struct net_device *dev, struct list_head *head)
+{
+   unregister_netdevice_queue(dev, head);
+}
+
+static size_t 

[RFC PATCH v3 1/2] ppp: define reusable device creation functions

[Guillaume Nault]

Move PPP device initialisation and registration out of
ppp_create_interface().
This prepares code for device registration with rtnetlink.

While there, simplify the prototype of ppp_create_interface():
  * Since ppp_dev_configure() takes care of setting file->private_data,
there's no need to return a ppp structure to ppp_unattached_ioctl()
anymore.
  * The unit parameter is made read/write so that ppp_create_interface()
can tell which unit number has been assigned.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 206 --
 1 file changed, 118 insertions(+), 88 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f572b31..f581a77 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -183,6 +183,11 @@ struct channel {
 #endif /* CONFIG_PPP_MULTILINK */
 };
 
+struct ppp_config {
+   struct file *file;
+   s32 unit;
+};
+
 /*
  * SMP locking issues:
  * Both the ppp.rlock and ppp.wlock locks protect the ppp.channels
@@ -269,8 +274,7 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit,
-   struct file *file, int *retp);
+static int ppp_create_interface(struct net *net, struct file *file, int *unit);
 static void init_ppp_file(struct ppp_file *pf, int kind);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
@@ -853,12 +857,12 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, file, );
-   if (!ppp)
+   err = ppp_create_interface(net, file, );
+   if (err < 0)
break;
-   file->private_data = >file;
+
err = -EFAULT;
-   if (put_user(ppp->file.index, p))
+   if (put_user(unit, p))
break;
err = 0;
break;
@@ -960,6 +964,94 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
+static int ppp_unit_register(struct ppp *ppp, int unit)
+{
+   struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
+   int ret;
+
+   mutex_lock(>all_ppp_mutex);
+
+   if (unit < 0) {
+   ret = unit_get(>units_idr, ppp);
+   if (ret < 0)
+   goto err;
+   } else {
+   /* Caller asked for a specific unit number. Fail with -EEXIST
+* if unavailable. For backward compatibility, return -EEXIST
+* too if idr allocation fails; this makes pppd retry without
+* requesting a specific unit number.
+*/
+   if (unit_find(>units_idr, unit)) {
+   ret = -EEXIST;
+   goto err;
+   }
+   ret = unit_set(>units_idr, ppp, unit);
+   if (ret < 0) {
+   /* Rewrite error for backward compatibility */
+   ret = -EEXIST;
+   goto err;
+   }
+   }
+   ppp->file.index = ret;
+
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+
+   ret = register_netdevice(ppp->dev);
+   if (ret < 0)
+   goto err_unit;
+
+   atomic_inc(_unit_count);
+
+   mutex_unlock(>all_ppp_mutex);
+
+   return 0;
+
+err_unit:
+   unit_put(>units_idr, ppp->file.index);
+err:
+   mutex_unlock(>all_ppp_mutex);
+
+   return ret;
+}
+
+static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
+const struct ppp_config *conf)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   int indx;
+   int err;
+
+   ppp->dev = dev;
+   ppp->ppp_net = src_net;
+   ppp->mru = PPP_MRU;
+   ppp->owner = conf->file;
+
+   init_ppp_file(>file, INTERFACE);
+   ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
+
+   for (indx = 0; indx < NUM_NP; ++indx)
+   ppp->npmode[indx] = NPMODE_PASS;
+   INIT_LIST_HEAD(>channels);
+   spin_lock_init(>rlock);
+   spin_lock_init(>wlock);
+#ifdef CONFIG_PPP_MULTILINK
+   ppp->minseq = -1;
+   skb_queue_head_init(>mrq);
+#endif /* CONFIG_PPP_MULTILINK */
+#ifdef CONFIG_PPP_FILTER
+   ppp->pass_filter = NULL;
+   ppp->active_filter = NULL

[PATCH v4 net-next 2/2] ppp: add rtnetlink device creation support

[Guillaume Nault]

Define PPP device handler for use with rtnetlink.
The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
contains the file descriptor of the associated /dev/ppp instance (the
file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
the ioctl-based API). The PPP device is removed when this file
descriptor is released (same behaviour as with ioctl based PPP
devices).

PPP devices created with the rtnetlink API behave like the ones created
with ioctl(PPPIOCNEWUNIT). In particular existing ioctls work the same
way, no matter how the PPP device was created.
The rtnl callbacks are also assigned to ioctl based PPP devices. This
way, rtnl messages have the same effect on any PPP devices.
The immediate effect is that all PPP devices, even ioctl-based
ones, can now be removed with "ip link del".

A minor difference still exists between ioctl and rtnl based PPP
interfaces: in the device name, the number following the "ppp" prefix
corresponds to the PPP unit number for ioctl based devices, while it is
just an unrelated incrementing index for rtnl ones.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 115 --
 include/uapi/linux/if_link.h  |   8 +++
 2 files changed, 120 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 59077c8..8dedafa 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -46,6 +46,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -186,6 +187,7 @@ struct channel {
 struct ppp_config {
struct file *file;
s32 unit;
+   bool ifname_is_set;
 };
 
 /*
@@ -286,6 +288,7 @@ static int unit_get(struct idr *p, void *ptr);
 static int unit_set(struct idr *p, void *ptr, int n);
 static void unit_put(struct idr *p, int n);
 static void *unit_find(struct idr *p, int n);
+static void ppp_setup(struct net_device *dev);
 
 static const struct net_device_ops ppp_netdev_ops;
 
@@ -964,7 +967,7 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
-static int ppp_unit_register(struct ppp *ppp, int unit)
+static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
 {
struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
int ret;
@@ -994,7 +997,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
}
ppp->file.index = ret;
 
-   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+   if (!ifname_is_set)
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
 
ret = register_netdevice(ppp->dev);
if (ret < 0)
@@ -1043,7 +1047,7 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
ppp->active_filter = NULL;
 #endif /* CONFIG_PPP_FILTER */
 
-   err = ppp_unit_register(ppp, conf->unit);
+   err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
if (err < 0)
return err;
 
@@ -1052,6 +1056,99 @@ static int ppp_dev_configure(struct net *src_net, struct 
net_device *dev,
return 0;
 }
 
+static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
+   [IFLA_PPP_DEV_FD]   = { .type = NLA_S32 },
+};
+
+static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+   if (!data)
+   return -EINVAL;
+
+   if (!data[IFLA_PPP_DEV_FD])
+   return -EINVAL;
+   if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
+   return -EBADF;
+
+   return 0;
+}
+
+static int ppp_nl_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+   struct ppp_config conf = {
+   .unit = -1,
+   .ifname_is_set = true,
+   };
+   struct file *file;
+   int err;
+
+   file = fget(nla_get_s32(data[IFLA_PPP_DEV_FD]));
+   if (!file)
+   return -EBADF;
+
+   /* rtnl_lock is already held here, but ppp_create_interface() locks
+* ppp_mutex before holding rtnl_lock. Using mutex_trylock() avoids
+* possible deadlock due to lock order inversion, at the cost of
+* pushing the problem back to userspace.
+*/
+   if (!mutex_trylock(_mutex)) {
+   err = -EBUSY;
+   goto out;
+   }
+
+   if (file->f_op != _device_fops || file->private_data) {
+   err = -EBADF;
+   goto out_unlock;
+   }
+
+   conf.file = file;
+   err = ppp_dev_configure(src_net, dev, );
+
+out_unlock:
+   mutex_unlock(_mutex);
+out:
+   fput(file);
+
+   return err;
+}
+
+static void ppp_nl_dellink(struct net_device *dev, struct list_head *head)
+{
+   unregister_netdevice_queue(dev, head);
+}
+
+static size_t 

[PATCH v4 net-next 0/2] ppp: add rtnetlink support

[Guillaume Nault]

PPP devices lack the ability to be customised at creation time. In
particular they can't be created in a given netns or with a particular
name. Moving or renaming the device after creation is possible, but
creates undesirable transient effects on servers where PPP devices are
constantly created and removed, as users connect and disconnect.
Implementing rtnetlink support solves this problem.

The rtnetlink handlers implemented in this series are minimal, and can
only replace the PPPIOCNEWUNIT ioctl. The rest of PPP ioctls remains
necessary for any other operation on channels and units.
It is perfectly possible to mix PPP devices created by rtnl
and by ioctl(PPPIOCNEWUNIT). Devices will behave in the same way.

mutex_trylock() is used to resolve the locking issue wrt. locking
dependency between rtnl_lock() and ppp_mutex (see ppp_nl_newlink() in
patch #2).

A user visible difference brought by this series is that old PPP
interfaces (those created with ioctl(PPPIOCNEWUNIT)), can now be
removed by "ip link del", just like new rtnl based PPP devices.


Changes since v3:
  - Rebase on net-next.
  - Not an RFC anymore.

Changes since v2:
  - Define ->rtnl_link_ops for ioctl based PPP devices, so they can
handle rtnl messages just like rtnl based ones (suggested by
Stephen Hemminger).
  - Move back to original lock ordering between ppp_mutex and rtnl_lock
to simplify patch series. Handle lock inversion issue using
mutex_trylock() (suggested by Stephen Hemminger).
  - Do file descriptor lookup directly in ppp_nl_newlink(), to simplify
ppp_dev_configure().

Changes since v1:
  - Rebase on net-next.
  - Invert locking order wrt. ppp_mutex and rtnl_lock and protect
file->private_data with ppp_mutex.


Guillaume Nault (2):
  ppp: define reusable device creation functions
  ppp: add rtnetlink device creation support

 drivers/net/ppp/ppp_generic.c | 315 ++
 include/uapi/linux/if_link.h  |   8 ++
 2 files changed, 235 insertions(+), 88 deletions(-)

-- 
2.8.1

[PATCH v4 net-next 1/2] ppp: define reusable device creation functions

[Guillaume Nault]

Move PPP device initialisation and registration out of
ppp_create_interface().
This prepares code for device registration with rtnetlink.

While there, simplify the prototype of ppp_create_interface():

  * Since ppp_dev_configure() takes care of setting file->private_data,
there's no need to return a ppp structure to ppp_unattached_ioctl()
anymore.

  * The unit parameter is made read/write so that ppp_create_interface()
can tell which unit number has been assigned.

Signed-off-by: Guillaume Nault <g.na...@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 206 --
 1 file changed, 118 insertions(+), 88 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f572b31..59077c8 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -183,6 +183,11 @@ struct channel {
 #endif /* CONFIG_PPP_MULTILINK */
 };
 
+struct ppp_config {
+   struct file *file;
+   s32 unit;
+};
+
 /*
  * SMP locking issues:
  * Both the ppp.rlock and ppp.wlock locks protect the ppp.channels
@@ -269,8 +274,7 @@ static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff 
*skb, int inbound);
 static void ppp_ccp_closed(struct ppp *ppp);
 static struct compressor *find_compressor(int type);
 static void ppp_get_stats(struct ppp *ppp, struct ppp_stats *st);
-static struct ppp *ppp_create_interface(struct net *net, int unit,
-   struct file *file, int *retp);
+static int ppp_create_interface(struct net *net, struct file *file, int *unit);
 static void init_ppp_file(struct ppp_file *pf, int kind);
 static void ppp_destroy_interface(struct ppp *ppp);
 static struct ppp *ppp_find_unit(struct ppp_net *pn, int unit);
@@ -853,12 +857,12 @@ static int ppp_unattached_ioctl(struct net *net, struct 
ppp_file *pf,
/* Create a new ppp unit */
if (get_user(unit, p))
break;
-   ppp = ppp_create_interface(net, unit, file, );
-   if (!ppp)
+   err = ppp_create_interface(net, file, );
+   if (err < 0)
break;
-   file->private_data = >file;
+
err = -EFAULT;
-   if (put_user(ppp->file.index, p))
+   if (put_user(unit, p))
break;
err = 0;
break;
@@ -960,6 +964,94 @@ static struct pernet_operations ppp_net_ops = {
.size = sizeof(struct ppp_net),
 };
 
+static int ppp_unit_register(struct ppp *ppp, int unit)
+{
+   struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
+   int ret;
+
+   mutex_lock(>all_ppp_mutex);
+
+   if (unit < 0) {
+   ret = unit_get(>units_idr, ppp);
+   if (ret < 0)
+   goto err;
+   } else {
+   /* Caller asked for a specific unit number. Fail with -EEXIST
+* if unavailable. For backward compatibility, return -EEXIST
+* too if idr allocation fails; this makes pppd retry without
+* requesting a specific unit number.
+*/
+   if (unit_find(>units_idr, unit)) {
+   ret = -EEXIST;
+   goto err;
+   }
+   ret = unit_set(>units_idr, ppp, unit);
+   if (ret < 0) {
+   /* Rewrite error for backward compatibility */
+   ret = -EEXIST;
+   goto err;
+   }
+   }
+   ppp->file.index = ret;
+
+   snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
+
+   ret = register_netdevice(ppp->dev);
+   if (ret < 0)
+   goto err_unit;
+
+   atomic_inc(_unit_count);
+
+   mutex_unlock(>all_ppp_mutex);
+
+   return 0;
+
+err_unit:
+   unit_put(>units_idr, ppp->file.index);
+err:
+   mutex_unlock(>all_ppp_mutex);
+
+   return ret;
+}
+
+static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
+const struct ppp_config *conf)
+{
+   struct ppp *ppp = netdev_priv(dev);
+   int indx;
+   int err;
+
+   ppp->dev = dev;
+   ppp->ppp_net = src_net;
+   ppp->mru = PPP_MRU;
+   ppp->owner = conf->file;
+
+   init_ppp_file(>file, INTERFACE);
+   ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
+
+   for (indx = 0; indx < NUM_NP; ++indx)
+   ppp->npmode[indx] = NPMODE_PASS;
+   INIT_LIST_HEAD(>channels);
+   spin_lock_init(>rlock);
+   spin_lock_init(>wlock);
+#ifdef CONFIG_PPP_MULTILINK
+   ppp->minseq = -1;
+   skb_queue_head_init(>mrq);
+#endif /* CONFIG_PPP_MULTILINK */
+#ifdef CONFIG_PPP_FILTER
+   ppp->pass_filter = NULL;
+   ppp->active_filter = NULL

Re: [Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

[Guillaume Nault]

On Sun, May 01, 2016 at 09:38:57PM +0800, Wang Shanker wrote:
> static int ppp_open(struct inode *inode, struct file *file)
> {
>   /*
>* This could (should?) be enforced by the permissions on /dev/ppp.
>*/
>   if (!capable(CAP_NET_ADMIN))
>   return -EPERM;
>   return 0;
> }
> ```
> 
> I wonder why CAP_NET_ADMIN is needed here, rather than leaving it to the
> permission of the device node. If there is no need, I suggest that the
> CAP_NET_ADMIN check be removed.
> 
If this test was removed here, then it'd have to be added again in the
PPPIOCNEWUNIT ioctl, at the very least, because creating a netdevice
should require CAP_NET_ADMIN. Therefore that wouldn't help for your
case.
I don't know why the test was placed in ppp_open() in the first place,
but changing it now would have side effects on user space. So I'd
rather leave the code as is.

Re: [Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

[Guillaume Nault]

On Tue, May 03, 2016 at 01:23:34PM +0200, Hannes Frederic Sowa wrote:
> On Tue, May 3, 2016, at 12:35, Richard Weinberger wrote:
> > On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.na...@alphalink.fr>
> > wrote:
> > > On Sun, May 01, 2016 at 09:38:57PM +0800, Wang Shanker wrote:
> > >> static int ppp_open(struct inode *inode, struct file *file)
> > >> {
> > >>   /*
> > >>* This could (should?) be enforced by the permissions on /dev/ppp.
> > >>*/
> > >>   if (!capable(CAP_NET_ADMIN))
> > >>   return -EPERM;
> > >>   return 0;
> > >> }
> > >> ```
> > >>
> > >> I wonder why CAP_NET_ADMIN is needed here, rather than leaving it to the
> > >> permission of the device node. If there is no need, I suggest that the
> > >> CAP_NET_ADMIN check be removed.
> > >>
> > > If this test was removed here, then it'd have to be added again in the
> > > PPPIOCNEWUNIT ioctl, at the very least, because creating a netdevice
> > > should require CAP_NET_ADMIN. Therefore that wouldn't help for your
> > > case.
> > > I don't know why the test was placed in ppp_open() in the first place,
> > > but changing it now would have side effects on user space. So I'd
> > > rather leave the code as is.
> > 
> > I think the question is whether we really require having CAP_NET_ADMIN
> > in the initial namespace and not just in the current one.
> > Is ppp not network namespace aware?
> 
> I agree, ns_capable(net->user_ns, CAP_NET_ADMIN), would probably make
> more sense.
>
I guess you assume net is set to current->nsproxy->net_ns here.
Why about ns_capable(current_user_ns(), CAP_NET_ADMIN)?

>From my understanding of the code (I currently have no practical
experience with user namespaces), net->user_ns points to the userns in
which the current netns was created, while current_user_ns() refers to
the caller's userns. Shouldn't we check the later? Otherwise, any
process running in the netns would have the same capabilities regarding
PPP ioctls().

But I'm certainly missing important points. Interactions between netns
and userns are something I never investigated before, and using
net->user_ns seems to be way more common than using current_user_ns()
for checking capabilities in the networking stack.

Re: [Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

[Guillaume Nault]

On Tue, May 03, 2016 at 12:35:12PM +0200, Richard Weinberger wrote:
> On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.na...@alphalink.fr> wrote:
> > On Sun, May 01, 2016 at 09:38:57PM +0800, Wang Shanker wrote:
> >> static int ppp_open(struct inode *inode, struct file *file)
> >> {
> >>   /*
> >>* This could (should?) be enforced by the permissions on /dev/ppp.
> >>*/
> >>   if (!capable(CAP_NET_ADMIN))
> >>   return -EPERM;
> >>   return 0;
> >> }
> >> ```
> >>
> >> I wonder why CAP_NET_ADMIN is needed here, rather than leaving it to the
> >> permission of the device node. If there is no need, I suggest that the
> >> CAP_NET_ADMIN check be removed.
> >>
> > If this test was removed here, then it'd have to be added again in the
> > PPPIOCNEWUNIT ioctl, at the very least, because creating a netdevice
> > should require CAP_NET_ADMIN. Therefore that wouldn't help for your
> > case.
> > I don't know why the test was placed in ppp_open() in the first place,
> > but changing it now would have side effects on user space. So I'd
> > rather leave the code as is.
> 
> I think the question is whether we really require having CAP_NET_ADMIN
> in the initial namespace and not just in the current one.
> Is ppp not network namespace aware?
> 
Indeed, I overlooked the namespace aspect of the problem. PPP is netns
aware, but ioctls performed on /dev/ppp file descriptors are all
serialised with ppp_mutex. A user could therefore affect other PPP
users by artificially creating contention on the ppp_mutex lock.

Other than that, I agree it'd make sense to test for user capabilies in
the current namespace rather than in the initial one.

Re: 4.6.3, pppoe + shaper workload, skb_panic / skb_push / ppp_start_xmit

[Guillaume Nault]

On Tue, Jul 12, 2016 at 10:31:18AM -0700, Cong Wang wrote:
> On Mon, Jul 11, 2016 at 12:45 PM,   wrote:
> > Hi
> >
> > On latest kernel i noticed kernel panic happening 1-2 times per day. It is
> > also happening on older kernel (at least 4.5.3).
> >
> ...
> >  [42916.426463] Call Trace:
> >  [42916.426658]  
> >
> >  [42916.426719]  [] skb_push+0x36/0x37
> >  [42916.427111]  [] ppp_start_xmit+0x10f/0x150
> > [ppp_generic]
> >  [42916.427314]  [] dev_hard_start_xmit+0x25a/0x2d3
> >  [42916.427516]  [] ?
> > validate_xmit_skb.isra.107.part.108+0x11d/0x238
> >  [42916.427858]  [] sch_direct_xmit+0x89/0x1b5
> >  [42916.428060]  [] __qdisc_run+0x133/0x170
> >  [42916.428261]  [] net_tx_action+0xe3/0x148
> >  [42916.428462]  [] __do_softirq+0xb9/0x1a9
> >  [42916.428663]  [] irq_exit+0x37/0x7c
> >  [42916.428862]  [] smp_apic_timer_interrupt+0x3d/0x48
> >  [42916.429063]  [] apic_timer_interrupt+0x7c/0x90
> 
> Interesting, we call a skb_cow_head() before skb_push() in ppp_start_xmit(),
> I have no idea why this could happen.
>
The skb is corrupted: head is at 8800b0bf2800 while data is at
ffa00500b0bf284c.

Figuring out how this corruption happened is going to be hard without a
way to reproduce the problem.

Denys, can you confirm you're using a vanilla kernel?
Also I guess the ppp devices and tc settings are handled by accel-ppp.
If so, can you share more info about your setup (accel-ppp.conf, radius
attributes, iptables...) so that I can try to reproduce it on my
machines?

Regards

Guillaume

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.benn...@alliedtelesis.co.nz>
> > > Cc: Paul Mackerras <pau...@samba.org>
> > > Cc: linux-...@vger.kernel.org
> > > Cc: Guillaume Nault <g.na...@alphalink.fr>
> > > Cc: Cyrill Gorcunov <gorcu...@openvz.org>
> > > Signed-off-by: Cong Wang <xiyou.wangc...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-ppp" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 4.6.3, pppoe + shaper workload, skb_panic / skb_push / ppp_start_xmit

[Guillaume Nault]

On Mon, Aug 08, 2016 at 02:25:00PM +0300, Denys Fedoryshchenko wrote:
> On 2016-08-01 23:59, Guillaume Nault wrote:
> > Do you still have the vmlinux file with debug symbols that generated
> > this panic?
> Sorry for delay, i didn't had same image on all servers and probably i found
> cause of panic, but still testing on several servers.
> If i remove SFQ qdisc from ppp shapers, servers not rebooting anymore.
> 
Thanks for the feedback. I wonder which interactions between SFQ and
PPP can lead to this problem. I'll take a look.

> But still i need around 2 days to make sure that's the reason.
> 
Okay, just let me know if you can confirm that removing SFQ really
solves the problem.