tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed

If the gsmtty is still used by some process, we could not just simply clear gsm_mux[gsm->num]. Clear it when gsm is being free. Otherwise we will hit crashes when userspace close the gsmtty.

Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely. We can do activation/deactivation with same gsm more than once now. This is for fixing the FIXME.

Signed-off-by xinhui.pan <[email protected]>

Reviewed-by Zhang Yanmin <[email protected]>

---
 drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 24 deletions(-)

drivers/tty/n_gsm.c +0 -0

index 81e7ccb..290df56 100644

drivers/tty/n_gsm.c +23 -0

+	if (gsm_mux[gsm->num] == gsm) /* We have already set gsm->num */
+		return 0;
+
+	spin_lock(&gsm_mux_lock);
+	for (i = 0; i < MAX_MUX; i++) {
+		if (gsm_mux[i] == NULL) {
+			gsm->num = i;
+			gsm_mux[i] = gsm;
+			break;
+		}
+	}
+	spin_unlock(&gsm_mux_lock);
+
+	if (i == MAX_MUX)
+		return -EBUSY;
+	return 0;
+}
+
+/**
+ *	gsm_mux_put		-	put/clear one entry in gsm_mux
+ *	@gsm: our gsm
+ *
+ *	Although its name end with put, it don't dec ref-count actually.

Quoted 21 lines · xinhui.pan

If the gsmtty is still used by some process, we could not just simply clear gsm_mux[gsm->num]. Clear it when gsm is being free. Otherwise we will hit crashes when userspace close the gsmtty.

Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely. We can do activation/deactivation with same gsm more than once now. This is for fixing the FIXME.

Signed-off-by xinhui.pan <[email protected]>

Reviewed-by Zhang Yanmin <[email protected]>

---
 drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 24 deletions(-)

drivers/tty/n_gsm.c +0 -0

index 81e7ccb..290df56 100644
@@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
 }
 

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2026

Quoted 5 lines

drivers/tty/n_gsm.c +4 -0

 /**
+ *	gsm_mux_get		-	get/fill one entry in gsm_mux
+ *	@gsm: our gsm
+ *
+ *	Although its name end with get, it don't inc ref-count actually.

Then don't call it a 'get' function :(

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2028

Quoted 2 lines

drivers/tty/n_gsm.c +2 -0

+ *	get one entry is just like fill pte, first memory access will
+ *	cause page_fault, the next accesses don't. So do here.

This doesn't make much sense to me, can you please explain it better?

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2030

Quoted 2 lines

drivers/tty/n_gsm.c +2 -0

+ */
+

blank line?

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2036

Quoted 6 lines

drivers/tty/n_gsm.c +6 -0

+static int gsm_mux_get(struct gsm_mux *gsm)
+{
+	int i;
+
+	if (gsm->num >= MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
+		return -EIO;

-EIO?

Quoted 27 lines

drivers/tty/n_gsm.c +27 -0

+	if (gsm_mux[gsm->num] == gsm) /* We have already set gsm->num */
+		return 0;
+
+	spin_lock(&gsm_mux_lock);
+	for (i = 0; i < MAX_MUX; i++) {
+		if (gsm_mux[i] == NULL) {
+			gsm->num = i;
+			gsm_mux[i] = gsm;
+			break;
+		}
+	}
+	spin_unlock(&gsm_mux_lock);
+
+	if (i == MAX_MUX)
+		return -EBUSY;
+	return 0;
+}
+
+/**
+ *	gsm_mux_put		-	put/clear one entry in gsm_mux
+ *	@gsm: our gsm
+ *
+ *	Although its name end with put, it don't dec ref-count actually.
+ *	put one entry is just like clear pte, So do here.
+ */
+
+static void gsm_mux_put(struct gsm_mux *gsm)

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2069

Quoted 6 lines

drivers/tty/n_gsm.c +6 -0

+{
+	if (gsm->num >= MAX_MUX)
+		return;
+
+	spin_lock(&gsm_mux_lock);
+	if (gsm_mux[gsm->num] == gsm)

How can this not be true?

drivers/tty/n_gsm.c static void gsm_error(struct gsm_mux *gsm, · line 2072

Quoted 4 lines

drivers/tty/n_gsm.c +3 -0

+		gsm_mux[gsm->num] = NULL;
+	spin_unlock(&gsm_mux_lock);
+}

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

thanks,

greg k-h

Quoted 3 lines

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

Thanks for your nice comments.
Struct gsm has a ref-count already. :)

And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.

and it can tell us if we can create a new gsm. :)

