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.Wolokita@xxxxxxxxxx>
>>>>> ---
>>>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
