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

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

[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 <xinhuiX.pan@intel.com>
Reviewed-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
---
 drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 81e7ccb..290df56 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
 }
 
 /**
+ *	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.
+ *	get one entry is just like fill pte, first memory access will
+ *	cause page_fault, the next accesses don't. So do here.
+ */
+
+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;
+	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)
+{
+	if (gsm->num >= MAX_MUX)
+		return;
+
+	spin_lock(&gsm_mux_lock);
+	if (gsm_mux[gsm->num] == gsm)
+		gsm_mux[gsm->num] = NULL;
+	spin_unlock(&gsm_mux_lock);
+}
+
+/**
  *	gsm_cleanup_mux		-	generic GSM protocol cleanup
  *	@gsm: our mux
  *
@@ -2037,16 +2089,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 
 	gsm->dead = 1;
 
-	spin_lock(&gsm_mux_lock);
-	for (i = 0; i < MAX_MUX; i++) {
-		if (gsm_mux[i] == gsm) {
-			gsm_mux[i] = NULL;
-			break;
-		}
-	}
-	spin_unlock(&gsm_mux_lock);
-	WARN_ON(i == MAX_MUX);
-
 	/* In theory disconnecting DLCI 0 is sufficient but for some
 	   modems this is apparently not the case. */
 	if (dlci) {
@@ -2086,7 +2128,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 static int gsm_activate_mux(struct gsm_mux *gsm)
 {
 	struct gsm_dlci *dlci;
-	int i = 0;
+	int ret = 0;
 
 	init_timer(&gsm->t2_timer);
 	gsm->t2_timer.function = gsm_control_retransmit;
@@ -2101,17 +2143,12 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
 		gsm->receive = gsm1_receive;
 	gsm->error = gsm_error;
 
-	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;
+	/*
+	* call gsm_mux_get more than once is safe with same gsm
+	*/
+	ret = gsm_mux_get(gsm);
+	if (ret)
+		return ret;
 
 	dlci = gsm_dlci_alloc(gsm, 0);
 	if (dlci == NULL)
@@ -2142,6 +2179,7 @@ static void gsm_free_mux(struct gsm_mux *gsm)
 static void gsm_free_muxr(struct kref *ref)
 {
 	struct gsm_mux *gsm = container_of(ref, struct gsm_mux, ref);
+	gsm_mux_put(gsm);
 	gsm_free_mux(gsm);
 }
 
@@ -2559,8 +2597,6 @@ static int gsmld_config(struct tty_struct *tty, struct gsm_mux *gsm,
 	if (c->t2)
 		gsm->t2 = c->t2;
 
-	/* FIXME: We need to separate activation/deactivation from adding
-	   and removing from the mux array */
 	if (need_restart)
 		gsm_activate_mux(gsm);
 	if (gsm->initiator && need_close)
-- 
1.7.9.5

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

[Greg Kroah-Hartman]

On Thu, Jul 24, 2014 at 05:17:01PM +0800, xinhui.pan wrote:
> 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 <xinhuiX.pan@intel.com>
> Reviewed-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
> ---
>  drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 81e7ccb..290df56 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
>  }
>  
>  /**
> + *	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 :(

> + *	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?

> + */
> +

blank line?

> +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?

> +	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)
> +{
> +	if (gsm->num >= MAX_MUX)
> +		return;
> +
> +	spin_lock(&gsm_mux_lock);
> +	if (gsm_mux[gsm->num] == gsm)

How can this not be true?

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

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

[xinhui.pan]

hi, Greg

于 2014年07月28日 02:09, Greg Kroah-Hartman 写道:
> On Thu, Jul 24, 2014 at 05:17:01PM +0800, xinhui.pan wrote:
>> 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 <xinhuiX.pan@intel.com>
>> Reviewed-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
>> ---
>>  drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 60 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
>> index 81e7ccb..290df56 100644
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
>>  }
>>  
>>  /**
>> + *	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 :(
> 

Thanks for you nice advices!
I will change its name. This is really a confusable name.

>> + *	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?
> 
>> + */
>> +
> 
> blank line?
> 

Sorry about that. But I notice the other function's introduce in n_gsm.c is done in same way.
I just wanted to keep same code style.
OK, I will change it.
I think no blank line is better. :)

>> +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?
> 

Thanks for pointing out the mistake.
perhaps -EINVAL is better.

