Re: [PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[Pan Xinhui]

Hi, Alan
thanks for your reply :)

On 2015/12/18 21:17, xinhui wrote:

hi, Alan
this is xinhui. My eyes got badly hurt, and i am ooo this whole week and next 
coming week. sorry for late responce.
I just review the codes in my mind. gsm ioctl callback might change gsm->num, 
so you are right.
i still have many confusion. but tears came out several times:( when i am back, 
i will reply you again.

thx
xinhui



On 2015-12-14 23:40 , One Thousand Gnomes Wrote:

On Mon, 14 Dec 2015 15:08:03 +0800
Pan Xinhui  wrote:


From: Pan Xinhui 

There is one filed gsm->num to store mux's index of gsm_mux[]. So use
gsm->num to remove itself from gsm_mux[] instead of the for-loop
traverse in gsm_cleanup_mux().

Signed-off-by: Pan Xinhui 
---
  drivers/tty/n_gsm.c |   14 +-
  1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9aff371..cf28054 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)

   gsm->dead = 1;

- spin_lock(_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
-  if (gsm_mux[i] == gsm) {
-   gsm_mux[i] = NULL;
-   break;
-  }
- }
- spin_unlock(_mux_lock);
   /* open failed before registering => nothing to do */
- if (i == MAX_MUX)
+ if (gsm_mux[gsm->num] != gsm)
return;

+ spin_lock(_mux_lock);
+ gsm_mux[gsm->num] = NULL;
+ spin_unlock(_mux_lock);


Its a highly theoretical and probably impossible corner case but I can't
help thinking the lock should be held for the if () as well as NULLing
this out.


yes, gsm_mux[] must be touched with gsm_mux_lock held.

I am still wondering if it's possible that two gsm_cleanup_mux() run on the 
same mux.
seems gsmld_config() -> gsm_cleanup_mux() might have race with gsmld_detach_gsm() 
-> gsm_cleanup_mux().
what's more, we need make sure gsm_mux[gsm->num] == gsm, as if there is a new 
mux put into gsm_mux[], we might NULL this new mux out.

here is one possible race.
CPUA  CPUB  CPUC
in cleanup() in cleanup() in 
activate()
if (gsm_mux[gsm->num] != gsm)if (gsm_mux[gsm->num] != gsm)
..  ...
spin_lock(_mux_lock);
gsm_mux[gsm->num] = NULL;
spin_unlock(_mux_lock);

spin_lock(_mux_lock);
...

gsm->num = i;

gsm_mux[i] = gsm;
...

spin_unlock(_mux_lock);
spin_lock(_mux_lock);
gsm_mux[gsm->num] = NULL;//this NULLing might 
cause BUGS!!
spin_unlock(_mux_lock);

I will send out patch V2 to avoid any possible race.
thanks for pointing it out.

thanks
xinhui


Alan



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[Pan Xinhui]

Hi, Alan
thanks for your reply :)

On 2015/12/18 21:17, xinhui wrote:

hi, Alan
this is xinhui. My eyes got badly hurt, and i am ooo this whole week and next 
coming week. sorry for late responce.
I just review the codes in my mind. gsm ioctl callback might change gsm->num, 
so you are right.
i still have many confusion. but tears came out several times:( when i am back, 
i will reply you again.

thx
xinhui



On 2015-12-14 23:40 , One Thousand Gnomes Wrote:

On Mon, 14 Dec 2015 15:08:03 +0800
Pan Xinhui  wrote:


From: Pan Xinhui 

There is one filed gsm->num to store mux's index of gsm_mux[]. So use
gsm->num to remove itself from gsm_mux[] instead of the for-loop
traverse in gsm_cleanup_mux().

Signed-off-by: Pan Xinhui 
---
  drivers/tty/n_gsm.c |   14 +-
  1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9aff371..cf28054 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)

   gsm->dead = 1;

- spin_lock(_mux_lock);
- for (i = 0; i < MAX_MUX; i++) {
-  if (gsm_mux[i] == gsm) {
-   gsm_mux[i] = NULL;
-   break;
-  }
- }
- spin_unlock(_mux_lock);
   /* open failed before registering => nothing to do */
- if (i == MAX_MUX)
+ if (gsm_mux[gsm->num] != gsm)
return;

+ spin_lock(_mux_lock);
+ gsm_mux[gsm->num] = NULL;
+ spin_unlock(_mux_lock);


Its a highly theoretical and probably impossible corner case but I can't
help thinking the lock should be held for the if () as well as NULLing
this out.


yes, gsm_mux[] must be touched with gsm_mux_lock held.

I am still wondering if it's possible that two gsm_cleanup_mux() run on the 
same mux.
seems gsmld_config() -> gsm_cleanup_mux() might have race with gsmld_detach_gsm() 
-> gsm_cleanup_mux().
what's more, we need make sure gsm_mux[gsm->num] == gsm, as if there is a new 
mux put into gsm_mux[], we might NULL this new mux out.

here is one possible race.
CPUA  CPUB  CPUC
in cleanup() in cleanup() in 
activate()
if (gsm_mux[gsm->num] != gsm)if (gsm_mux[gsm->num] != gsm)
..  ...
spin_lock(_mux_lock);
gsm_mux[gsm->num] = NULL;
spin_unlock(_mux_lock);

