Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On 10/10/2014 12:05 AM, Andre Wolokita wrote:
On 10/10/14 00:47, Peter Hurley wrote:
Hi Andre,
On 10/08/2014 11:54 PM, Andre Wolokita wrote:
On 09/10/14 14:38, Greg KH wrote:
On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Yes.
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
Hm, I dug into the tty core, it should be ok, but it just seems really
odd, and bad-form to be doing something with port-port after calling a
destroy function with it, don't you agree?
I do. The call to wait_event() can be removed as we no longer care whether
gs_closed(port) is returning true - if it even can, having destroyed the
tty_port.
Neither the patch nor the idea that this wait_event() is unnecessary is
correct (within the current design of u_serial).
tty_port_destroy() frees the input processing buffers, which may be in use
right at this moment, if a port is still in use by its tty. Since whatever
was using those buffers doesn't know this has happened, it may still be
writing
to that memory, which may now be reallocated to some other kernel subsystem,
like
file cache buffers.
The wait_event() tries to ensure that the port destruction can't take place
while the port is still in use, so when it's hung there, it's preventing bad
things from happening.
There is no way to simply 'force' the port to no longer be in use, since a
userspace process can maintain an open file indefinitely.
There are a couple of possible solutions though:
In gserial_free_line(), hangup the tty associated with the port. You can
look at usb_serial_disconnect() for how to do that properly. This doesn't
guarantee that the userspace process will close the tty, but most programs
will close the file on end-of-file read (which is what hangup will cause).
I tried adding the adding the same tty_vhangup(tty) and tty_kref_put(tty)
logic that usb serial has
but the problem still occurs.
That's because g_serial has no hangup() method; ports aren't going to
close themselves.
It doesn't look like making the wait_event() interruptible is possible
(or desirable).
The ideal solution would be for u_serial to reference count its gs_ports;
that's what usb serial does for its usb_serial_port. Then gserial_free_line()
becomes a 'disconnect', and the actual cleanup happens later. The key
implementation details are:
1. The tty core helps keep reference counting simple by calling the tty
driver's install() and cleanup() methods; install() for the first open() and
cleanup() when the tty is being released. usb serial does this with
usb_serial_port_get_by_minor() from serial_install() and usb_serial_put() in
serial_cleanup() and usb_serial_disconnect().
2. a flag (like usb serial's '-disconnected') to prevent continued port
allocation after 'disconnect'.
The key concept is that although the tty and port still exist, neither
does anything useful after the disconnect.
And u_serial.c should really be ported to using tty_port which takes care of
stuff like parallel opens and closes without looping and other stuff like
the port-openclose flag.
FWIW, the tty_unregister_device() does not need to be after
gserial_free_port()
because existing ttys maintain a device reference count which prevents the
underlying tty device from being released.
To be honest with you, I am in way over my head at the moment. I'll continue
working on this problem but I can't guarantee it'll be fixed any time soon.
Maybe you should reconsider if this use-case is really worth
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On Thu, 9 Oct 2014, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
You know, this last paragraph doesn't explain anything. The existing
code already calls tty_port_destroy; all you did was move that call
down a couple of lines. You need to explain why _moving_ the function
call is important, not why _calling_ the function is important.
Alan Stern
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
WARN_ON(port-port_usb != NULL);
- tty_port_destroy(port-port);
kfree(port);
}
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On Thu, 9 Oct 2014, Andre Wolokita wrote: Isn't this now a use-after-free issue? Are you referring to the subsequent call to wait event() on gs_closed()? Yes. Testing the use-case with this patch applied seemed to work without any issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r but I'm just a newbie, so I could be doing sometime horrible here. Hm, I dug into the tty core, it should be ok, but it just seems really odd, and bad-form to be doing something with port-port after calling a destroy function with it, don't you agree? I do. The call to wait_event() can be removed as we no longer care whether gs_closed(port) is returning true - if it even can, having destroyed the tty_port. Maybe you don't care whether gs_closed(port) returns true, but you should care about whether gs_closed() crashes -- which it might well do if it tries to access deallocated memory. Did you test your patch by unloading the module while there were pending opens? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
Hi Andre,
On 10/08/2014 11:54 PM, Andre Wolokita wrote:
On 09/10/14 14:38, Greg KH wrote:
On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Yes.
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
Hm, I dug into the tty core, it should be ok, but it just seems really
odd, and bad-form to be doing something with port-port after calling a
destroy function with it, don't you agree?
I do. The call to wait_event() can be removed as we no longer care whether
gs_closed(port) is returning true - if it even can, having destroyed the
tty_port.
Neither the patch nor the idea that this wait_event() is unnecessary is
correct (within the current design of u_serial).
tty_port_destroy() frees the input processing buffers, which may be in use
right at this moment, if a port is still in use by its tty. Since whatever
was using those buffers doesn't know this has happened, it may still be writing
to that memory, which may now be reallocated to some other kernel subsystem,
like
file cache buffers.
The wait_event() tries to ensure that the port destruction can't take place
while the port is still in use, so when it's hung there, it's preventing bad
things from happening.
There is no way to simply 'force' the port to no longer be in use, since a
userspace process can maintain an open file indefinitely.
There are a couple of possible solutions though:
In gserial_free_line(), hangup the tty associated with the port. You can
look at usb_serial_disconnect() for how to do that properly. This doesn't
guarantee that the userspace process will close the tty, but most programs
will close the file on end-of-file read (which is what hangup will cause).
It doesn't look like making the wait_event() interruptible is possible
(or desirable).
The ideal solution would be for u_serial to reference count its gs_ports;
that's what usb serial does for its usb_serial_port. Then gserial_free_line()
becomes a 'disconnect', and the actual cleanup happens later. The key
implementation details are:
1. The tty core helps keep reference counting simple by calling the tty
driver's install() and cleanup() methods; install() for the first open() and
cleanup() when the tty is being released. usb serial does this with
usb_serial_port_get_by_minor() from serial_install() and usb_serial_put() in
serial_cleanup() and usb_serial_disconnect().
2. a flag (like usb serial's '-disconnected') to prevent continued port
allocation after 'disconnect'.
The key concept is that although the tty and port still exist, neither
does anything useful after the disconnect.
And u_serial.c should really be ported to using tty_port which takes care of
stuff like parallel opens and closes without looping and other stuff like
the port-openclose flag.
FWIW, the tty_unregister_device() does not need to be after gserial_free_port()
because existing ttys maintain a device reference count which prevents the
underlying tty device from being released.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On 10/09/2014 10:26 AM, Alan Stern wrote: On Thu, 9 Oct 2014, Andre Wolokita wrote: Isn't this now a use-after-free issue? Are you referring to the subsequent call to wait event() on gs_closed()? Yes. Testing the use-case with this patch applied seemed to work without any issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r but I'm just a newbie, so I could be doing sometime horrible here. Hm, I dug into the tty core, it should be ok, but it just seems really odd, and bad-form to be doing something with port-port after calling a destroy function with it, don't you agree? I do. The call to wait_event() can be removed as we no longer care whether gs_closed(port) is returning true - if it even can, having destroyed the tty_port. Maybe you don't care whether gs_closed(port) returns true, but you should care about whether gs_closed() crashes -- which it might well do if it tries to access deallocated memory. Did you test your patch by unloading the module while there were pending opens? gs_closed() won't crash because that's not the memory being deallocated. tty_port_destroy() is somewhat misnamed; should be more like tty_port_cleanup(), as it doesn't actually deallocate the port (because tty_port could be an embedded structure in a larger 'port' which is the most common usage). I've been struggling to understand how this patch could even cause the wait_event() to complete and the only thing I can hypothesize is that the deallocation in tty_port_destroy() somehow causes the process with the open tty to crash but I don't see how. Regards, Peter Hurley PS - My earlier email was greylisted so that's why I appeared to reply well after your answer :) X-Greylist: delayed 4693 seconds by postgrey-1.27 at vger.kernel.org; Thu, 09 Oct 2014 11:06:05 EDT -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On 10/10/14 00:47, Peter Hurley wrote:
Hi Andre,
On 10/08/2014 11:54 PM, Andre Wolokita wrote:
On 09/10/14 14:38, Greg KH wrote:
On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Yes.
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
Hm, I dug into the tty core, it should be ok, but it just seems really
odd, and bad-form to be doing something with port-port after calling a
destroy function with it, don't you agree?
I do. The call to wait_event() can be removed as we no longer care whether
gs_closed(port) is returning true - if it even can, having destroyed the
tty_port.
Neither the patch nor the idea that this wait_event() is unnecessary is
correct (within the current design of u_serial).
tty_port_destroy() frees the input processing buffers, which may be in use
right at this moment, if a port is still in use by its tty. Since whatever
was using those buffers doesn't know this has happened, it may still be
writing
to that memory, which may now be reallocated to some other kernel subsystem,
like
file cache buffers.
The wait_event() tries to ensure that the port destruction can't take place
while the port is still in use, so when it's hung there, it's preventing bad
things from happening.
There is no way to simply 'force' the port to no longer be in use, since a
userspace process can maintain an open file indefinitely.
There are a couple of possible solutions though:
In gserial_free_line(), hangup the tty associated with the port. You can
look at usb_serial_disconnect() for how to do that properly. This doesn't
guarantee that the userspace process will close the tty, but most programs
will close the file on end-of-file read (which is what hangup will cause).
I tried adding the adding the same tty_vhangup(tty) and tty_kref_put(tty) logic
that usb serial has
but the problem still occurs.
It doesn't look like making the wait_event() interruptible is possible
(or desirable).
The ideal solution would be for u_serial to reference count its gs_ports;
that's what usb serial does for its usb_serial_port. Then gserial_free_line()
becomes a 'disconnect', and the actual cleanup happens later. The key
implementation details are:
1. The tty core helps keep reference counting simple by calling the tty
driver's install() and cleanup() methods; install() for the first open() and
cleanup() when the tty is being released. usb serial does this with
usb_serial_port_get_by_minor() from serial_install() and usb_serial_put() in
serial_cleanup() and usb_serial_disconnect().
2. a flag (like usb serial's '-disconnected') to prevent continued port
allocation after 'disconnect'.
The key concept is that although the tty and port still exist, neither
does anything useful after the disconnect.
And u_serial.c should really be ported to using tty_port which takes care of
stuff like parallel opens and closes without looping and other stuff like
the port-openclose flag.
FWIW, the tty_unregister_device() does not need to be after
gserial_free_port()
because existing ttys maintain a device reference count which prevents the
underlying tty device from being released.
To be honest with you, I am in way over my head at the moment. I'll continue
working on this problem but I can't guarantee it'll be fixed any time soon.
Regards,
Andre.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
Hi,
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
does anybody know if this is a valid usecase ? I'd expect us to return
-EBUSY in such a case, no ? In any case, using g_serial to rmmod
g_serial is, from what I can tell, not supported.
Let's wait to hear from Alan Cox or somebody else.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
WARN_ON(port-port_usb != NULL);
- tty_port_destroy(port-port);
kfree(port);
}
--
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Yes.
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
Hm, I dug into the tty core, it should be ok, but it just seems really
odd, and bad-form to be doing something with port-port after calling a
destroy function with it, don't you agree?
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] u_serial.c - Force destroy tty_port in gserial_free_port
On 09/10/14 14:38, Greg KH wrote:
On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
On 09/10/14 13:56, Greg KH wrote:
On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
Issuing a modprobe -r g_serial command to the target
over the gadget serial communications line causes
modprobe to enter uninterruptable sleep, leaving the
system in an unstable state.
The associated tty_port.count won't drop to 0 because
the command is issued over the very line being removed.
Destroying the tty_port will ensure that resources are
freed and modprobe will not hang.
Signed-off-by: Andre Wolokita andre.wolok...@analog.com
---
drivers/usb/gadget/function/u_serial.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..db631c4 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
static void gserial_free_port(struct gs_port *port)
{
tasklet_kill(port-push);
+ tty_port_destroy(port-port);
/* wait for old opens to finish */
wait_event(port-port.close_wait, gs_closed(port));
Isn't this now a use-after-free issue?
Are you referring to the subsequent call to wait event() on gs_closed()?
Yes.
Testing the use-case with this patch applied seemed to work without any
issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
but I'm just a newbie, so I could be doing sometime horrible here.
Hm, I dug into the tty core, it should be ok, but it just seems really
odd, and bad-form to be doing something with port-port after calling a
destroy function with it, don't you agree?
I do. The call to wait_event() can be removed as we no longer care whether
gs_closed(port) is returning true - if it even can, having destroyed the
tty_port.
cheers.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
