* [PATCH] Input: evdev - Use EBADFD for flush() errors @ 2015-08-19 7:38 Takashi Iwai 2015-09-01 5:23 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2015-08-19 7:38 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Al Viro, linux-input We've got bug reports showing the old systemd-logind (at least system-210) aborting unexpectedly, and this turned out to be because of an invalid error code from close() call to evdev devices. close() is supposed to return only either EINTR or EBADFD, while the device returned ENODEV. logind was overreacting to it and decided to kill itself when an unexpected error code was received. What a tragedy. The bad error code comes from flush fops, and actually evdev_flush() returns -ENODEV and else. This patch papers over it, simply fixing the error return code to the acceptable values above. Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/input/evdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 9d35499faca4..28e9efd837e1 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -302,7 +302,7 @@ static int evdev_flush(struct file *file, fl_owner_t id) retval = input_flush_device(&evdev->handle, file); mutex_unlock(&evdev->mutex); - return retval; + return retval < 0 ? -EBADFD : 0; } static void evdev_free(struct device *dev) -- 2.5.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Input: evdev - Use EBADFD for flush() errors 2015-08-19 7:38 [PATCH] Input: evdev - Use EBADFD for flush() errors Takashi Iwai @ 2015-09-01 5:23 ` Takashi Iwai 2015-09-04 5:37 ` Dmitry Torokhov 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2015-09-01 5:23 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Al Viro, linux-input On Wed, 19 Aug 2015 09:38:15 +0200, Takashi Iwai wrote: > > We've got bug reports showing the old systemd-logind (at least > system-210) aborting unexpectedly, and this turned out to be because > of an invalid error code from close() call to evdev devices. close() > is supposed to return only either EINTR or EBADFD, while the device > returned ENODEV. logind was overreacting to it and decided to kill > itself when an unexpected error code was received. What a tragedy. > > The bad error code comes from flush fops, and actually evdev_flush() > returns -ENODEV and else. This patch papers over it, simply fixing > the error return code to the acceptable values above. > > Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> Hi, any comments on this patch? thanks, Takashi > --- > drivers/input/evdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c > index 9d35499faca4..28e9efd837e1 100644 > --- a/drivers/input/evdev.c > +++ b/drivers/input/evdev.c > @@ -302,7 +302,7 @@ static int evdev_flush(struct file *file, fl_owner_t id) > retval = input_flush_device(&evdev->handle, file); > > mutex_unlock(&evdev->mutex); > - return retval; > + return retval < 0 ? -EBADFD : 0; > } > > static void evdev_free(struct device *dev) > -- > 2.5.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Input: evdev - Use EBADFD for flush() errors 2015-09-01 5:23 ` Takashi Iwai @ 2015-09-04 5:37 ` Dmitry Torokhov 2015-09-04 5:42 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Torokhov @ 2015-09-04 5:37 UTC (permalink / raw) To: Takashi Iwai; +Cc: Al Viro, linux-input Hi Takashi, On Tue, Sep 01, 2015 at 07:23:25AM +0200, Takashi Iwai wrote: > On Wed, 19 Aug 2015 09:38:15 +0200, > Takashi Iwai wrote: > > > > We've got bug reports showing the old systemd-logind (at least > > system-210) aborting unexpectedly, and this turned out to be because > > of an invalid error code from close() call to evdev devices. close() > > is supposed to return only either EINTR or EBADFD, while the device > > returned ENODEV. logind was overreacting to it and decided to kill > > itself when an unexpected error code was received. What a tragedy. > > > > The bad error code comes from flush fops, and actually evdev_flush() > > returns -ENODEV and else. This patch papers over it, simply fixing > > the error return code to the acceptable values above. > > > > Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > Hi, > > any comments on this patch? > As we have discussed previously returning EBADF will not actually help failing versions of systemd because they were not expecting getting any errors from close(). Considering the code and close() behavior I think the best way would be to stop reporting any errors from evdev_flush(), like in the version of the patch below. Please let me know if you are OK with this and I'll apply it. Thanks! -- Dmitry Input: evdev - do not report errors form flush() From: Takashi Iwai <tiwai@suse.de> We've got bug reports showing the old systemd-logind (at least system-210) aborting unexpectedly, and this turned out to be because of an invalid error code from close() call to evdev devices. close() is supposed to return only either EINTR or EBADFD, while the device returned ENODEV. logind was overreacting to it and decided to kill itself when an unexpected error code was received. What a tragedy. The bad error code comes from flush fops, and actually evdev_flush() returns -ENODEV when device is disconnected or client's access to it is revoked. But in these cases the fact that flush did not actually happen is not an error, but rather normal behavior. For non-disconnected devices result of flush is also not that interesting as there is no potential of data loss and even if it fails application has no way of handling the error. Because of that we are better off always returning success from evdev_flush(). Also returning -EINTR from flush()/close() is discouraged (as it is not clear how application should handle this error), so let's stop taking evdev->mutex interruptibly. Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/evdev.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 9d35499..08d4964 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -290,19 +290,14 @@ static int evdev_flush(struct file *file, fl_owner_t id) { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev; - int retval; - retval = mutex_lock_interruptible(&evdev->mutex); - if (retval) - return retval; + mutex_lock(&evdev->mutex); - if (!evdev->exist || client->revoked) - retval = -ENODEV; - else - retval = input_flush_device(&evdev->handle, file); + if (evdev->exist && !client->revoked) + input_flush_device(&evdev->handle, file); mutex_unlock(&evdev->mutex); - return retval; + return 0; } static void evdev_free(struct device *dev) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Input: evdev - Use EBADFD for flush() errors 2015-09-04 5:37 ` Dmitry Torokhov @ 2015-09-04 5:42 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2015-09-04 5:42 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Al Viro, linux-input On Fri, 04 Sep 2015 07:37:23 +0200, Dmitry Torokhov wrote: > > Hi Takashi, > > On Tue, Sep 01, 2015 at 07:23:25AM +0200, Takashi Iwai wrote: > > On Wed, 19 Aug 2015 09:38:15 +0200, > > Takashi Iwai wrote: > > > > > > We've got bug reports showing the old systemd-logind (at least > > > system-210) aborting unexpectedly, and this turned out to be because > > > of an invalid error code from close() call to evdev devices. close() > > > is supposed to return only either EINTR or EBADFD, while the device > > > returned ENODEV. logind was overreacting to it and decided to kill > > > itself when an unexpected error code was received. What a tragedy. > > > > > > The bad error code comes from flush fops, and actually evdev_flush() > > > returns -ENODEV and else. This patch papers over it, simply fixing > > > the error return code to the acceptable values above. > > > > > > Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 > > > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > Hi, > > > > any comments on this patch? > > > > As we have discussed previously returning EBADF will not actually help > failing versions of systemd because they were not expecting getting > any errors from close(). > > Considering the code and close() behavior I think the best way would be > to stop reporting any errors from evdev_flush(), like in the version of > the patch below. Please let me know if you are OK with this and I'll > apply it. Yes, that works, too. Thanks! Takashi > > Thanks! > > -- > Dmitry > > Input: evdev - do not report errors form flush() > > From: Takashi Iwai <tiwai@suse.de> > > We've got bug reports showing the old systemd-logind (at least > system-210) aborting unexpectedly, and this turned out to be because > of an invalid error code from close() call to evdev devices. close() > is supposed to return only either EINTR or EBADFD, while the device > returned ENODEV. logind was overreacting to it and decided to kill > itself when an unexpected error code was received. What a tragedy. > > The bad error code comes from flush fops, and actually evdev_flush() > returns -ENODEV when device is disconnected or client's access to it is > revoked. But in these cases the fact that flush did not actually happen is > not an error, but rather normal behavior. For non-disconnected devices > result of flush is also not that interesting as there is no potential of > data loss and even if it fails application has no way of handling the > error. Because of that we are better off always returning success from > evdev_flush(). > > Also returning -EINTR from flush()/close() is discouraged (as it is not > clear how application should handle this error), so let's stop taking > evdev->mutex interruptibly. > > Bugzilla: http://bugzilla.suse.com/show_bug.cgi?id=939834 > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/evdev.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c > index 9d35499..08d4964 100644 > --- a/drivers/input/evdev.c > +++ b/drivers/input/evdev.c > @@ -290,19 +290,14 @@ static int evdev_flush(struct file *file, fl_owner_t id) > { > struct evdev_client *client = file->private_data; > struct evdev *evdev = client->evdev; > - int retval; > > - retval = mutex_lock_interruptible(&evdev->mutex); > - if (retval) > - return retval; > + mutex_lock(&evdev->mutex); > > - if (!evdev->exist || client->revoked) > - retval = -ENODEV; > - else > - retval = input_flush_device(&evdev->handle, file); > + if (evdev->exist && !client->revoked) > + input_flush_device(&evdev->handle, file); > > mutex_unlock(&evdev->mutex); > - return retval; > + return 0; > } > > static void evdev_free(struct device *dev) > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-04 5:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-19 7:38 [PATCH] Input: evdev - Use EBADFD for flush() errors Takashi Iwai 2015-09-01 5:23 ` Takashi Iwai 2015-09-04 5:37 ` Dmitry Torokhov 2015-09-04 5:42 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).