spin_lock(_mux_lock);
...

gsm->num = i;

gsm_mux[i] = gsm;
...

spin_unlock(_mux_lock);
spin_lock(_mux_lock);
gsm_mux[gsm->num] = NULL;//this NULLing might 
cause BUGS!!
spin_unlock(_mux_lock);

I will send out patch V2 to avoid any possible race.
thanks for pointing it out.

thanks
xinhui


Alan



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[One Thousand Gnomes]

On Mon, 14 Dec 2015 15:08:03 +0800
Pan Xinhui  wrote:

> From: Pan Xinhui 
> 
> There is one filed gsm->num to store mux's index of gsm_mux[]. So use
> gsm->num to remove itself from gsm_mux[] instead of the for-loop
> traverse in gsm_cleanup_mux().
> 
> Signed-off-by: Pan Xinhui 
> ---
>  drivers/tty/n_gsm.c |   14 +-
>  1 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 9aff371..cf28054 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
>  
>   gsm->dead = 1;
>  
> - spin_lock(_mux_lock);
> - for (i = 0; i < MAX_MUX; i++) {
> - if (gsm_mux[i] == gsm) {
> - gsm_mux[i] = NULL;
> - break;
> - }
> - }
> - spin_unlock(_mux_lock);
>   /* open failed before registering => nothing to do */
> - if (i == MAX_MUX)
> + if (gsm_mux[gsm->num] != gsm)
>   return;
>  
> + spin_lock(_mux_lock);
> + gsm_mux[gsm->num] = NULL;
> + spin_unlock(_mux_lock);

Its a highly theoretical and probably impossible corner case but I can't
help thinking the lock should be held for the if () as well as NULLing
this out.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[One Thousand Gnomes]

On Mon, 14 Dec 2015 15:08:03 +0800
Pan Xinhui  wrote:

> From: Pan Xinhui 
> 
> There is one filed gsm->num to store mux's index of gsm_mux[]. So use
> gsm->num to remove itself from gsm_mux[] instead of the for-loop
> traverse in gsm_cleanup_mux().
> 
> Signed-off-by: Pan Xinhui 
> ---
>  drivers/tty/n_gsm.c |   14 +-
>  1 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 9aff371..cf28054 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
>  
>   gsm->dead = 1;
>  
> - spin_lock(_mux_lock);
> - for (i = 0; i < MAX_MUX; i++) {
> - if (gsm_mux[i] == gsm) {
> - gsm_mux[i] = NULL;
> - break;
> - }
> - }
> - spin_unlock(_mux_lock);
>   /* open failed before registering => nothing to do */
> - if (i == MAX_MUX)
> + if (gsm_mux[gsm->num] != gsm)
>   return;
>  
> + spin_lock(_mux_lock);
> + gsm_mux[gsm->num] = NULL;
> + spin_unlock(_mux_lock);

Its a highly theoretical and probably impossible corner case but I can't
help thinking the lock should be held for the if () as well as NULLing
this out.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[Pan Xinhui]

From: Pan Xinhui 

There is one filed gsm->num to store mux's index of gsm_mux[]. So use
gsm->num to remove itself from gsm_mux[] instead of the for-loop
traverse in gsm_cleanup_mux().

Signed-off-by: Pan Xinhui 
---
 drivers/tty/n_gsm.c |   14 +-
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9aff371..cf28054 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 
gsm->dead = 1;
 
-   spin_lock(_mux_lock);
-   for (i = 0; i < MAX_MUX; i++) {
-   if (gsm_mux[i] == gsm) {
-   gsm_mux[i] = NULL;
-   break;
-   }
-   }
-   spin_unlock(_mux_lock);
/* open failed before registering => nothing to do */
-   if (i == MAX_MUX)
+   if (gsm_mux[gsm->num] != gsm)
return;
 
+   spin_lock(_mux_lock);
+   gsm_mux[gsm->num] = NULL;
+   spin_unlock(_mux_lock);
+
/* In theory disconnecting DLCI 0 is sufficient but for some
   modems this is apparently not the case. */
if (dlci) {
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[]

[Pan Xinhui]

From: Pan Xinhui 

There is one filed gsm->num to store mux's index of gsm_mux[]. So use
gsm->num to remove itself from gsm_mux[] instead of the for-loop
traverse in gsm_cleanup_mux().

Signed-off-by: Pan Xinhui 
---
 drivers/tty/n_gsm.c |   14 +-
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9aff371..cf28054 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 
gsm->dead = 1;
 
-   spin_lock(_mux_lock);
-   for (i = 0; i < MAX_MUX; i++) {
-   if (gsm_mux[i] == gsm) {
-   gsm_mux[i] = NULL;
-   break;
-   }
-   }
-   spin_unlock(_mux_lock);
/* open failed before registering => nothing to do */
-   if (i == MAX_MUX)
+   if (gsm_mux[gsm->num] != gsm)
return;
 
+   spin_lock(_mux_lock);
+   gsm_mux[gsm->num] = NULL;
+   spin_unlock(_mux_lock);
+
/* In theory disconnecting DLCI 0 is sufficient but for some
   modems this is apparently not the case. */
if (dlci) {
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/