linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: ppp_generic - fix broken userspace
@ 2009-01-10 20:28 Cyrill Gorcunov
  2009-01-10 20:31 ` Cyrill Gorcunov
  0 siblings, 1 reply; 8+ messages in thread
From: Cyrill Gorcunov @ 2009-01-10 20:28 UTC (permalink / raw)
  To: David Miller, Andrew Morton, James Chapman; +Cc: LKML, NKML

The commits:

	7a95d267fb62cd6b80ef73be0592bbbe1dbd5df7
	ab5024ab23b78c86a0a1425defcdde48710fe449

introduced usage of IDR functionality but broke
userspace side.

Before this commits it was possible to allocate new ppp
interface with specified number. Now it fails with EINVAL.
Fix it by trying to allocate interface with specified unit
number and return EEXIST if fail which allow pppd to ask
us to allocate new unit number.

And fix messages on memory allocation fails - add details
that it's PPP module who is complaining.

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

If this is not a right way to fix this issue maybe better
would be to just revert the commits mentioned. Please
review carefully. Thanks!

The patch is on top of current net-next-2.6 tree.

 drivers/net/ppp_generic.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

Index: linux-2.6.git/drivers/net/ppp_generic.c
===================================================================
--- linux-2.6.git.orig/drivers/net/ppp_generic.c
+++ linux-2.6.git/drivers/net/ppp_generic.c
@@ -250,6 +250,7 @@ static int ppp_connect_channel(struct ch
 static int ppp_disconnect_channel(struct channel *pch);
 static void ppp_destroy_channel(struct channel *pch);
 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);
 
@@ -2432,11 +2433,18 @@ ppp_create_interface(int unit, int *retp
 	} else {
 		if (unit_find(&ppp_units_idr, unit))
 			goto out2; /* unit already exists */
-		else {
-			/* darn, someone is cheating us? */
-			*retp = -EINVAL;
+		/*
+		 * 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 :)
+		 */
+		unit = unit_set(&ppp_units_idr, ppp, unit);
+		if (unit < 0)
 			goto out2;
-		}
 	}
 
 	/* Initialize the new ppp unit */
@@ -2677,14 +2685,37 @@ static void __exit ppp_cleanup(void)
  * by holding all_ppp_mutex
  */
 
+/* associate pointer with specified number */
+static int unit_set(struct idr *p, void *ptr, int n)
+{
+	int unit, err;
+
+again:
+	if (!idr_pre_get(p, GFP_KERNEL)) {
+		printk(KERN_ERR "PPP: No free memory for idr\n");
+		return -ENOMEM;
+	}
+
+	err = idr_get_new_above(p, ptr, n, &unit);
+	if (err == -EAGAIN)
+		goto again;
+
+	if (unit != n) {
+		idr_remove(p, unit);
+		return -EINVAL;
+	}
+
+	return unit;
+}
+
 /* get new free unit number and associate pointer with it */
 static int unit_get(struct idr *p, void *ptr)
 {
 	int unit, err;
 
 again:
-	if (idr_pre_get(p, GFP_KERNEL) == 0) {
-		printk(KERN_ERR "Out of memory expanding drawable idr\n");
+	if (!idr_pre_get(p, GFP_KERNEL)) {
+		printk(KERN_ERR "PPP: No free memory for idr\n");
 		return -ENOMEM;
 	}
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-10 20:28 [PATCH] net: ppp_generic - fix broken userspace Cyrill Gorcunov
@ 2009-01-10 20:31 ` Cyrill Gorcunov
  2009-01-11  0:31   ` Paul Mackerras
  0 siblings, 1 reply; 8+ messages in thread
From: Cyrill Gorcunov @ 2009-01-10 20:31 UTC (permalink / raw)
  To: David Miller, Andrew Morton, James Chapman, LKML, NKML; +Cc: Paul Mackerras

Ugh, forgot to CC Paul...
...

		- Cyrill -

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-10 20:31 ` Cyrill Gorcunov
@ 2009-01-11  0:31   ` Paul Mackerras
  2009-01-11  7:32     ` Cyrill Gorcunov
  2009-01-11  7:35     ` David Miller
  0 siblings, 2 replies; 8+ messages in thread
From: Paul Mackerras @ 2009-01-11  0:31 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: David Miller, Andrew Morton, James Chapman, LKML, NKML

Cyrill Gorcunov writes:

> Ugh, forgot to CC Paul...
> ...
> 
> 		- Cyrill -

Forgot to cc me on what?

Judging by the subject, it sounds like I'm not going to like it...

Paul.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-11  0:31   ` Paul Mackerras
@ 2009-01-11  7:32     ` Cyrill Gorcunov
  2009-01-11  7:35     ` David Miller
  1 sibling, 0 replies; 8+ messages in thread
