[xf86-input-libinput] If the parent libinput_device is unavailable, create a new one

Submitted by Peter Hutterer on Nov. 15, 2016, 4:37 a.m.

Details

Message ID 20161115043744.GA25383@jelly
State Accepted
Headers show
Series "If the parent libinput_device is unavailable, create a new one" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Peter Hutterer Nov. 15, 2016, 4:37 a.m.
The parent device ref's the libinput device during pre_init and unref's it
during DEVICE_INIT, so the copy is lost. During DEVICE_ON, the libinput device
is re-added and ref'd, this one stays around now. But the takeaway is: unless
the device is enabled, no libinput device reference is available.

If a device is a mixed pointer + keyboard device, a subdevice is created
during a WorkProc. The subdevice relied on the parent's libinput_device being
available and didn't even check for it. This WorkProc usually runs after
the parent's DEVICE_ON, so in most cases all is well.

But when running without logind and the server is vt-switched away, the parent
device only runs PreInit and DEVICE_INIT but never DEVICE_ON, causing the
subdevice to burn, crash, and generally fail horribly when it dereferences the
parent's libinput device.

Fix this because we have global warming already and don't need to burn more
things and also because it's considered bad user experience to have the
server crash. The simple fix is to check the parent device first and if it is
unavailable, create a new one because it will end up disabled as well anyway,
so the ref goes away as well. The use-case where the parent somehow gets
disabled but the subdevice doesn't is a bit too niche to worry about.

This doesn't happen with logind because in that case we don't get a usable fd
while VT-switched away, so we can't even run PreInit and never get this far
(see the paused fd handling in the xfree86 code for that). It can be
reproduced by setting AutoEnableDevices off, but why would you do that,
seriously.

