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.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.
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 supporting;
I'm not really even sure if you get past the g_serial driver issues that
the tty core will cooperate with what you're trying to accomplish.
If this works, how are you going to get back on the console?
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
