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

[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 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.nault@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, &err);
-		if (!ppp)
+
+		err = ppp_create_interface(net, file, &unit);
+		if (err < 0)
 			break;
-		file->private_data = &ppp->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(&pn->all_ppp_mutex);
 
-	if (unit < 0) {
-		unit = unit_get(&pn->units_idr, ppp);
-		if (unit < 0) {
-			ret = unit;
+	if (*unit < 0) {
+		ret = unit_get(&pn->units_idr, ppp);
+		if (ret < 0)
 			goto out2;
-		}
 	} else {
 		ret = -EEXIST;
-		if (unit_find(&pn->units_idr, unit))
+		if (unit_find(&pn->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(&pn->units_idr, ppp, unit);
-		if (unit < 0)
+		ret = unit_set(&pn->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(&pn->units_idr, unit);
+		unit_put(&pn->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 = &ppp->file;
+	*unit = ppp->file.index;
 	atomic_inc(&ppp_unit_count);
+
 	mutex_unlock(&pn->all_ppp_mutex);
 	rtnl_unlock();
 
-	*retp = 0;
-	return ppp;
+	return 0;
 
 out2:
 	mutex_unlock(&pn->all_ppp_mutex);
 	rtnl_unlock();
 	free_netdev(dev);
 out1:
-	*retp = ret;
-	return NULL;
+	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.nault@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(&ppp_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(&ppp_mutex);
 		mutex_lock(&pn->all_ppp_mutex);
 		ppp = ppp_find_unit(pn, unit);
 		if (ppp) {
-			atomic_inc(&ppp->file.refcnt);
-			file->private_data = &ppp->file;
-			err = 0;
+			err = -ENOTTY;
+			if (!file->private_data) {
+				atomic_inc(&ppp->file.refcnt);
+				file->private_data = &ppp->file;
+				err = 0;
+			}
 		}
 		mutex_unlock(&pn->all_ppp_mutex);
+		mutex_unlock(&ppp_mutex);
 		break;
 
 	case PPPIOCATTCHAN:
 		if (get_user(unit, p))
 			break;
+
 		err = -ENXIO;
 		pn = ppp_pernet(net);
+		mutex_lock(&ppp_mutex);
 		spin_lock_bh(&pn->all_channels_lock);
 		chan = ppp_find_channel(pn, unit);
 		if (chan) {
-			atomic_inc(&chan->file.refcnt);
-			file->private_data = &chan->file;
-			err = 0;
+			err = -ENOTTY;
+			if (!file->private_data) {
+				atomic_inc(&chan->file.refcnt);
+				file->private_data = &chan->file;
+				err = 0;
+			}
 		}
 		spin_unlock_bh(&pn->all_channels_lock);
+		mutex_unlock(&ppp_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(&ppp_mutex);
 	rtnl_lock();
 	mutex_lock(&pn->all_ppp_mutex);
 
+	if (file->private_data) {
+		ret = -ENOTTY;
+		goto out2;
+	}
+
 	if (*unit < 0) {
 		ret = unit_get(&pn->units_idr, ppp);
 		if (ret < 0)
@@ -2818,12 +2843,14 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit)
 
 	mutex_unlock(&pn->all_ppp_mutex);
 	rtnl_unlock();
+	mutex_unlock(&ppp_mutex);
 
 	return 0;
 
 out2:
 	mutex_unlock(&pn->all_ppp_mutex);
 	rtnl_unlock();
+	mutex_unlock(&ppp_mutex);
 	free_netdev(dev);
 out1:
 	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.nault@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(&ppp_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(&file->f_count));
+
+		return err;
+	}
+
+	mutex_lock(&ppp_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.nault@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(&ppp_mutex);
 	rtnl_lock();
+	mutex_lock(&ppp_mutex);
 	mutex_lock(&pn->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(&ppp_unit_count);
 
 	mutex_unlock(&pn->all_ppp_mutex);
-	rtnl_unlock();
 	mutex_unlock(&ppp_mutex);
+	rtnl_unlock();
 
 	return 0;
 
 out2:
 	mutex_unlock(&pn->all_ppp_mutex);
-	rtnl_unlock();
 	mutex_unlock(&ppp_mutex);
+	rtnl_unlock();
 	free_netdev(dev);
 out1:
 	return ret;
-- 
2.8.0.rc3

[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.nault@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(&pn->all_ppp_mutex);
+
+	if (unit < 0) {
+		ret = unit_get(&pn->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(&pn->units_idr, unit)) {
+			ret = -EEXIST;
+			goto err;
+		}
+		ret = unit_set(&pn->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(&ppp_unit_count);
+
+	mutex_unlock(&pn->all_ppp_mutex);
+
+	return 0;
+
+err_unit:
+	unit_put(&pn->units_idr, ppp->file.index);
+err:
+	mutex_unlock(&pn->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(&ppp->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(&ppp->channels);
+	spin_lock_init(&ppp->rlock);
+	spin_lock_init(&ppp->wlock);
+#ifdef CONFIG_PPP_MULTILINK
+	ppp->minseq = -1;
+	skb_queue_head_init(&ppp->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(&ppp->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(&ppp->channels);
-	spin_lock_init(&ppp->rlock);
-	spin_lock_init(&ppp->wlock);
-#ifdef CONFIG_PPP_MULTILINK
-	ppp->minseq = -1;
-	skb_queue_head_init(&ppp->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 belong to
-	 */
 	dev_net_set(dev, net);
 
 	rtnl_lock();
 	mutex_lock(&ppp_mutex);
-	mutex_lock(&pn->all_ppp_mutex);
-
 	if (file->private_data) {
-		ret = -ENOTTY;
-		goto out2;
-	}
-
-	if (*unit < 0) {
-		ret = unit_get(&pn->units_idr, ppp);
-		if (ret < 0)
-			goto out2;
-	} else {
-		ret = -EEXIST;
-		if (unit_find(&pn->units_idr, *unit))
-			goto out2; /* unit already exists */
-		/*
-		 * if caller need a specified unit number
-		 * lets try to satisfy him, otherwise --
-		 * he should better ask us for new unit number
-		 *
-		 * NOTE: yes I know that returning EEXIST it's not
-		 * fair but at least pppd will ask us to allocate
-		 * new unit in this case so user is happy :)
-		 */
-		ret = unit_set(&pn->units_idr, ppp, *unit);
-		if (ret < 0) {
-			ret = -EEXIST;
-			goto out2;
-		}
+		err = -ENOTTY;
+		goto err_dev;
 	}
 
-	/* Initialize the new ppp unit */
-	ppp->file.index = ret;
-	sprintf(dev->name, "ppp%d", ret);
+	err = ppp_dev_configure(net, dev, &conf);
+	if (err < 0)
+		goto err_dev;
 
-	ret = register_netdevice(dev);
-	if (ret != 0) {
-		unit_put(&pn->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 = &ppp->file;
+	ppp = netdev_priv(dev);
 	*unit = ppp->file.index;
-	atomic_inc(&ppp_unit_count);
+	file->private_data = &ppp->file;
 
-	mutex_unlock(&pn->all_ppp_mutex);
 	mutex_unlock(&ppp_mutex);
 	rtnl_unlock();
 
 	return 0;
 
-out2:
-	mutex_unlock(&pn->all_ppp_mutex);
+err_dev:
 	mutex_unlock(&ppp_mutex);
 	rtnl_unlock();
 	free_netdev(dev);
-out1:
-	return ret;
+err:
+	return err;
 }
 
 /*
-- 
2.8.0.rc3

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

[Stephen Hemminger]

On Tue, 5 Apr 2016 02:56:29 +0200
Guillaume Nault <g.nault@alphalink.fr> wrote:

> 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.nault@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(&pn->all_ppp_mutex);
> +
> +	if (unit < 0) {
> +		ret = unit_get(&pn->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(&pn->units_idr, unit)) {
> +			ret = -EEXIST;
> +			goto err;
> +		}
> +		ret = unit_set(&pn->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(&ppp_unit_count);
> +
> +	mutex_unlock(&pn->all_ppp_mutex);
> +
> +	return 0;
> +
> +err_unit:
> +	unit_put(&pn->units_idr, ppp->file.index);
> +err:
> +	mutex_unlock(&pn->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(&ppp->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(&ppp->channels);
> +	spin_lock_init(&ppp->rlock);
> +	spin_lock_init(&ppp->wlock);
> +#ifdef CONFIG_PPP_MULTILINK
> +	ppp->minseq = -1;
> +	skb_queue_head_init(&ppp->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(&ppp->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(&ppp->channels);
> -	spin_lock_init(&ppp->rlock);
> -	spin_lock_init(&ppp->wlock);
> -#ifdef CONFIG_PPP_MULTILINK
> -	ppp->minseq = -1;
> -	skb_queue_head_init(&ppp->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 belong to
> -	 */
>  	dev_net_set(dev, net);
>  
>  	rtnl_lock();
>  	mutex_lock(&ppp_mutex);
> -	mutex_lock(&pn->all_ppp_mutex);
> -
>  	if (file->private_data) {
> -		ret = -ENOTTY;
> -		goto out2;
> -	}
> -
> -	if (*unit < 0) {
> -		ret = unit_get(&pn->units_idr, ppp);
> -		if (ret < 0)
> -			goto out2;
> -	} else {
> -		ret = -EEXIST;
> -		if (unit_find(&pn->units_idr, *unit))
> -			goto out2; /* unit already exists */
> -		/*
> -		 * if caller need a specified unit number
> -		 * lets try to satisfy him, otherwise --
> -		 * he should better ask us for new unit number
> -		 *
> -		 * NOTE: yes I know that returning EEXIST it's not
> -		 * fair but at least pppd will ask us to allocate
> -		 * new unit in this case so user is happy :)
> -		 */
> -		ret = unit_set(&pn->units_idr, ppp, *unit);
> -		if (ret < 0) {
> -			ret = -EEXIST;
> -			goto out2;
> -		}
> +		err = -ENOTTY;
> +		goto err_dev;
>  	}
>  
> -	/* Initialize the new ppp unit */
> -	ppp->file.index = ret;
> -	sprintf(dev->name, "ppp%d", ret);
> +	err = ppp_dev_configure(net, dev, &conf);
> +	if (err < 0)
> +		goto err_dev;
>  
> -	ret = register_netdevice(dev);
> -	if (ret != 0) {
> -		unit_put(&pn->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 = &ppp->file;
> +	ppp = netdev_priv(dev);
>  	*unit = ppp->file.index;
> -	atomic_inc(&ppp_unit_count);
> +	file->private_data = &ppp->file;
>  
> -	mutex_unlock(&pn->all_ppp_mutex);
>  	mutex_unlock(&ppp_mutex);
>  	rtnl_unlock();
>  
>  	return 0;
>  
> -out2:
> -	mutex_unlock(&pn->all_ppp_mutex);
> +err_dev:
>  	mutex_unlock(&ppp_mutex);
>  	rtnl_unlock();
>  	free_netdev(dev);
> -out1:
> -	return ret;
> +err:
> +	return err;
>  }
>  
>  /*

Does PPP module autoload correctly based on the netlink attributes?

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.nault@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 5/6] ppp: define reusable device creation functions

[Stephen Hemminger]

On Tue, 5 Apr 2016 23:14:56 +0200
Guillaume Nault <g.nault@alphalink.fr> wrote:

> 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.nault@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?
> 

That should be enough.

[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.nault@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 <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/file.h>
 #include <asm/unaligned.h>
 #include <net/slhc_vj.h>
 #include <linux/atomic.h>
@@ -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 != &ppp_device_fops) {
+			err = -EBADF;
+			goto out;
+		}
+	}
+
+	mutex_lock(&ppp_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(&ppp->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 = &ppp->file;
+
+out_mutex:
+	mutex_unlock(&ppp_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 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, &conf);
+}
+
+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		= "ppp",
+	.maxtype	= IFLA_PPP_MAX,
+	.policy		= ppp_nl_policy,
+	.priv_size	= sizeof(struct ppp),
+	.setup		= ppp_setup,
+	.validate	= ppp_nl_validate,
+	.newlink	= ppp_nl_newlink,
+	.dellink	= ppp_nl_dellink,
+	.get_size	= ppp_nl_get_size,
+	.fill_info	= ppp_nl_fill_info,
+	.get_link_net	= ppp_nl_get_link_net,
+};
+
 #define PPP_MAJOR	108
 
 /* Called at boot time if ppp is compiled into the kernel,
@@ -1098,11 +1209,19 @@ static int __init ppp_init(void)
 		goto out_chrdev;
 	}
 
+	err = rtnl_link_register(&ppp_link_ops);
+	if (err) {
+		pr_err("failed to register rtnetlink PPP handler\n");
+		goto out_class;
+	}
+
 	/* not a big deal if we fail here :-) */
 	device_create(ppp_class, NULL, MKDEV(PPP_MAJOR, 0), NULL, "ppp");
 
 	return 0;
 
+out_class:
+	class_destroy(ppp_class);
 out_chrdev:
 	unregister_chrdev(PPP_MAJOR, "ppp");
 out_net:
@@ -2846,7 +2965,9 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit)
 {
 	struct ppp_config conf = {
 		.file = file,
+		.fd = -1,
 		.unit = *unit,
+		.ifname_is_set = false,
 	};
 	struct net_device *dev;
 	struct ppp *ppp;
@@ -2861,27 +2982,17 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit)
 	dev_net_set(dev, net);
 
 	rtnl_lock();
-	mutex_lock(&ppp_mutex);
-	if (file->private_data) {
-		err = -ENOTTY;
-		goto err_dev;
-	}
-
 	err = ppp_dev_configure(net, dev, &conf);
 	if (err < 0)
 		goto err_dev;
+	rtnl_unlock();
 
 	ppp = netdev_priv(dev);
 	*unit = ppp->file.index;
-	file->private_data = &ppp->file;
-
-	mutex_unlock(&ppp_mutex);
-	rtnl_unlock();
 
 	return 0;
 
 err_dev:
-	mutex_unlock(&ppp_mutex);
 	rtnl_unlock();
 	free_netdev(dev);
 err:
@@ -3074,6 +3185,7 @@ static void __exit ppp_cleanup(void)
 	/* should never happen */
 	if (atomic_read(&ppp_unit_count) || atomic_read(&channel_count))
 		pr_err("PPP: removing module but units remain!\n");
+	rtnl_link_unregister(&ppp_link_ops);
 	unregister_chrdev(PPP_MAJOR, "ppp");
 	device_destroy(ppp_class, MKDEV(PPP_MAJOR, 0));
 	class_destroy(ppp_class);
@@ -3132,4 +3244,5 @@ EXPORT_SYMBOL(ppp_register_compressor);
 EXPORT_SYMBOL(ppp_unregister_compressor);
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_CHARDEV(PPP_MAJOR, 0);
+MODULE_ALIAS_RTNL_LINK("ppp");
 MODULE_ALIAS("devname:ppp");
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index c488066..f238de9 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -515,6 +515,14 @@ enum {
 };
 #define IFLA_GENEVE_MAX	(__IFLA_GENEVE_MAX - 1)
 
+/* PPP section */
+enum {
+	IFLA_PPP_UNSPEC,
+	IFLA_PPP_DEV_FD,
+	__IFLA_PPP_MAX,
+};
+#define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
+
 /* Bonding section */
 
 enum {
-- 
2.8.0.rc3

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

[walter harms]

Am 05.04.2016 02:56, schrieb 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.nault@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 <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/file.h>
>  #include <asm/unaligned.h>
>  #include <net/slhc_vj.h>
>  #include <linux/atomic.h>
> @@ -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;

why not just return -EBADF;

> +		}
> +	} else {
> +		file = fget(conf->fd);
> +		if (!file) {
> +			err = -EBADF;
> +			goto out;
	
why not just return -EBADF;
> +		}

just my 2 cents,

re,
 wh

> +
> +		if (file->f_op != &ppp_device_fops) {
> +			err = -EBADF;
> +			goto out;
> +		}
> +	}
> +
> +	mutex_lock(&ppp_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(&ppp->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 = &ppp->file;
> +
> +out_mutex:
> +	mutex_unlock(&ppp_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 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, &conf);
> +}
> +
> +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		= "ppp",
> +	.maxtype	= IFLA_PPP_MAX,
> +	.policy		= ppp_nl_policy,
> +	.priv_size	= sizeof(struct ppp),
> +	.setup		= ppp_setup,
> +	.validate	= ppp_nl_validate,
> +	.newlink	= ppp_nl_newlink,
> +	.dellink	= ppp_nl_dellink,
> +	.get_size	= ppp_nl_get_size,
> +	.fill_info	= ppp_nl_fill_info,
> +	.get_link_net	= ppp_nl_get_link_net,
> +};
> +
>  #define PPP_MAJOR	108
>  
>  /* Called at boot time if ppp is compiled into the kernel,
> @@ -1098,11 +1209,19 @@ static int __init ppp_init(void)
>  		goto out_chrdev;
>  	}
>  
> +	err = rtnl_link_register(&ppp_link_ops);
> +	if (err) {
> +		pr_err("failed to register rtnetlink PPP handler\n");
> +		goto out_class;
> +	}
> +
>  	/* not a big deal if we fail here :-) */
>  	device_create(ppp_class, NULL, MKDEV(PPP_MAJOR, 0), NULL, "ppp");
>  
>  	return 0;
>  
> +out_class:
> +	class_destroy(ppp_class);
>  out_chrdev:
>  	unregister_chrdev(PPP_MAJOR, "ppp");
>  out_net:
> @@ -2846,7 +2965,9 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit)
>  {
>  	struct ppp_config conf = {
>  		.file = file,
> +		.fd = -1,
>  		.unit = *unit,
> +		.ifname_is_set = false,
>  	};
>  	struct net_device *dev;
>  	struct ppp *ppp;
> @@ -2861,27 +2982,17 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit)
>  	dev_net_set(dev, net);
>  
>  	rtnl_lock();
> -	mutex_lock(&ppp_mutex);
> -	if (file->private_data) {
> -		err = -ENOTTY;
> -		goto err_dev;
> -	}
> -
>  	err = ppp_dev_configure(net, dev, &conf);
>  	if (err < 0)
>  		goto err_dev;
> +	rtnl_unlock();
>  
>  	ppp = netdev_priv(dev);
>  	*unit = ppp->file.index;
> -	file->private_data = &ppp->file;
> -
> -	mutex_unlock(&ppp_mutex);
> -	rtnl_unlock();
>  
>  	return 0;
>  
>  err_dev:
> -	mutex_unlock(&ppp_mutex);
>  	rtnl_unlock();
>  	free_netdev(dev);
>  err:
> @@ -3074,6 +3185,7 @@ static void __exit ppp_cleanup(void)
>  	/* should never happen */
>  	if (atomic_read(&ppp_unit_count) || atomic_read(&channel_count))
>  		pr_err("PPP: removing module but units remain!\n");
> +	rtnl_link_unregister(&ppp_link_ops);
>  	unregister_chrdev(PPP_MAJOR, "ppp");
>  	device_destroy(ppp_class, MKDEV(PPP_MAJOR, 0));
>  	class_destroy(ppp_class);
> @@ -3132,4 +3244,5 @@ EXPORT_SYMBOL(ppp_register_compressor);
>  EXPORT_SYMBOL(ppp_unregister_compressor);
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS_CHARDEV(PPP_MAJOR, 0);
> +MODULE_ALIAS_RTNL_LINK("ppp");
>  MODULE_ALIAS("devname:ppp");
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index c488066..f238de9 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -515,6 +515,14 @@ enum {
>  };
>  #define IFLA_GENEVE_MAX	(__IFLA_GENEVE_MAX - 1)
>  
> +/* PPP section */
> +enum {
> +	IFLA_PPP_UNSPEC,
> +	IFLA_PPP_DEV_FD,
> +	__IFLA_PPP_MAX,
> +};
> +#define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
> +
>  /* Bonding section */
>  
>  enum {

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

[walter harms]

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.

NTL its up to you.

re,
 wh

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: [RFC PATCH 0/6] ppp: add rtnetlink support

[Stephen Hemminger]

On Tue, 5 Apr 2016 02:56:17 +0200
Guillaume Nault <g.nault@alphalink.fr> wrote:

> 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.

Good to see PPP behave like other tunnels.

> 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),
and work with same API set independent of how device was created.
Special cases are nuisance and source of bugs.

> 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.

> 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.

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.nault@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.