New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept4() breaks Linux 2.6.31 compatibility #7

Closed
ryao opened this Issue Nov 18, 2012 · 10 comments

Comments

Projects
None yet
4 participants
@ryao
Contributor

ryao commented Nov 18, 2012

I was informed in IRC that ARM systems often use older kernels that lack accept4() support. The only invocation of accept4() in the tree is in ./src/udev/udev-ctrl.c. Based on the kernel documentation, it looks like we can replace the call to accept4() with a call to accept(), call fcntl to set SOCK_NONBLOCK and use O_CLOEXEC on open().

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 18, 2012

Contributor

Commit ff2c503 introduced accept4. We need to rework this to fix it.

Contributor

ryao commented Nov 18, 2012

Commit ff2c503 introduced accept4. We need to rework this to fix it.

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 18, 2012

Contributor

Commit b51f834 is intended to fix this. It needs testing before we can merge it to master. I will come back to this after some more pressing issues that prevent eudev from being a drop-in replacement for Gentoo's sys-fs/udev-171-r9 package are addressed.

Contributor

ryao commented Nov 18, 2012

Commit b51f834 is intended to fix this. It needs testing before we can merge it to master. I will come back to this after some more pressing issues that prevent eudev from being a drop-in replacement for Gentoo's sys-fs/udev-171-r9 package are addressed.

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 18, 2012

Contributor

I have backported this patch to udev 171 and verified that it works on Linux 3.6.6:

diff --git a/libudev/libudev-ctrl.c b/libudev/libudev-ctrl.c
index e0ec2fa..9233d0c 100644
--- a/libudev/libudev-ctrl.c
+++ b/libudev/libudev-ctrl.c
@@ -186,7 +186,7 @@ struct udev_ctrl_connection *udev_ctrl_get_connection(struct udev_ctrl *uctrl)
    conn->refcount = 1;
    conn->uctrl = uctrl;

