[PATCH] udev: fix problem due to unsorted events

[PATCH] udev: fix problem due to unsorted events

[Andrey Vagin]

From: Andrey Vagin <avagin@openvz.org>

A kernel gives events, but this events can be unsorted.
For example, here is log from system:
udevd[77]: seq 924 queued, 'add' 'bdi'
udevd[77]: seq 926 queued, 'add' 'block'
udevd[77]: seq 927 queued, 'add' 'block'
udevd[77]: seq 928 queued, 'add' 'block
udevd[77]: seq 925 queued, 'add' 'drivers'

In this case "udevadm settle" returns an error:
queue is empty but kernel events still pending [928]<->[925]

Let's look at update_queue(). It contains the follow code:

/* now write to the queue */
if (state = DEVICE_QUEUED) {
        udev_queue_export->queued_count++;
        udev_queue_export->seqnum_min = seqnum;
}

where seqnum_min is latest sequence number in queue file. Probably we
should check that seqnum is not less than seqnum_min and update it only
in this case.

https://bugzilla.redhat.com/show_bug.cgi?idy9834

Signed-off-by: Andrey Vagin <avagin@gmail.com>
---
 src/libudev-queue-private.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/libudev-queue-private.c b/src/libudev-queue-private.c
index 7177195..6a689c4 100644
--- a/src/libudev-queue-private.c
+++ b/src/libudev-queue-private.c
@@ -376,7 +376,8 @@ static int update_queue(struct udev_queue_export *udev_queue_export,
         /* now write to the queue */
         if (state = DEVICE_QUEUED) {
                 udev_queue_export->queued_count++;
-                udev_queue_export->seqnum_min = seqnum;
+                if (udev_queue_export->seqnum_min < seqnum)
+                        udev_queue_export->seqnum_min = seqnum;
         } else {
                 udev_queue_export->waste_bytes += queue_record_size(devpath_len) + queue_record_size(0);
                 udev_queue_export->queued_count--;
-- 
1.7.7.6

Re: [PATCH] udev: fix problem due to unsorted events

[Kay Sievers]

On Tue, Mar 6, 2012 at 09:48, Andrey Vagin <avagin@gmail.com> wrote:
> From: Andrey Vagin <avagin@openvz.org>
>
> A kernel gives events, but this events can be unsorted.
> For example, here is log from system:
> udevd[77]: seq 924 queued, 'add' 'bdi'
> udevd[77]: seq 926 queued, 'add' 'block'
> udevd[77]: seq 927 queued, 'add' 'block'
> udevd[77]: seq 928 queued, 'add' 'block
> udevd[77]: seq 925 queued, 'add' 'drivers'

The kernel cannot send un-ordered events. Please try to reproduce that
with 'udevadm monitor'.

I doubt that this is a problem udev should try to work around, its
logic is entirely based on the assumption that kernel events are
always in order.

Kay

Re: [PATCH] udev: fix problem due to unsorted events

[Kay Sievers]

On Tue, Mar 6, 2012 at 13:13, Andrew Wagin <avagin@gmail.com> wrote:
> 2012/3/6 Kay Sievers <kay.sievers@vrfy.org>:
>> On Tue, Mar 6, 2012 at 09:48, Andrey Vagin <avagin@gmail.com> wrote:
>>> From: Andrey Vagin <avagin@openvz.org>
>>>
>>> A kernel gives events, but this events can be unsorted.
>>> For example, here is log from system:
>>> udevd[77]: seq 924 queued, 'add' 'bdi'
>>> udevd[77]: seq 926 queued, 'add' 'block'
>>> udevd[77]: seq 927 queued, 'add' 'block'
>>> udevd[77]: seq 928 queued, 'add' 'block
>>> udevd[77]: seq 925 queued, 'add' 'drivers'
>>
>> The kernel cannot send un-ordered events. Please try to reproduce that
>> with 'udevadm monitor'.
>
> I can't reproduce this bug, because I have not this host already.
> I've attached the console.log. I have not other logs.
> I showed you messages from event_queue_insert(). Actually it's what you ask.
> I suppose that a kernel should not send un-ordered events, but it does.
>
> Let's look at kobject_uevent_env() in kernel source:
>
>        spin_lock(&sequence_lock);
>        seq = ++uevent_seqnum;
>        spin_unlock(&sequence_lock);
>
> .... <-- here someone can send its event
>
>        /* send netlink message */
>        mutex_lock(&uevent_sock_mutex);
> .....
>
> I want to say that code contains a window between incrementing seqnum
> and sending an event.

Yeah, that looks suspicious. We need to find out what and how to fix
that. The queue handling in the daemon assumes that the events are
ordered, so there might not only a problem with 'settle'.

Kay

[PATCH] uevent: send events in correct order according to seqnum

[Andrew Vagin]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]


The queue handling in the udev daemon assumes that the events are
ordered.

Before this patch uevent_seqnum is incremented under sequence_lock,
than an event is send uner uevent_sock_mutex. I want to say that code
contained a window between incrementing seqnum and sending an event.

This patch locks uevent_sock_mutex before incrementing uevent_seqnum.

Signed-off-by: Andrew Vagin <avagin@openvz.org>
---
 lib/kobject_uevent.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-uevent-send-events-in-correct-order-according-to-seq.patch --]