In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

Quoted 6 lines · xinhui.pan

Quoted 3 lines

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

Thanks for your nice comments.
Struct gsm has a ref-count already. :)

Then you should be fine, no need to keep it in an array.

Quoted 2 lines

And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.

You shouldn't need that at all, just use a list, you don't care what the XX number is within the driver, just allocate a new one with the next available number and you should be fine.

Quoted 1 line

and it can tell us if we can create a new gsm. :)

You should always be able to create a new gsm if you need to :)

Quoted 1 line

In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

Why limit to 256? Just use a list, and a idr structure to allocate the minor number, and all should be good.

Quoted 17 lines · Greg Kroah-Hartman

Quoted 6 lines · xinhui.pan

Quoted 3 lines

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

Thanks for your nice comments.
Struct gsm has a ref-count already. :)

Then you should be fine, no need to keep it in an array.

Quoted 2 lines

And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.

You shouldn't need that at all, just use a list, you don't care what the XX number is within the driver, just allocate a new one with the next available number and you should be fine.

Quoted 8 lines

Quoted 1 line

and it can tell us if we can create a new gsm. :)

You should always be able to create a new gsm if you need to :)

Quoted 1 line

In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

Why limit to 256? Just use a list, and a idr structure to allocate the minor number, and all should be good.

Hi Greg,

This is still broken in the gsm driver.

As much as I'd like to see someone take ownership of the gsm driver and do this 'the right way', I think until that happens we should consider fixing the reuse-while-in-use error.

Would you be willing to take a patch from me that just does the bare minimum to keep this from panicking in the cdev code?

Quoted 29 lines · Peter Hurley

Quoted 17 lines · Greg Kroah-Hartman

Quoted 6 lines · xinhui.pan

Quoted 3 lines

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

Thanks for your nice comments.
Struct gsm has a ref-count already. :)

Then you should be fine, no need to keep it in an array.

Quoted 2 lines

And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.

You shouldn't need that at all, just use a list, you don't care what the XX number is within the driver, just allocate a new one with the next available number and you should be fine.

Quoted 8 lines

Quoted 1 line

and it can tell us if we can create a new gsm. :)

You should always be able to create a new gsm if you need to :)

Quoted 1 line

In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

Why limit to 256? Just use a list, and a idr structure to allocate the minor number, and all should be good.

Hi Greg,

Quoted 5 lines

This is still broken in the gsm driver.

As much as I'd like to see someone take ownership of the gsm driver and do this 'the right way', I think until that happens we should consider fixing the reuse-while-in-use error.

What happened to the gsm driver maintainers?

Quoted 2 lines

Would you be willing to take a patch from me that just does the bare minimum to keep this from panicking in the cdev code?

Depends on what the patch looks like :)

Send it on, and I'll be glad to review it.

Quoted 36 lines · Greg Kroah-Hartman

Quoted 34 lines · Peter Hurley

Quoted 17 lines · Greg Kroah-Hartman

Quoted 6 lines · xinhui.pan

Quoted 3 lines

Why can't you do dynamic reference counting of your structure, that would allow you to get rid of your global array, right?

Thanks for your nice comments.
Struct gsm has a ref-count already. :)

Then you should be fine, no need to keep it in an array.

Quoted 2 lines

And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.

You shouldn't need that at all, just use a list, you don't care what the XX number is within the driver, just allocate a new one with the next available number and you should be fine.

Quoted 8 lines

Quoted 1 line

and it can tell us if we can create a new gsm. :)

You should always be able to create a new gsm if you need to :)

Quoted 1 line

In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

Why limit to 256? Just use a list, and a idr structure to allocate the minor number, and all should be good.

Hi Greg,

This is still broken in the gsm driver.

As much as I'd like to see someone take ownership of the gsm driver and do this 'the right way', I think until that happens we should consider fixing the reuse-while-in-use error.

Quoted 1 line

What happened to the gsm driver maintainers?

hahaha

$ ./scripts/get_maintainer.pl -f drivers/tty/n_gsm.c 
Greg Kroah-Hartman <[email protected]> (supporter:TTY LAYER)
Jiri Slaby <[email protected]> (supporter:TTY LAYER)
[email protected] (open list)

Quoted 1 line

What happened to the gsm driver maintainers?

hahaha

$ ./scripts/get_maintainer.pl -f drivers/tty/n_gsm.c 
Greg Kroah-Hartman <[email protected]> (supporter:TTY LAYER)
Jiri Slaby <[email protected]> (supporter:TTY LAYER)
[email protected] (open list)