From: Cyrill Gorcunov @ 2009-01-11  7:32 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: David Miller, Andrew Morton, James Chapman, LKML, NKML

[Paul Mackerras - Sun, Jan 11, 2009 at 11:31:19AM +1100]
| Cyrill Gorcunov writes:
| 
| > Ugh, forgot to CC Paul...
| > ...
| > 
| > 		- Cyrill -
| 
| Forgot to cc me on what?
| 
| Judging by the subject, it sounds like I'm not going to like it...
| 
| Paul.
| 

On this http://lkml.org/lkml/2009/1/10/185
I stripped the patch body since I thought
you're subscribed to LKML. Here is the patch
so you don't need to search it.

		- Cyrill -
---
Subject: [PATCH] net: ppp_generic - fix broken userspace

The commits:

	7a95d267fb62cd6b80ef73be0592bbbe1dbd5df7
	ab5024ab23b78c86a0a1425defcdde48710fe449

introduced usage of IDR functionality but broke
userspace side.

Before this commits it was possible to allocate new ppp
interface with specified number. Now it fails with EINVAL.
Fix it by trying to allocate interface with specified unit
number and return EEXIST if fail which allow pppd to ask
us to allocate new unit number.

And fix messages on memory allocation fails - add details
that it's PPP module who is complaining.

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

If this is not a right way to fix this issue maybe better
would be to just revert the commits mentioned. Please
review carefully. Thanks!

 drivers/net/ppp_generic.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

Index: linux-2.6.git/drivers/net/ppp_generic.c
===================================================================
--- linux-2.6.git.orig/drivers/net/ppp_generic.c
+++ linux-2.6.git/drivers/net/ppp_generic.c
@@ -250,6 +250,7 @@ static int ppp_connect_channel(struct ch
 static int ppp_disconnect_channel(struct channel *pch);
 static void ppp_destroy_channel(struct channel *pch);
 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);
 
@@ -2432,11 +2433,18 @@ ppp_create_interface(int unit, int *retp
 	} else {
 		if (unit_find(&ppp_units_idr, unit))
 			goto out2; /* unit already exists */
-		else {
-			/* darn, someone is cheating us? */
-			*retp = -EINVAL;
+		/*
+		 * 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 :)
+		 */
+		unit = unit_set(&ppp_units_idr, ppp, unit);
+		if (unit < 0)
 			goto out2;
-		}
 	}
 
 	/* Initialize the new ppp unit */
@@ -2677,14 +2685,37 @@ static void __exit ppp_cleanup(void)
  * by holding all_ppp_mutex
  */
 
