[systemd-devel] [PATCH] udevd: fix synchronization with settle when handling inotify events

[David Herrmann]

Hi

On Tue, Apr 7, 2015 at 12:03 AM, Daniel Drake <drake at endlessm.com> wrote:
> udev uses inotify to implement a scheme where when the user closes
> a writable device node, a change uevent is forcefully generated.
> In the case of block devices, it actually requests a partition rescan.
>
> This currently can't be synchronized with "udevadm settle", i.e. this
> is not reliable in a script:
>
>  sfdisk --change-id /dev/sda 1 81
>  udevadm settle
>  mount /dev/sda1 /foo
>
> The settle call doesn't synchronize there, so at the same time we try
> to mount the device, udevd is busy removing the partition device nodes and
> readding them again. The mount call often happens in that moment where the
> partition node has been removed but not readded yet.
>
> This exact issue was fixed long ago:
> http://git.kernel.org/cgit/linux/hotplug/udev.git/commit/?id=bb38678e3ccc02bcd970ccde3d8166a40edf92d3
>
> but that fix is no longer valid now that sequence numbers are no longer
> used.
>
> Fix this by forcing another mainloop iteration after handling inotify events
> before unblocking settle. If the inotify event caused us to generate a
> "change" event, we'll pick that up in the following loop iteration, before
> we reach the end of the loop where we respond to settle's control message,
> unblocking it.
> ---
>  src/udev/udevd.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/udev/udevd.c b/src/udev/udevd.c
> index 830aedd..dfecef8 100644
> --- a/src/udev/udevd.c
> +++ b/src/udev/udevd.c
> @@ -1504,9 +1504,22 @@ int main(int argc, char *argv[]) {
>                          continue;
>
>                  /* device node watch */
> -                if (is_inotify)
> +                if (is_inotify) {
>                          handle_inotify(udev);
>
> +                        /*
> +                         * settle might be waiting on us to determine the queue
> +                         * state. If we just handled an inotify event, we might have
> +                         * generated a "change" event, but we won't have queued up
> +                         * the resultant uevent yet.
> +                         *
> +                         * Before we go ahead and potentially tell settle that the
> +                         * queue is empty, lets loop one more time to update the
> +                         * queue state again before deciding.
> +                         */
> +                        continue;
> +                }
> +

Nice catch!

There's indeed a small race between handling inotify and queuing up
the change-event. We need to re-loop there. One day we should switch
to sd-event to avoid such bugs... I mean the symptom is inherent to
queuing up events while handling them. Meh!

Applied!

Thanks
David

>                  /* tell settle that we are busy or idle, this needs to be before the
>                   * PING handling
>                   */
> --
> 2.1.0
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel