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