[-- Type: text/x-patch; name="0001-uevent-send-events-in-correct-order-according-to-seq.patch", Size: 798 bytes --]

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index e66e9b6..596c40d 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -243,6 +243,9 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	else if (action == KOBJ_REMOVE)
 		kobj->state_remove_uevent_sent = 1;
 
+#if defined(CONFIG_NET)
+	mutex_lock(&uevent_sock_mutex);
+#endif
 	/* we will send an event, so request a new sequence number */
 	spin_lock(&sequence_lock);
 	seq = ++uevent_seqnum;
@@ -253,7 +256,6 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 
 #if defined(CONFIG_NET)
 	/* send netlink message */
-	mutex_lock(&uevent_sock_mutex);
 	list_for_each_entry(ue_sk, &uevent_sock_list, list) {
 		struct sock *uevent_sock = ue_sk->sk;
 		struct sk_buff *skb;

Re: [PATCH] uevent: send events in correct order according to seqnum

[Kay Sievers]

On Tue, Mar 6, 2012 at 21:06, Andrew Vagin <avagin@openvz.org> wrote:
>
> The queue handling in the udev daemon assumes that the events are
> ordered.
>
> Before this patch uevent_seqnum is incremented under sequence_lock,
> than an event is send uner uevent_sock_mutex. I want to say that code
> contained a window between incrementing seqnum and sending an event.
>
> This patch locks uevent_sock_mutex before incrementing uevent_seqnum.

I think we can remove the spin_lock(&sequence_lock); entirely now, right?

Also the section with:
  seq = ++uevent_seqnum;
can just be:
  add_uevent_var(env, "SEQNUM=%llu", (unsigned long long) ++uevent_seqnum);
right?

And the:
  mutex_lock(&uevent_sock_mutex);
can just move outside of the _NET ifdef and we always use the mutex
instead of the spinlock?

That could look much simpler than the current code, I think.

Thanks,
Kay

Re: [PATCH] uevent: send events in correct order according to seqnum

[avagin]

On 03/07/2012 01:03 AM, Kay Sievers wrote:
> On Tue, Mar 6, 2012 at 21:06, Andrew Vagin<avagin@openvz.org>  wrote:
>>
>> The queue handling in the udev daemon assumes that the events are
>> ordered.
>>
>> Before this patch uevent_seqnum is incremented under sequence_lock,
>> than an event is send uner uevent_sock_mutex. I want to say that code
>> contained a window between incrementing seqnum and sending an event.
>>
>> This patch locks uevent_sock_mutex before incrementing uevent_seqnum.
>
> I think we can remove the spin_lock(&sequence_lock); entirely now, right?

I thought about that too. sequence_lock is used when CONFIG_NET isn't 
defined. I've looked on this code one more time and we may leave only 
uevent_sock_mutex and use it even when CONFIG_NET isn't defined.
Thanks for the comment.

Greg, do you have other objections about this patch?

>
> Also the section with:
>    seq = ++uevent_seqnum;
> can just be:
>    add_uevent_var(env, "SEQNUM=%llu", (unsigned long long) ++uevent_seqnum);
> right?
>
> And the:
>    mutex_lock(&uevent_sock_mutex);
> can just move outside of the _NET ifdef and we always use the mutex
> instead of the spinlock?
>
> That could look much simpler than the current code, I think.
>
> Thanks,
> Kay

Re: [PATCH] uevent: send events in correct order according to seqnum

[Greg Kroah-Hartman]

On Wed, Mar 07, 2012 at 01:14:04AM +0400, avagin@gmail.com wrote:
> On 03/07/2012 01:03 AM, Kay Sievers wrote:
> >On Tue, Mar 6, 2012 at 21:06, Andrew Vagin<avagin@openvz.org>  wrote:
> >>
> >>The queue handling in the udev daemon assumes that the events are
> >>ordered.
> >>
> >>Before this patch uevent_seqnum is incremented under sequence_lock,
> >>than an event is send uner uevent_sock_mutex. I want to say that code
> >>contained a window between incrementing seqnum and sending an event.
> >>
> >>This patch locks uevent_sock_mutex before incrementing uevent_seqnum.
> >
> >I think we can remove the spin_lock(&sequence_lock); entirely now, right?
> 
> I thought about that too. sequence_lock is used when CONFIG_NET
> isn't defined. I've looked on this code one more time and we may
> leave only uevent_sock_mutex and use it even when CONFIG_NET isn't
> defined.
> Thanks for the comment.
> 
> Greg, do you have other objections about this patch?

Let's see the one based on Kay's comments first please.

thanks,

greg k-h

[PATCH] uevent: send events in correct order according to seqnum (v2)

[Andrew Vagin]

The queue handling in the udev daemon assumes that the events are
ordered.

Before this patch uevent_seqnum is incremented under sequence_lock,
than an event is send uner uevent_sock_mutex. I want to say that code
contained a window between incrementing seqnum and sending an event.

This patch locks uevent_sock_mutex before incrementing uevent_seqnum.

v2: delete sequence_lock, uevent_seqnum is protected by uevent_sock_mutex

Signed-off-by: Andrew Vagin <avagin@openvz.org>
---
 lib/kobject_uevent.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index e66e9b6..15a7ab9 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -29,16 +29,17 @@
 
 u64 uevent_seqnum;
 char uevent_helper[UEVENT_HELPER_PATH_LEN] = CONFIG_UEVENT_HELPER_PATH;
-static DEFINE_SPINLOCK(sequence_lock);
 #ifdef CONFIG_NET
 struct uevent_sock {
 	struct list_head list;
 	struct sock *sk;
 };
 static LIST_HEAD(uevent_sock_list);
-static DEFINE_MUTEX(uevent_sock_mutex);
 #endif
 
+/* This lock protects uevent_seqnum and uevent_sock_list */
+static DEFINE_MUTEX(uevent_sock_mutex);
+
 /* the strings here must match the enum in include/linux/kobject.h */
 static const char *kobject_actions[] = {
 	[KOBJ_ADD] =		"add",
@@ -136,7 +137,6 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	struct kobject *top_kobj;
 	struct kset *kset;
 	const struct kset_uevent_ops *uevent_ops;
-	u64 seq;
 	int i = 0;
 	int retval = 0;
 #ifdef CONFIG_NET
@@ -243,17 +243,14 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	else if (action = KOBJ_REMOVE)
 		kobj->state_remove_uevent_sent = 1;
 
+	mutex_lock(&uevent_sock_mutex);
 	/* we will send an event, so request a new sequence number */
-	spin_lock(&sequence_lock);
-	seq = ++uevent_seqnum;
-	spin_unlock(&sequence_lock);
-	retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)seq);
+	retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)++uevent_seqnum);
 	if (retval)
 		goto exit;
 
 #if defined(CONFIG_NET)
 	/* send netlink message */
-	mutex_lock(&uevent_sock_mutex);
 	list_for_each_entry(ue_sk, &uevent_sock_list, list) {
 		struct sock *uevent_sock = ue_sk->sk;
 		struct sk_buff *skb;
@@ -290,8 +287,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 		} else
 			retval = -ENOMEM;
 	}
-	mutex_unlock(&uevent_sock_mutex);
 #endif
+	mutex_unlock(&uevent_sock_mutex);
 
 	/* call uevent_helper, usually only enabled during early boot */
 	if (uevent_helper[0] && !kobj_usermode_filter(kobj)) {
-- 
1.7.1

Re: [PATCH] uevent: send events in correct order according to seqnum (v2)

[Kay Sievers]

On Wed, Mar 7, 2012 at 10:59, Andrew Vagin <avagin@openvz.org> wrote:
> The queue handling in the udev daemon assumes that the events are
> ordered.

> -       retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)seq);
> +       retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)++uevent_seqnum);
>        if (retval)
>                goto exit;

We need to unlock the mutex before the goto exit?

Other than that, looks good to me.

Thanks,
Kay

[PATCH] uevent: send events in correct order according to seqnum (v3)

[Andrew Vagin]

The queue handling in the udev daemon assumes that the events are
ordered.

Before this patch uevent_seqnum is incremented under sequence_lock,
than an event is send uner uevent_sock_mutex. I want to say that code
contained a window between incrementing seqnum and sending an event.

This patch locks uevent_sock_mutex before incrementing uevent_seqnum.

v2: delete sequence_lock, uevent_seqnum is protected by uevent_sock_mutex
v3: unlock the mutex before the goto exit

Thanks for Kay for the comments.

Signed-off-by: Andrew Vagin <avagin@openvz.org>
---
 lib/kobject_uevent.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index e66e9b6..75cbdb5 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -29,16 +29,17 @@
 
 u64 uevent_seqnum;
 char uevent_helper[UEVENT_HELPER_PATH_LEN] = CONFIG_UEVENT_HELPER_PATH;
-static DEFINE_SPINLOCK(sequence_lock);
 #ifdef CONFIG_NET
 struct uevent_sock {
 	struct list_head list;
 	struct sock *sk;
 };
 static LIST_HEAD(uevent_sock_list);
-static DEFINE_MUTEX(uevent_sock_mutex);
 #endif
 
+/* This lock protects uevent_seqnum and uevent_sock_list */
+static DEFINE_MUTEX(uevent_sock_mutex);
+
 /* the strings here must match the enum in include/linux/kobject.h */
 static const char *kobject_actions[] = {
 	[KOBJ_ADD] =		"add",
@@ -136,7 +137,6 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	struct kobject *top_kobj;
 	struct kset *kset;
 	const struct kset_uevent_ops *uevent_ops;
-	u64 seq;
 	int i = 0;
 	int retval = 0;
 #ifdef CONFIG_NET
@@ -243,17 +243,16 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	else if (action = KOBJ_REMOVE)
 		kobj->state_remove_uevent_sent = 1;
 
+	mutex_lock(&uevent_sock_mutex);
 	/* we will send an event, so request a new sequence number */
-	spin_lock(&sequence_lock);
-	seq = ++uevent_seqnum;
-	spin_unlock(&sequence_lock);
-	retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)seq);
-	if (retval)
+	retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)++uevent_seqnum);
+	if (retval) {
+		mutex_unlock(&uevent_sock_mutex);
 		goto exit;
+	}
 
 #if defined(CONFIG_NET)
 	/* send netlink message */
-	mutex_lock(&uevent_sock_mutex);
 	list_for_each_entry(ue_sk, &uevent_sock_list, list) {
 		struct sock *uevent_sock = ue_sk->sk;
 		struct sk_buff *skb;
@@ -290,8 +289,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 		} else
 			retval = -ENOMEM;
 	}
-	mutex_unlock(&uevent_sock_mutex);
 #endif
+	mutex_unlock(&uevent_sock_mutex);
 
 	/* call uevent_helper, usually only enabled during early boot */
 	if (uevent_helper[0] && !kobj_usermode_filter(kobj)) {
-- 
1.7.1

Re: [PATCH] uevent: send events in correct order according to seqnum (v3)

[Kay Sievers]

On Wed, Mar 7, 2012 at 11:49, Andrew Vagin <avagin@openvz.org> wrote:
> The queue handling in the udev daemon assumes that the events are
> ordered.
>
> Before this patch uevent_seqnum is incremented under sequence_lock,
> than an event is send uner uevent_sock_mutex. I want to say that code
> contained a window between incrementing seqnum and sending an event.
>
> This patch locks uevent_sock_mutex before incrementing uevent_seqnum.
>
> v2: delete sequence_lock, uevent_seqnum is protected by uevent_sock_mutex
> v3: unlock the mutex before the goto exit
>
> Thanks for Kay for the comments.
>
> Signed-off-by: Andrew Vagin <avagin@openvz.org>

Looks good to me. Works fine in a kvm installation here.
Feel free to add:
  Tested-By: Kay Sievers <kay.sievers@vrfy.org>

Thanks lot for the udev debugging and taking care of it,
Kay

Re: [PATCH] uevent: send events in correct order according to seqnum (v3)

[Greg Kroah-Hartman]

On Wed, Mar 07, 2012 at 02:49:56PM +0400, Andrew Vagin wrote:
> The queue handling in the udev daemon assumes that the events are
> ordered.
> 
> Before this patch uevent_seqnum is incremented under sequence_lock,
> than an event is send uner uevent_sock_mutex. I want to say that code
> contained a window between incrementing seqnum and sending an event.

Is this something that you have seen "in the wild"?  If so, we should
backport it to older kernels as well, right?

thanks,

greg k-h

Re: [PATCH] uevent: send events in correct order according to seqnum (v3)

[Andrew Wagin]

> Is this something that you have seen "in the wild"?  If so, we should
> backport it to older kernels as well, right?

Yes, we should. I found this bug in RHEL6, it uses 2.6.32 kernel.

https://bugzilla.redhat.com/show_bug.cgi?idy9834

Re: [PATCH] uevent: send events in correct order according to seqnum (v3)

[Greg Kroah-Hartman]

On Wed, Mar 07, 2012 at 09:34:19PM +0400, Andrew Wagin wrote:
> > Is this something that you have seen "in the wild"?  If so, we should
> > backport it to older kernels as well, right?
> 
> Yes, we should. I found this bug in RHEL6, it uses 2.6.32 kernel.
> 
> https://bugzilla.redhat.com/show_bug.cgi?idy9834

Great, thanks for letting me know, I'll queue it up for the older
kernels as well.

greg k-h