eudev-project / eudev

Repository for eudev development
GNU General Public License v2.0
523 stars 147 forks source link

eudev 3.2.7 fails to start monitor in Buildroot environment #172

Closed sammj closed 4 years ago

sammj commented 5 years ago

Hi All,

Recently we updated our buildroot-based project https://github.com/open-power/op-build to eudev 3.2.7 and found that setting up a udev monitor ( https://github.com/open-power/petitboot/blob/master/discover/udev.c#L527 ) failed silently. I tracked this down to this change: https://github.com/gentoo/eudev/commit/b1e47be38259f33e1db18193904575a1ffa21be8

It looks like we're failing the udev_has_devtmpfs check, specifically on name_to_handle_at, because devtmpfs doesn't support this syscall. If we revert this (https://github.com/sammj/eudev/commit/c1c4b3bbe72ccc947e17c70b4c7e747a69e29288) udev events are processed as normal.

I imagine we probably do want to have a better check here but I'm not totally across what it needs to be here in our environment. Could someone please offer some insight?

blueness commented 5 years ago

Okay let's start narrowing this down. There are two possibilities, if either is true, it fails. So replace this code if (access(UDEV_ROOT_RUN "/udev/control", F_OK) < 0 || !udev_has_devtmpfs(udev)) { with if (access(UDEV_ROOT_RUN "/udev/control", F_OK)) { and then with if (!udev_has_devtmpfs(udev)) { and see which fails.

sammj commented 5 years ago

Yep, while debugging this I saw that it's !udev_has_devtmpfs(udev) that fails - specifically on the name_to_handle_at call.

blueness commented 5 years ago

I looked through that code and I can't get a handle on why it might be failing for your build env. So: 1) Make sure name_to_handle_at() is supported. linux > 2.6.39 and glibc > 2.14 2) If that's the case, try to get me more logs contextualizing why the call fails.

sammj commented 5 years ago

Sorry for the radio silence, I got pulled into a few other fires. This is occurring with linux 5.0.7 and glibc 2.28. What logs will be useful for you, and are there any debug options I should enable?

shenki commented 4 years ago

I was able to confirm Sam's fundings under a similar environment (powerpc64le, glibc 2.28, linux 5.3). We see name_to_handle_at returning EOPNOTSUPP on /dev:

strace -e trace=name_to_handle_at ./udev-test 
name_to_handle_at(AT_FDCWD, "/dev", {handle_bytes=128}, 0x7fffdf4d1b80, 0) = -1 EOPNOTSUPP (Operation not supported)

The kernel has COFNIG_FHANDLE enabled, so I'm a little puzzled.

# zcat /proc/config.gz | grep FHANDLE
CONFIG_FHANDLE=y

The same test program (https://ozlabs.org/~joel/udev-test.c) on the same hardware, but running under a distro, works fine:

looking for mid 21

20 1 8:2 / / rw,relatime shared:1 - ext4 /dev/root rw,errors=remount-ro
got mid (20)

21 20 0:6 / /dev rw,relatime shared:2 - devtmpfs devtmpfs rw,size=63278912k,nr_inodes=988733,mode=755
got mid (21)
mid matched
got seperator

udev_has_devtmpfs: true
shenki commented 4 years ago

To answer my question in the previous comment, the reason it's not supported is kernel has CONFIG_SHMEM=n and therefore CONFIG_TMPFS=n. Looking at the kernel's implementation of name_to_handle_at, there's one code path that returns EOPNOTSUPP:

        /*
         * We need to make sure whether the file system
         * support decoding of the file handle
         */
        if (!path->dentry->d_sb->s_export_op ||
            !path->dentry->d_sb->s_export_op->fh_to_dentry)
                return -EOPNOTSUPP;

When TMPFS is disabled, devtmpfs is implemented as ramfs. This lacks a fh_to_dentry operation (struct export_operations *s_export_op is where the fh_to_dentry callback is added, as can be seen in mm/shmem.c shmem_fill_super).

The behavior can be reproduced by turning off SHMEM in a kernel config.

shenki commented 4 years ago

Now that we know how to reproduce, I was wondering how to fix it. Going back to the change that https://github.com/gentoo/eudev/commit/b1e47be38259f33e1db18193904575a1ffa21be8 caused the issue, it's not clear to me why the and condition was changed to an or.

The code read as "if there's no socket, and /dev/ is not a tmpfs, we can't open the socket". This is the same implementation that systemd ships (https://github.com/systemd/systemd/blob/master/src/libsystemd/sd-device/device-monitor.c#L126).

Can someone explain why the devtmpfs test needs to be there at all?

blueness commented 4 years ago

Okay, I now see what's going on with the buildroot env. I'm okay with dropping the devtmpfs check since there should be no reason why you can't create a socket on a regular filesystem, although it is cleaner on tmpfs.

Do you want to create a PR so you get attribution for work. Else its easy enough for me to just remove the check.

shenki commented 4 years ago

@blueness I'm happy for you to fix it up. Thanks very much!

blueness commented 4 years ago

Okay please test this patch. If it works for you, it will be out in the next release I cut. Reopen the issue if it is still a problem. Sorry for the delay in acting, but not being a buildroot user, I just assumed that in setting up the chroot, ${ROOT}/dev was mounted tmpfs or bind mounted from /dev.