>> +	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)
>> +{
>> +	if (gsm->num >= MAX_MUX)
>> +		return;
>> +
>> +	spin_lock(&gsm_mux_lock);
>> +	if (gsm_mux[gsm->num] == gsm)
> 
> How can this not be true?

That is a big problem. let me explain it.

In current code, we add gsm into gsm_mux[] in gsm_activate_mux(). So if gsm_activate_mux() fails when gsm_mux[] is full, gsm->num is invaild, it is zero in fact.
So when will free this gsm, we have to check if gsm_mux[gsm->num] is really the one we are deleting.
The scenario is like below.
gsmld_open -> gsmld_attach_gsm -> gsm_activate_mux(fails) -> .... -> mux_put -> gsm_free_muxr -> gsm_mux_put(need check gsm_mux[gsm->num] == gsm).

Current code is a little hard to understand, So I suggest that move the *codes that changes gsm_mux[]* into gsm_alloc_mux(), then we don't need this check anymore.
Actually I have worked out a new patch with such idea.
And I am doing the normal tests set up by Intel test team and other trigger tests set up by myself. :)

> 
>> +		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 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);*

thanks,

xinhui

> thanks,
> 
> greg k-h
> 

Your reply really helps us a lot.
thanks,

xinhui

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

[Greg Kroah-Hartman]

On Mon, Jul 28, 2014 at 03:16:25PM +0800, xinhui.pan wrote:
> > 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.

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

> 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 :)

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

thanks,

greg k-h

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

[Peter Hurley]

On 07/28/2014 11:13 AM, Greg Kroah-Hartman wrote:
> On Mon, Jul 28, 2014 at 03:16:25PM +0800, xinhui.pan wrote:
>>> 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.
> 
>> 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.
> 
>> 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 :)
> 
>> 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?

Regards,
Peter Hurley

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

[Greg Kroah-Hartman]

On Thu, Oct 09, 2014 at 03:01:46PM -0400, Peter Hurley wrote:
> On 07/28/2014 11:13 AM, Greg Kroah-Hartman wrote:
> > On Mon, Jul 28, 2014 at 03:16:25PM +0800, xinhui.pan wrote:
> >>> 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.
> > 
> >> 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.
> > 
> >> 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 :)
> > 
> >> 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.

What happened to the gsm driver maintainers?

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

thanks,

greg k-h

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

[Peter Hurley]

On 10/09/2014 03:13 PM, Greg Kroah-Hartman wrote:
> On Thu, Oct 09, 2014 at 03:01:46PM -0400, Peter Hurley wrote:
>> On 07/28/2014 11:13 AM, Greg Kroah-Hartman wrote:
>>> On Mon, Jul 28, 2014 at 03:16:25PM +0800, xinhui.pan wrote:
>>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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 :)
>>>
>>>> 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.
> 
> What happened to the gsm driver maintainers?

hahaha

$ ./scripts/get_maintainer.pl -f drivers/tty/n_gsm.c 
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:TTY LAYER)
Jiri Slaby <jslaby@suse.cz> (supporter:TTY LAYER)
linux-kernel@vger.kernel.org (open list)

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

Ok.

Regards,
Peter Hurley

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

[Greg Kroah-Hartman]

On Thu, Oct 09, 2014 at 03:23:49PM -0400, Peter Hurley wrote:
> On 10/09/2014 03:13 PM, Greg Kroah-Hartman wrote:
> > On Thu, Oct 09, 2014 at 03:01:46PM -0400, Peter Hurley wrote:
> >> On 07/28/2014 11:13 AM, Greg Kroah-Hartman wrote:
> >>> On Mon, Jul 28, 2014 at 03:16:25PM +0800, xinhui.pan wrote:
> >>>>> 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.
> >>>
> >>>> 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.
> >>>
> >>>> 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 :)
> >>>
> >>>> 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.
> > 
> > What happened to the gsm driver maintainers?
> 
> hahaha
> 
> $ ./scripts/get_maintainer.pl -f drivers/tty/n_gsm.c 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:TTY LAYER)
> Jiri Slaby <jslaby@suse.cz> (supporter:TTY LAYER)
> linux-kernel@vger.kernel.org (open list)

Damm, yet-another-codebase I'm maintainer of and didn't know it...

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

[One Thousand Gnomes]

> > What happened to the gsm driver maintainers?
> 
> hahaha
> 
> $ ./scripts/get_maintainer.pl -f drivers/tty/n_gsm.c 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:TTY LAYER)
> Jiri Slaby <jslaby@suse.cz> (supporter:TTY LAYER)
> linux-kernel@vger.kernel.org (open list)


We have a bunch of folks in Intel working on some improvements to it. I'll
poke them tomorrow since I think we are one of the main users.

Alan