[PATCH] udev: fix problem due to unsorted events

[Andrey Vagin]

From: Andrey Vagin <***@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?id=799834

Signed-off-by: Andrey Vagin <***@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--;

[Kay Sievers]

Post by Andrey Vagin
A kernel gives events, but this events can be unsorted.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kay Sievers]

Post by Andrey Vagin
A kernel gives events, but this events can be unsorted.
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 th=

at

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 y=

ou ask.

I suppose that a kernel should not send un-ordered events, but it doe=

s.

=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&sequence_lock);
=C2=A0 =C2=A0 =C2=A0 =C2=A0seq =3D ++uevent_seqnum;
=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock(&sequence_lock);
.... <-- here someone can send its event
=C2=A0 =C2=A0 =C2=A0 =C2=A0/* send netlink message */
=C2=A0 =C2=A0 =C2=A0 =C2=A0mutex_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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug=
" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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.

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

[Kay Sievers]

Post by 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[a***@gmail.com]

Post by Kay Sievers

Post by 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.

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?

Post by Kay Sievers
seq = ++uevent_seqnum;
add_uevent_var(env, "SEQNUM=%llu", (unsigned long long) ++uevent_seqnum);
right?
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

--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Greg Kroah-Hartman]

Post by a***@gmail.com

Post by Kay Sievers

Post by 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html