-   conn->sock = accept4(uctrl->sock, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK);
+   conn->sock = accept(uctrl->sock, NULL, NULL);
    if (conn->sock < 0) {
        if (errno != EINTR)
            err(uctrl->udev, "unable to receive ctrl connection: %m\n");

I have asked steev for confirmation that this works on Linux 2.6.31. I will merge it to master afterward.

Contributor

ryao commented Nov 18, 2012

I have backported this patch to udev 171 and verified that it works on Linux 3.6.6:

diff --git a/libudev/libudev-ctrl.c b/libudev/libudev-ctrl.c
index e0ec2fa..9233d0c 100644
--- a/libudev/libudev-ctrl.c
+++ b/libudev/libudev-ctrl.c
@@ -186,7 +186,7 @@ struct udev_ctrl_connection *udev_ctrl_get_connection(struct udev_ctrl *uctrl)
    conn->refcount = 1;
    conn->uctrl = uctrl;

-   conn->sock = accept4(uctrl->sock, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK);
+   conn->sock = accept(uctrl->sock, NULL, NULL);
    if (conn->sock < 0) {
        if (errno != EINTR)
            err(uctrl->udev, "unable to receive ctrl connection: %m\n");

I have asked steev for confirmation that this works on Linux 2.6.31. I will merge it to master afterward.

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 18, 2012

Contributor

As an additional note, this regression occurred in udev 169.

Contributor

ryao commented Nov 18, 2012

As an additional note, this regression occurred in udev 169.

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 19, 2012

Contributor

It has been pointed out to me that Linux decided against supporting inheritance on the file descriptors involved, which requires that we invoke fcntl. I have written f04fc412682a98f9da9f8e9b353ac3cbbef66a77 to address that by introducing a fallback path and updated the backported patch:

diff --git a/src/udev/udev-ctrl.c b/src/udev/udev-ctrl.c
index a235912..01747b1 100644
--- a/libudev/libudev-ctrl.c
+++ b/libudev/libudev-ctrl.c
@@ -189,6 +189,18 @@ struct udev_ctrl_connection *udev_ctrl_get_connection(struct udev_ctrl *uctrl)
         conn->uctrl = uctrl;

         conn->sock = accept4(uctrl->sock, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK);
+
+        /* Fallback path when accept4() is unavailable */
+        if ( conn->sock < 0 && (errno == ENOSYS || errno == ENOTSUP) )
+        {
+                conn->sock = accept(uctrl->sock, NULL, NULL);
+
+                if (conn->sock >= 0) {
+                        fcntl(conn->sock, F_SETFL, O_NONBLOCK);
+                        fcntl(conn->sock, F_SETFD, FD_CLOEXEC);
+                }
+        }
+
         if (conn->sock < 0) {
                 if (errno != EINTR)
                         log_error("unable to receive ctrl connection: %m\n");
Contributor

ryao commented Nov 19, 2012

It has been pointed out to me that Linux decided against supporting inheritance on the file descriptors involved, which requires that we invoke fcntl. I have written f04fc412682a98f9da9f8e9b353ac3cbbef66a77 to address that by introducing a fallback path and updated the backported patch:

diff --git a/src/udev/udev-ctrl.c b/src/udev/udev-ctrl.c
index a235912..01747b1 100644
--- a/libudev/libudev-ctrl.c
+++ b/libudev/libudev-ctrl.c
@@ -189,6 +189,18 @@ struct udev_ctrl_connection *udev_ctrl_get_connection(struct udev_ctrl *uctrl)
         conn->uctrl = uctrl;

         conn->sock = accept4(uctrl->sock, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK);
+
+        /* Fallback path when accept4() is unavailable */
+        if ( conn->sock < 0 && (errno == ENOSYS || errno == ENOTSUP) )
+        {
+                conn->sock = accept(uctrl->sock, NULL, NULL);
+
+                if (conn->sock >= 0) {
+                        fcntl(conn->sock, F_SETFL, O_NONBLOCK);
+                        fcntl(conn->sock, F_SETFD, FD_CLOEXEC);
+                }
+        }
+
         if (conn->sock < 0) {
                 if (errno != EINTR)
                         log_error("unable to receive ctrl connection: %m\n");
@lu-zero

This comment has been minimized.

Show comment
Hide comment
@lu-zero

lu-zero Nov 19, 2012

Member

On Mon, Nov 19, 2012 at 7:15 AM, Richard Yao notifications@github.com wrote:

It has been pointed out to me that Linux decided against supporting inheritance on the file descriptors involved, which requires that we invoke fcntl. I have written f04fc41 to address that by introducing a fallback path and updated the backported patch:

Looks fine to me.

Member

lu-zero commented Nov 19, 2012

On Mon, Nov 19, 2012 at 7:15 AM, Richard Yao notifications@github.com wrote:

It has been pointed out to me that Linux decided against supporting inheritance on the file descriptors involved, which requires that we invoke fcntl. I have written f04fc41 to address that by introducing a fallback path and updated the backported patch:

Looks fine to me.

@ghost ghost assigned ryao Nov 23, 2012

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Nov 26, 2012

Contributor

This is still waiting on feedback from steev.

Contributor

ryao commented Nov 26, 2012

This is still waiting on feedback from steev.

@steev

This comment has been minimized.

Show comment
Hide comment
@steev

steev Nov 27, 2012

Requires including linux/signalfd.h, not sure if that's a big deal to others or not.

steev commented Nov 27, 2012

Requires including linux/signalfd.h, not sure if that's a big deal to others or not.

@ryao

This comment has been minimized.

Show comment
Hide comment
@ryao

ryao Dec 14, 2012

Contributor

This was merged into eudev in time for the 1_beta1 tag. I was inclined to keep this issue open until it was backported to udev-171, but that belongs in the Gentoo bug tracker.

Contributor

ryao commented Dec 14, 2012

This was merged into eudev in time for the 1_beta1 tag. I was inclined to keep this issue open until it was backported to udev-171, but that belongs in the Gentoo bug tracker.

@ryao ryao closed this Dec 14, 2012

@jshafer817

This comment has been minimized.

Show comment
Hide comment
@jshafer817

jshafer817 Sep 9, 2013

Thanks!!! I think this will do the trick!

jshafer817 commented Sep 9, 2013

Thanks!!! I think this will do the trick!

blueness added a commit that referenced this issue Sep 10, 2013

libudev: enumerate: fix NULL-deref for subsystem-matches
udev_device_get_subsystem() may return NULL if no subsystem could be
figured out by libudev. This might be due to OOM or if the device
disconnected between the udev_device_new() call and
udev_device_get_subsystem(). Therefore, we need to handle subsystem==NULL
safely.

Instead of testing for it in each helper, we treat subsystem==NULL as
empty subsystem in match_subsystem().

Backtrace of udev_enumerate with an input-device disconnecting in exactly
this time-frame:
 (gdb) bt
 #0  0x00007ffff569dc24 in strnlen () from /usr/lib/libc.so.6
 #1  0x00007ffff56d9e04 in fnmatch@@GLIBC_2.2.5 () from /usr/lib/libc.so.6
 #2  0x00007ffff5beb83d in match_subsystem (udev_enumerate=0x7a05f0, subsystem=0x0) at src/libudev/libudev-enumerate.c:727
 #3  0x00007ffff5bebb30 in parent_add_child (enumerate=enumerate@entry=0x7a05f0, path=<optimized out>) at src/libudev/libudev-enumerate.c:834
 #4  0x00007ffff5bebc3f in parent_crawl_children (enumerate=enumerate@entry=0x7a05f0, path=0x7a56b0 "/sys/devices/<shortened>/input/input97", maxdepth=maxdepth@entry=254) at src/libudev/libudev-enumerate.c:866
 #5  0x00007ffff5bebc54 in parent_crawl_children (enumerate=enumerate@entry=0x7a05f0, path=0x79e8c0 "/sys/devices/<shortened>/input", maxdepth=maxdepth@entry=255) at src/libudev/libudev-enumerate.c:868
 #6  0x00007ffff5bebc54 in parent_crawl_children (enumerate=enumerate@entry=0x7a05f0, path=path@entry=0x753190 "/sys/devices/<shortened>", maxdepth=maxdepth@entry=256) at src/libudev/libudev-enumerate.c:868
 #7  0x00007ffff5bec7df in scan_devices_children (enumerate=0x7a05f0) at src/libudev/libudev-enumerate.c:882
 #8  udev_enumerate_scan_devices (udev_enumerate=udev_enumerate@entry=0x7a05f0) at src/libudev/libudev-enumerate.c:919
 #9  0x00007ffff5df8777 in <random_caller> () at some/file.c:181
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment