Re: [PATCH V2] n_gsm: race between ld close and gsmtty open
On Mon, 2013-11-25 at 19:16 -0800, Greg KH wrote: > > > > > > What is different from the previous version? That information needs to > > > be somewhere, otherwise I'm just going to guess and say this is the same > > > as your last one, which was incorrect. > > The difference with previous one is to use a mutex instead of spin lock > > to avoid race, purpose is to avoid sleep in atomic context. I've also > > updated commit a little as above. > > Then be explicit as to what has changed somewhere. We deal with > thousands of patches a week, we can not know that you changed one > sentance in a patch description of a few hundred lines long to know you > made a change to the patch itself as well... OK, in next patch set, I'll highlight difference with previous patch set. -- 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 V2] n_gsm: race between ld close and gsmtty open
On Tue, Nov 26, 2013 at 11:35:14AM +0800, channing wrote: > On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote: > > On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote: > > > > This patch is try to avoid it by: > > > > > > 1) in n_gsm driver, use a global gsm mutex lock to avoid > > > gsm_dlci_release() run in > > > parallel with gsmtty_install(); > The commit is updated here than formal patch set: we use mutex lock in > patch V2, while use spin lock in patch V1. > > > > > > > 2) Increase dlci's ref count in gsmtty_install() instead of in > > > gsmtty_open(), the > > > purpose is to prevent gsm_dlci_release() releasing dlci after > > > gsmtty_install() > > > allocats dlci but before gsmtty_open increases dlci's ref count; > > > > > > 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, > > > this is the > > > opposite process of step 2). > > > > > > Signed-off-by: Chao Bi > > > Signed-off-by: Greg Kroah-Hartman > > > > I have not signed off on this additional patch. > > > > What is different from the previous version? That information needs to > > be somewhere, otherwise I'm just going to guess and say this is the same > > as your last one, which was incorrect. > The difference with previous one is to use a mutex instead of spin lock > to avoid race, purpose is to avoid sleep in atomic context. I've also > updated commit a little as above. Then be explicit as to what has changed somewhere. We deal with thousands of patches a week, we can not know that you changed one sentance in a patch description of a few hundred lines long to know you made a change to the patch itself as well... thanks, greg k-h -- 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 V2] n_gsm: race between ld close and gsmtty open
On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote: > On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote: > > This patch is try to avoid it by: > > > > 1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() > > run in > > parallel with gsmtty_install(); The commit is updated here than formal patch set: we use mutex lock in patch V2, while use spin lock in patch V1. > > > > 2) Increase dlci's ref count in gsmtty_install() instead of in > > gsmtty_open(), the > > purpose is to prevent gsm_dlci_release() releasing dlci after > > gsmtty_install() > > allocats dlci but before gsmtty_open increases dlci's ref count; > > > > 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this > > is the > > opposite process of step 2). > > > > Signed-off-by: Chao Bi > > Signed-off-by: Greg Kroah-Hartman > > I have not signed off on this additional patch. > > What is different from the previous version? That information needs to > be somewhere, otherwise I'm just going to guess and say this is the same > as your last one, which was incorrect. The difference with previous one is to use a mutex instead of spin lock to avoid race, purpose is to avoid sleep in atomic context. I've also updated commit a little as above. > > Also, please fix your "From:" line in your email client to match your > Signed-off-by: line, or else add the proper "From:" line to your patch, > as the Documentation/SubmittingPatches file says. > > Care to try again? Yes, I'll correct it. thanks. > > greg k-h -- 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 V2] n_gsm: race between ld close and gsmtty open
On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote:
>
> ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
> to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
> is opening in parallel.
>
> Here are race cases we found recently in test:
>
> CASE #1
>
> releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
> in gsmtty_open(gsmttyB), as below:
>
> tty_release(ttyA) tty_open(gsmttyB)
> | |
>- gsmtty_install(gsmttyB)
> | |
>-gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
> tty_ldisc_release(ttyA) -
> | |
> gsm_dlci_release(dlci[B]) -
> | |
> gsm_dlci_free(dlci[B])-
> | |
>- gsmtty_open(gsmttyB)
>
> gsmtty_open()
> {
> struct gsm_dlci *dlci = tty->driver_data; => here it uses dlci[B]
> ...
> }
>
> In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
> =
>
> CASE #2
> =
> releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
> in gsmtty_open(), as below:
>
> tty_release(ttyA) tty_open(gsmttyB)
> | |
>- gsmtty_install(gsmttyB)
> | |
>-gsm_dlci_alloc(gsmttyB) => alloc dlci[B]
> | |
>- gsmtty_open(gsmttyB) fail
> | |
>- tty_release(gsmttyB)
> | |
>- gsmtty_close(gsmttyB)
> | |
>-gsmtty_detach_dlci(dlci[B])
> | |
>- dlci_put(dlci[B])
> | |
> tty_ldisc_release(ttyA) -
> | |
> gsm_dlci_release(dlci[0]) -
> | |
> gsm_dlci_free(dlci[0])-
> | |
>- dlci_put(dlci[0])
>
> In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
> then hit panic.
> =
>
> IMHO, n_gsm tty operations would refer released ldisc, as long as
> gsm_dlci_release() has chance to release ldisc data when some gsmtty
> operations
> are ongoing..
>
> This patch is try to avoid it by:
>
> 1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release()
> run in
> parallel with gsmtty_install();
>
> 2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(),
> the
> purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
> allocats dlci but before gsmtty_open increases dlci's ref count;
>
> 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is
> the
> opposite process of step 2).
>
> Signed-off-by: Chao Bi
> Signed-off-by: Greg Kroah-Hartman
I have not signed off on this additional patch.
What is different from the previous version? That information needs to
be somewhere, otherwise I'm just going to guess and say this is the same
as your last one, which was incorrect.
Also, please fix your "From:" line in your email client to match your
Signed-off-by: line, or else add the proper "From:" line to your patch,
as the Documentation/SubmittingPatches file says.
Care to try again?
greg k-h
--
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 V2] n_gsm: race between ld close and gsmtty open
On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote:
ttyA has ld associated to n_gsm, when ttyA is closing, it triggers
to release gsmttyB's ld data dlci[B], then race would happen if gsmttyB
is opening in parallel.
Here are race cases we found recently in test:
CASE #1
releasing dlci[B] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(gsmttyB), as below:
tty_release(ttyA) tty_open(gsmttyB)
| |
- gsmtty_install(gsmttyB)
| |
-gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
tty_ldisc_release(ttyA) -
| |
gsm_dlci_release(dlci[B]) -
| |
gsm_dlci_free(dlci[B])-
| |
- gsmtty_open(gsmttyB)
gsmtty_open()
{
struct gsm_dlci *dlci = tty-driver_data; = here it uses dlci[B]
...
}
In gsmtty_open(gsmttyA), it uses dlci[B] which was release, so hit a panic.
=
CASE #2
=
releasing dlci[0] race with gsmtty_install(gsmttyB), then panic
in gsmtty_open(), as below:
tty_release(ttyA) tty_open(gsmttyB)
| |
- gsmtty_install(gsmttyB)
| |
-gsm_dlci_alloc(gsmttyB) = alloc dlci[B]
| |
- gsmtty_open(gsmttyB) fail
| |
- tty_release(gsmttyB)
| |
- gsmtty_close(gsmttyB)
| |
-gsmtty_detach_dlci(dlci[B])
| |
- dlci_put(dlci[B])
| |
tty_ldisc_release(ttyA) -
| |
gsm_dlci_release(dlci[0]) -
| |
gsm_dlci_free(dlci[0])-
| |
- dlci_put(dlci[0])
In gsmtty_detach_dlci(dlci[B]), it tries to use dlci[0] which was released,
then hit panic.
=
IMHO, n_gsm tty operations would refer released ldisc, as long as
gsm_dlci_release() has chance to release ldisc data when some gsmtty
operations
are ongoing..
This patch is try to avoid it by:
1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release()
run in
parallel with gsmtty_install();
2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(),
the
purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install()
allocats dlci but before gsmtty_open increases dlci's ref count;
3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is
the
opposite process of step 2).
Signed-off-by: Chao Bi chao...@intel.com
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
I have not signed off on this additional patch.
What is different from the previous version? That information needs to
be somewhere, otherwise I'm just going to guess and say this is the same
as your last one, which was incorrect.
Also, please fix your From: line in your email client to match your
Signed-off-by: line, or else add the proper From: line to your patch,
as the Documentation/SubmittingPatches file says.
Care to try again?
greg k-h
--
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 V2] n_gsm: race between ld close and gsmtty open
On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote: On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote: This patch is try to avoid it by: 1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() run in parallel with gsmtty_install(); The commit is updated here than formal patch set: we use mutex lock in patch V2, while use spin lock in patch V1. 2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), the purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install() allocats dlci but before gsmtty_open increases dlci's ref count; 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is the opposite process of step 2). Signed-off-by: Chao Bi chao...@intel.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org I have not signed off on this additional patch. What is different from the previous version? That information needs to be somewhere, otherwise I'm just going to guess and say this is the same as your last one, which was incorrect. The difference with previous one is to use a mutex instead of spin lock to avoid race, purpose is to avoid sleep in atomic context. I've also updated commit a little as above. Also, please fix your From: line in your email client to match your Signed-off-by: line, or else add the proper From: line to your patch, as the Documentation/SubmittingPatches file says. Care to try again? Yes, I'll correct it. thanks. greg k-h -- 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 V2] n_gsm: race between ld close and gsmtty open
On Tue, Nov 26, 2013 at 11:35:14AM +0800, channing wrote: On Mon, 2013-11-25 at 18:54 -0800, Greg KH wrote: On Tue, Nov 26, 2013 at 11:14:05AM +0800, channing wrote: This patch is try to avoid it by: 1) in n_gsm driver, use a global gsm mutex lock to avoid gsm_dlci_release() run in parallel with gsmtty_install(); The commit is updated here than formal patch set: we use mutex lock in patch V2, while use spin lock in patch V1. 2) Increase dlci's ref count in gsmtty_install() instead of in gsmtty_open(), the purpose is to prevent gsm_dlci_release() releasing dlci after gsmtty_install() allocats dlci but before gsmtty_open increases dlci's ref count; 3) Decrease dlci's ref count in gsmtty_remove(), a tty framework API, this is the opposite process of step 2). Signed-off-by: Chao Bi chao...@intel.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org I have not signed off on this additional patch. What is different from the previous version? That information needs to be somewhere, otherwise I'm just going to guess and say this is the same as your last one, which was incorrect. The difference with previous one is to use a mutex instead of spin lock to avoid race, purpose is to avoid sleep in atomic context. I've also updated commit a little as above. Then be explicit as to what has changed somewhere. We deal with thousands of patches a week, we can not know that you changed one sentance in a patch description of a few hundred lines long to know you made a change to the patch itself as well... thanks, greg k-h -- 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 V2] n_gsm: race between ld close and gsmtty open
On Mon, 2013-11-25 at 19:16 -0800, Greg KH wrote: What is different from the previous version? That information needs to be somewhere, otherwise I'm just going to guess and say this is the same as your last one, which was incorrect. The difference with previous one is to use a mutex instead of spin lock to avoid race, purpose is to avoid sleep in atomic context. I've also updated commit a little as above. Then be explicit as to what has changed somewhere. We deal with thousands of patches a week, we can not know that you changed one sentance in a patch description of a few hundred lines long to know you made a change to the patch itself as well... OK, in next patch set, I'll highlight difference with previous patch set. -- 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/