https://bugs.freedesktop.org/show_bug.cgi?id=97117

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 src/xf86libinput.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 6792d1c..747e84b 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -2850,7 +2850,7 @@  xf86libinput_pre_init(InputDriverPtr drv,
 	struct xf86libinput *driver_data = NULL;
 	struct xf86libinput_device *shared_device = NULL;
 	struct libinput *libinput = NULL;
-	struct libinput_device *device;
+	struct libinput_device *device = NULL;
 	char *path = NULL;
 	bool is_subdevice;
 
@@ -2885,7 +2885,28 @@  xf86libinput_pre_init(InputDriverPtr drv,
 	}
 
 	is_subdevice = xf86libinput_is_subdevice(pInfo);
-	if (!is_subdevice) {
+	if (is_subdevice) {
+		InputInfoPtr parent;
+		struct xf86libinput *parent_driver_data;
+
+		parent = xf86libinput_get_parent(pInfo);
+		if (!parent) {
+			xf86IDrvMsg(pInfo, X_ERROR, "Failed to find parent device\n");
+			goto fail;
+		}
+
+		parent_driver_data = parent->private;
+		if (!parent_driver_data) /* parent already removed again */
+			goto fail;
+
+		xf86IDrvMsg(pInfo, X_INFO, "is a virtual subdevice\n");
+		shared_device = xf86libinput_shared_ref(parent_driver_data->shared_device);
+		device = shared_device->device;
+		if (!device)
+			xf86IDrvMsg(pInfo, X_ERROR, "Parent device not available\n");
+	}
+
+	if (!device) {
 		device = libinput_path_add_device(libinput, path);
 		if (!device) {
 			xf86IDrvMsg(pInfo, X_ERROR, "Failed to create a device for %s\n", path);
@@ -2903,23 +2924,6 @@  xf86libinput_pre_init(InputDriverPtr drv,
 			libinput_device_unref(device);
 			goto fail;
 		}
-	} else {
-		InputInfoPtr parent;
-		struct xf86libinput *parent_driver_data;
-
-		parent = xf86libinput_get_parent(pInfo);
-		if (!parent) {
-			xf86IDrvMsg(pInfo, X_ERROR, "Failed to find parent device\n");
-			goto fail;
-		}
-
-		parent_driver_data = parent->private;
-		if (!parent_driver_data) /* parent already removed again */
-			goto fail;
-
-		xf86IDrvMsg(pInfo, X_INFO, "is a virtual subdevice\n");
-		shared_device = xf86libinput_shared_ref(parent_driver_data->shared_device);
-		device = shared_device->device;
 	}
 
 	pInfo->private = driver_data;

Comments

Hi,

On 15-11-16 05:37, Peter Hutterer wrote:
> The parent device ref's the libinput device during pre_init and unref's it
> during DEVICE_INIT, so the copy is lost. During DEVICE_ON, the libinput device
> is re-added and ref'd, this one stays around now. But the takeaway is: unless
> the device is enabled, no libinput device reference is available.
>
> If a device is a mixed pointer + keyboard device, a subdevice is created
> during a WorkProc. The subdevice relied on the parent's libinput_device being
> available and didn't even check for it. This WorkProc usually runs after
> the parent's DEVICE_ON, so in most cases all is well.
>
> But when running without logind and the server is vt-switched away, the parent
> device only runs PreInit and DEVICE_INIT but never DEVICE_ON, causing the
> subdevice to burn, crash, and generally fail horribly when it dereferences the
> parent's libinput device.
>
> Fix this because we have global warming already and don't need to burn more
> things and also because it's considered bad user experience to have the
> server crash. The simple fix is to check the parent device first and if it is
> unavailable, create a new one because it will end up disabled as well anyway,
> so the ref goes away as well. The use-case where the parent somehow gets
> disabled but the subdevice doesn't is a bit too niche to worry about.
>
> This doesn't happen with logind because in that case we don't get a usable fd
> while VT-switched away, so we can't even run PreInit and never get this far
> (see the paused fd handling in the xfree86 code for that). It can be
> reproduced by setting AutoEnableDevices off, but why would you do that,
> seriously.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=97117
>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Hmm, so what happens if the parent device later does get DEVICE_ON, after
the subdevice has created its own libinputdevice ?

Regards,

Hans



> ---
>  src/xf86libinput.c | 42 +++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 6792d1c..747e84b 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -2850,7 +2850,7 @@ xf86libinput_pre_init(InputDriverPtr drv,
>  	struct xf86libinput *driver_data = NULL;
>  	struct xf86libinput_device *shared_device = NULL;
>  	struct libinput *libinput = NULL;
> -	struct libinput_device *device;
> +	struct libinput_device *device = NULL;
>  	char *path = NULL;
>  	bool is_subdevice;
>
> @@ -2885,7 +2885,28 @@ xf86libinput_pre_init(InputDriverPtr drv,
>  	}
>
>  	is_subdevice = xf86libinput_is_subdevice(pInfo);
> -	if (!is_subdevice) {
> +	if (is_subdevice) {
> +		InputInfoPtr parent;
> +		struct xf86libinput *parent_driver_data;
> +
> +		parent = xf86libinput_get_parent(pInfo);
> +		if (!parent) {
> +			xf86IDrvMsg(pInfo, X_ERROR, "Failed to find parent device\n");
> +			goto fail;
> +		}
> +
> +		parent_driver_data = parent->private;
> +		if (!parent_driver_data) /* parent already removed again */
> +			goto fail;
> +
> +		xf86IDrvMsg(pInfo, X_INFO, "is a virtual subdevice\n");
> +		shared_device = xf86libinput_shared_ref(parent_driver_data->shared_device);
> +		device = shared_device->device;
> +		if (!device)
> +			xf86IDrvMsg(pInfo, X_ERROR, "Parent device not available\n");
> +	}
> +
> +	if (!device) {
>  		device = libinput_path_add_device(libinput, path);
>  		if (!device) {
>  			xf86IDrvMsg(pInfo, X_ERROR, "Failed to create a device for %s\n", path);
> @@ -2903,23 +2924,6 @@ xf86libinput_pre_init(InputDriverPtr drv,
>  			libinput_device_unref(device);
>  			goto fail;
>  		}
> -	} else {
> -		InputInfoPtr parent;
> -		struct xf86libinput *parent_driver_data;
> -
> -		parent = xf86libinput_get_parent(pInfo);
> -		if (!parent) {
> -			xf86IDrvMsg(pInfo, X_ERROR, "Failed to find parent device\n");
> -			goto fail;
> -		}
> -
> -		parent_driver_data = parent->private;
> -		if (!parent_driver_data) /* parent already removed again */
> -			goto fail;
> -
> -		xf86IDrvMsg(pInfo, X_INFO, "is a virtual subdevice\n");
> -		shared_device = xf86libinput_shared_ref(parent_driver_data->shared_device);
> -		device = shared_device->device;
>  	}
>
>  	pInfo->private = driver_data;
>
On Tue, Nov 15, 2016 at 10:35:31AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 15-11-16 05:37, Peter Hutterer wrote:
> > The parent device ref's the libinput device during pre_init and unref's it
> > during DEVICE_INIT, so the copy is lost. During DEVICE_ON, the libinput device
> > is re-added and ref'd, this one stays around now. But the takeaway is: unless
> > the device is enabled, no libinput device reference is available.
> > 
> > If a device is a mixed pointer + keyboard device, a subdevice is created
> > during a WorkProc. The subdevice relied on the parent's libinput_device being
> > available and didn't even check for it. This WorkProc usually runs after
> > the parent's DEVICE_ON, so in most cases all is well.
> > 
> > But when running without logind and the server is vt-switched away, the parent
> > device only runs PreInit and DEVICE_INIT but never DEVICE_ON, causing the
> > subdevice to burn, crash, and generally fail horribly when it dereferences the
> > parent's libinput device.
> > 
> > Fix this because we have global warming already and don't need to burn more
> > things and also because it's considered bad user experience to have the
> > server crash. The simple fix is to check the parent device first and if it is
> > unavailable, create a new one because it will end up disabled as well anyway,
> > so the ref goes away as well. The use-case where the parent somehow gets
> > disabled but the subdevice doesn't is a bit too niche to worry about.
> > 
> > This doesn't happen with logind because in that case we don't get a usable fd
> > while VT-switched away, so we can't even run PreInit and never get this far
> > (see the paused fd handling in the xfree86 code for that). It can be
> > reproduced by setting AutoEnableDevices off, but why would you do that,
> > seriously.
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=97117
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> 
> Hmm, so what happens if the parent device later does get DEVICE_ON, after
> the subdevice has created its own libinputdevice ?

we don't differ between parent and subdevices during DEVICE_ON, whichever
one is the first adds the device and the rest just refcounts it. so we
should be good here.

Cheers,
   Peter
Hi,

On 18-11-16 07:47, Peter Hutterer wrote:
> On Tue, Nov 15, 2016 at 10:35:31AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 15-11-16 05:37, Peter Hutterer wrote:
>>> The parent device ref's the libinput device during pre_init and unref's it
>>> during DEVICE_INIT, so the copy is lost. During DEVICE_ON, the libinput device
>>> is re-added and ref'd, this one stays around now. But the takeaway is: unless
>>> the device is enabled, no libinput device reference is available.
>>>
>>> If a device is a mixed pointer + keyboard device, a subdevice is created
>>> during a WorkProc. The subdevice relied on the parent's libinput_device being
>>> available and didn't even check for it. This WorkProc usually runs after
>>> the parent's DEVICE_ON, so in most cases all is well.
>>>
>>> But when running without logind and the server is vt-switched away, the parent
>>> device only runs PreInit and DEVICE_INIT but never DEVICE_ON, causing the
>>> subdevice to burn, crash, and generally fail horribly when it dereferences the
>>> parent's libinput device.
>>>
>>> Fix this because we have global warming already and don't need to burn more
>>> things and also because it's considered bad user experience to have the
>>> server crash. The simple fix is to check the parent device first and if it is
>>> unavailable, create a new one because it will end up disabled as well anyway,
>>> so the ref goes away as well. The use-case where the parent somehow gets
>>> disabled but the subdevice doesn't is a bit too niche to worry about.
>>>
>>> This doesn't happen with logind because in that case we don't get a usable fd
>>> while VT-switched away, so we can't even run PreInit and never get this far
>>> (see the paused fd handling in the xfree86 code for that). It can be
>>> reproduced by setting AutoEnableDevices off, but why would you do that,
>>> seriously.
>>>
>>> https://bugs.freedesktop.org/show_bug.cgi?id=97117
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
>>
>> Hmm, so what happens if the parent device later does get DEVICE_ON, after
>> the subdevice has created its own libinputdevice ?
>
> we don't differ between parent and subdevices during DEVICE_ON, whichever
> one is the first adds the device and the rest just refcounts it. so we
> should be good here.

Ok, in that case, the original patch looks good to me and is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans