clone() with CLONE_NEWNET breaks kobject_uevent_env()

Hi,

after analysing very strange report (with running chromium some device-mapper ioctl functions started to fail) I found interesting problem:

If you run clone() with CLONE_NEWNET (which is chromium using for sanboxing), udev namespace is cloned too (newly registered in uevent_sock_list) and netlink send (except the first in list) fails with -ESRCH.

This causes that _every_ call of kobject_uevent_env() return failure.

Most of users silently ignores kobject_uevent() return value, so the problem was invisible for long time.

Unfortunately dm checks return value and reports failure, taking the wrong error path.

How is this supposed to work?

Why cloning net namespace breaks the udev netlink subsystem?

Is it bug or we need to do something differently? (I do not think ignoring return value is the proper way...)

Hi,

after analysing very strange report (with running chromium some device-mapper ioctl functions started to fail) I found interesting problem:

If you run clone() with CLONE_NEWNET (which is chromium using for sanboxing), udev namespace is cloned too (newly registered in uevent_sock_list) and netlink send (except the first in list) fails with -ESRCH.

This causes that _every_ call of kobject_uevent_env() return failure.

Most of users silently ignores kobject_uevent() return value, so the problem was invisible for long time.

Unfortunately dm checks return value and reports failure, taking the wrong error path.

How is this supposed to work?

Why cloning net namespace breaks the udev netlink subsystem?

Is it bug or we need to do something differently? (I do not think ignoring return value is the proper way...)

Milan Broz <[email protected]> writes:

I think the proper fix is to remove the error return from kobject_uevent_env and kobject_uevent, and make it harder to get calling of this function wrong. Possibly in conjunction with that tag all of the memory allocations of kobject_uevent_env with GFP_NOFAIL or something so the memory allocator knows that this path is totally not able to deal with failure.

Is kobject_uevent_env anything except an asynchronous best effort notification to user-space that a device has come or gone?

Quoted 11 lines · Eric W. Biederman

Milan Broz <[email protected]> writes:

I think the proper fix is to remove the error return from kobject_uevent_env and kobject_uevent, and make it harder to get calling of this function wrong. Possibly in conjunction with that tag all of the memory allocations of kobject_uevent_env with GFP_NOFAIL or something so the memory allocator knows that this path is totally not able to deal with failure.

Is kobject_uevent_env anything except an asynchronous best effort notification to user-space that a device has come or gone?

Unfortunately it is for device-mapper. libdevmapper depends on information that uevent was sent because udev rules uses semaphore to inform that some action was taken.
So if dm-ioctl returns flag that uevent was not sent, it fallback to different error path (otherwise it waits for completion forever). (TBH I am more and more convinced this was not quite clever concept.)

Quoted 13 lines · Milan Broz

Quoted 11 lines · Eric W. Biederman

Milan Broz <[email protected]> writes:

I think the proper fix is to remove the error return from kobject_uevent_env and kobject_uevent, and make it harder to get calling of this function wrong. Possibly in conjunction with that tag all of the memory allocations of kobject_uevent_env with GFP_NOFAIL or something so the memory allocator knows that this path is totally not able to deal with failure.

Is kobject_uevent_env anything except an asynchronous best effort notification to user-space that a device has come or gone?

Quoted 6 lines

Unfortunately it is for device-mapper. libdevmapper depends on information that uevent was sent because udev rules uses semaphore to inform that some action was taken.
So if dm-ioctl returns flag that uevent was not sent, it fallback to different error path (otherwise it waits for completion forever). (TBH I am more and more convinced this was not quite clever concept.)

If I understand your description and the code right the guarantee that you need is that kobject_uevent will return success only if it has queued a packet in every listening netlink socket.

We already ignore ENOBUFS so the guarantee you appear to need in libdevmapper does not appear to be present in kobject_uevent.

Does the libdevmapper code work despite getting a spurious failure?

Quoted 22 lines · Eric W. Biederman

Quoted 13 lines · Milan Broz

Quoted 11 lines · Eric W. Biederman

Milan Broz <[email protected]> writes:

I think the proper fix is to remove the error return from kobject_uevent_env and kobject_uevent, and make it harder to get calling of this function wrong. Possibly in conjunction with that tag all of the memory allocations of kobject_uevent_env with GFP_NOFAIL or something so the memory allocator knows that this path is totally not able to deal with failure.

Is kobject_uevent_env anything except an asynchronous best effort notification to user-space that a device has come or gone?

Quoted 6 lines

Unfortunately it is for device-mapper. libdevmapper depends on information that uevent was sent because udev rules uses semaphore to inform that some action was taken.
So if dm-ioctl returns flag that uevent was not sent, it fallback to different error path (otherwise it waits for completion forever). (TBH I am more and more convinced this was not quite clever concept.)

Quoted 3 lines

If I understand your description and the code right the guarantee that you need is that kobject_uevent will return success only if it has queued a packet in every listening netlink socket.

I think so. IOW success == event was sent to all active listeners.

Quoted 4 lines

We already ignore ENOBUFS so the guarantee you appear to need in libdevmapper does not appear to be present in kobject_uevent.

Does the libdevmapper code work despite getting a spurious failure?

BTW I do not see ENOBUFS but ESRCH (from netlink_broadcast_filtered).

If spurious failure is that event is sent (even partially) but it reports failure, it is the exact situation I see now - libdevmapper will try to decrement system semaphore which is already removed from udev rules.

Final state is correct, just it prints ugly warnings. IOW it recovers from this situation correctly.

But Kay's suggestion to use netlink_has_listeners() seems like good idea. IOW if there is no listener, it should skip quietly and not fail the whole call...

But Kay's suggestion to use netlink_has_listeners() seems like good idea. IOW if there is no listener, it should skip quietly and not fail the whole call...
In the case of ESRCH I completely agree.

We are currently ignoring errors in the semantically more interesting case when netlink_broadcast does not deliver the packet to one of the listening netlink sockets.

How does this patch look?

kobject_uevent() uses a multicast socket and should ignore if one of listeners cannot handle messages or nobody is listening at all.

Easily reproducible when a process in system is cloned with CLONE_NEWNET flag.

-                       if (retval == -ENOBUFS)
+                       if (retval == -ENOBUFS || retval == -ESRCH)

kobject_uevent() uses a multicast socket and should ignore if one of listeners cannot handle messages or nobody is listening at all.

Easily reproducible when a process in system is cloned with CLONE_NEWNET flag.

(See also
http://article.gmane.org/gmane.linux.kernel.device-mapper.dm-crypt/5256)

Signed-off-by Milan Broz <[email protected]>

---
 lib/kobject_uevent.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

lib/kobject_uevent.c +1 -1

index 70af0a7..ad72a03 100644
@@ -282,7 +282,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 							    kobj_bcast_filter,
 							    kobj);
 			/* ENOBUFS should be handled in userspace */
-			if (retval == -ENOBUFS)
+			if (retval == -ENOBUFS || retval == -ESRCH)
 				retval = 0;
 		} else
 			retval = -ENOMEM;

Quoted 9 lines · Milan Broz

kobject_uevent() uses a multicast socket and should ignore if one of listeners cannot handle messages or nobody is listening at all.

Easily reproducible when a process in system is cloned with CLONE_NEWNET flag.

(See also
http://article.gmane.org/gmane.linux.kernel.device-mapper.dm-crypt/5256)

I am fine with the patch below.

However if you are fine with the patch below let's just remove the return code from code from kobject_uevent_env. We are now explicitly ignoring all of the possible return codes from netlink_broadcast_filtered.

Ignoring all of the return codes from netlink_broadcast_filtered ignores all of the expected errors from kobject_uevent_env, short of programmer error.

So why have an error code on kobject_uevent_env?

Quoted 18 lines

Signed-off-by Milan Broz <[email protected]>

---
 lib/kobject_uevent.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

lib/kobject_uevent.c +1 -1

index 70af0a7..ad72a03 100644
@@ -282,7 +282,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 							    kobj_bcast_filter,
 							    kobj);
 			/* ENOBUFS should be handled in userspace */
-			if (retval == -ENOBUFS)
+			if (retval == -ENOBUFS || retval == -ESRCH)
 				retval = 0;
 		} else
 			retval = -ENOMEM;

Quoted 5 lines · Milan Broz

If you run clone() with CLONE_NEWNET (which is chromium using for sanboxing), udev namespace is cloned too (newly registered in uevent_sock_list) and netlink send (except the first in list) fails with -ESRCH.

Quoted 1 line

This causes that _every_ call of kobject_uevent_env() return failure.

That netlink_ broadcast chooses to treat failure to deliver a packet to anyone as an error and return -ESRCH is a little peculiar.  In general we don't see that error because when you are testing there is at least one listener on the netlink socket.  So as a practical matter I think we should be ignoring return values of -ESRCH from netlink_broadcast, in kobject_uevent_env.