+/* associate pointer with specified number */
+static int unit_set(struct idr *p, void *ptr, int n)
+{
+	int unit, err;
+
+again:
+	if (!idr_pre_get(p, GFP_KERNEL)) {
+		printk(KERN_ERR "PPP: No free memory for idr\n");
+		return -ENOMEM;
+	}
+
+	err = idr_get_new_above(p, ptr, n, &unit);
+	if (err == -EAGAIN)
+		goto again;
+
+	if (unit != n) {
+		idr_remove(p, unit);
+		return -EINVAL;
+	}
+
+	return unit;
+}
+
 /* get new free unit number and associate pointer with it */
 static int unit_get(struct idr *p, void *ptr)
 {
 	int unit, err;
 
 again:
-	if (idr_pre_get(p, GFP_KERNEL) == 0) {
-		printk(KERN_ERR "Out of memory expanding drawable idr\n");
+	if (!idr_pre_get(p, GFP_KERNEL)) {
+		printk(KERN_ERR "PPP: No free memory for idr\n");
 		return -ENOMEM;
 	}
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-11  0:31   ` Paul Mackerras
  2009-01-11  7:32     ` Cyrill Gorcunov
@ 2009-01-11  7:35     ` David Miller
  2009-01-11 10:29       ` Paul Mackerras
  1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2009-01-11  7:35 UTC (permalink / raw)
  To: paulus; +Cc: gorcunov, akpm, jchapman, linux-kernel, netdev

From: Paul Mackerras <paulus@samba.org>
Date: Sun, 11 Jan 2009 11:31:19 +1100

> Judging by the subject, it sounds like I'm not going to like it...

PPP was converted to use IDR instead of PPP's duplicate
implementation.

A bug was introduced in that conversion, and that bug is being fixed
here.

What is not to like? :-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-11  7:35     ` David Miller
@ 2009-01-11 10:29       ` Paul Mackerras
  2009-01-11 11:45         ` Cyrill Gorcunov
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Mackerras @ 2009-01-11 10:29 UTC (permalink / raw)
  To: David Miller; +Cc: gorcunov, akpm, jchapman, linux-kernel, netdev

David Miller writes:

> PPP was converted to use IDR instead of PPP's duplicate
> implementation.
> 
> A bug was introduced in that conversion, and that bug is being fixed
> here.
> 
> What is not to like? :-)

Ah OK, that sounds all right.  I was reacting to the "broken
userspace" in the patch description which at first glance seemed to be
saying that userspace (i.e. pppd :) is doing something which has
recently been declared to be wrong and therefore it is "broken".

I think "fix broken userspace" as a patch headline is quite
ambiguous.  Something like "Fix bug in conversion to use IDR" would be
more informative.

Paul.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-11 10:29       ` Paul Mackerras
@ 2009-01-11 11:45         ` Cyrill Gorcunov
  2009-01-13  6:12           ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Cyrill Gorcunov @ 2009-01-11 11:45 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: David Miller, akpm, jchapman, linux-kernel, netdev

[Paul Mackerras - Sun, Jan 11, 2009 at 09:29:49PM +1100]
| David Miller writes:
| 
| > PPP was converted to use IDR instead of PPP's duplicate
| > implementation.
| > 
| > A bug was introduced in that conversion, and that bug is being fixed
| > here.
| > 
| > What is not to like? :-)
| 
| Ah OK, that sounds all right.  I was reacting to the "broken
| userspace" in the patch description which at first glance seemed to be
| saying that userspace (i.e. pppd :) is doing something which has
| recently been declared to be wrong and therefore it is "broken".
| 
| I think "fix broken userspace" as a patch headline is quite
| ambiguous.  Something like "Fix bug in conversion to use IDR" would be
| more informative.
| 
| Paul.
| 

David, if you pick this patch could you rename
the subject then as Paul proposed? Or I should
resend the patch with new title?

To Paul: my bad, sorry. I thought like -- I broke
user-space pppd so it's broken now and lets fix
it -- here is "fix broken userspace" grown from :)

		- Cyrill -

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: ppp_generic - fix broken userspace
  2009-01-11 11:45         ` Cyrill Gorcunov
@ 2009-01-13  6:12           ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2009-01-13  6:12 UTC (permalink / raw)
  To: gorcunov; +Cc: paulus, akpm, jchapman, linux-kernel, netdev

From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Sun, 11 Jan 2009 14:45:53 +0300

> David, if you pick this patch could you rename
> the subject then as Paul proposed? Or I should
> resend the patch with new title?

Done, here is the commit message I used:

    net: ppp_generic - fix regressions caused by IDR conversion
    
    The commits:
    
    	7a95d267fb62cd6b80ef73be0592bbbe1dbd5df7
    	("net: ppp_generic - use idr technique instead of cardmaps")
    
    	ab5024ab23b78c86a0a1425defcdde48710fe449
    	("net: ppp_generic - use DEFINE_IDR for static initialization")
    
    introduced usage of IDR functionality but broke userspace side.
    
    Before this commits it was possible to allocate new ppp interface with
    specified number. Now it fails with EINVAL.  Fix it by trying to
    allocate interface with specified unit number and return EEXIST if
    fail which allow pppd to ask us to allocate new unit number.
    
    And fix messages on memory allocation fails - add details that it's
    PPP module who is complaining.
    
    Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2009-01-13  6:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-10 20:28 [PATCH] net: ppp_generic - fix broken userspace Cyrill Gorcunov
2009-01-10 20:31 ` Cyrill Gorcunov
2009-01-11  0:31   ` Paul Mackerras
2009-01-11  7:32     ` Cyrill Gorcunov
2009-01-11  7:35     ` David Miller
2009-01-11 10:29       ` Paul Mackerras
2009-01-11 11:45         ` Cyrill Gorcunov
2009-01-13  6:12           ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).