Re: [PATCH] race between util_create_path() and util_delete_path()?

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> That would limit paralellism to some degree.
> 
> Retrys won't be needed very often; they would surely have a lower
> overhead.  Less code too :-).  It's not completely out of left field;
> both seqlocks and transactional memory use retries.  There's no risk
> of livelock (i.e. infinite retries)...
> 
> I'm lazy and don't want udev to run measurably slower.  The only
> advantage I see of flock() is that it tells the reader "I'm a
> synchronization primitive, solving a concurrency problem here".  I
> think we can get close enough by explaining the race in a comment next
> to the definition of create_path().

plus, it seems to be the simpler solution by far - which is why I went
with flock() in my suggested fix you find below. Untested, as usual, and
I don't really have a clue whether there are any unhealthy interactions
with the rest of udev in this solution. Also, you most likely will want
to change the path of the lock file ;-)

Florian

diff --git a/libudev/libudev-device-private.c b/libudev/libudev-device-private.c
index 68dc0a5..43cf152 100644
--- a/libudev/libudev-device-private.c
+++ b/libudev/libudev-device-private.c
@@ -86,6 +86,7 @@ file:
 	f = fopen(filename, "w");
 	if (f = NULL) {
 		err(udev, "unable to create db file '%s': %m\n", filename);
+		util_create_path_done();
 		return -1;
 		}
 	info(udev, "created db file for '%s' in '%s'\n", udev_device_get_devpath(udev_device), filename);
@@ -114,6 +115,7 @@ file:
 	}
 	fclose(f);
 out:
+	util_create_path_done();
 	return 0;
 }
 
diff --git a/libudev/libudev-queue-private.c b/libudev/libudev-queue-private.c
index 0427b65..f2f3905 100644
--- a/libudev/libudev-queue-private.c
+++ b/libudev/libudev-queue-private.c
@@ -417,6 +417,7 @@ static void update_failed(struct udev_queue_export *udev_queue_export,
 		udev_selinux_setfscreatecon(udev, filename, S_IFLNK);
 		symlink(udev_device_get_devpath(udev_device), filename);
 		udev_selinux_resetfscreatecon(udev);
+		util_create_path_done();
 		break;
 
 	case DEVICE_QUEUED:
diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index c309945..9d205d4 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -25,6 +25,29 @@
 #include "libudev.h"
 #include "libudev-private.h"
 
+static int util_path_lock_fd = -1;
+
+static void util_path_unlock(void)
+{
+	if (util_path_lock_fd >= 0) {
+		close(util_create_path_lock_fd);
+		util_create_path_lock_fd = -1;
+	}
+}
+
+static int util_path_lock(void)
+{
+	if (util_path_lock_fd < 0) {
+		if ((util_path_lock_fd = open("/dev/udev_util_path_lock", O_WRONLY | O_CREAT)) < 0)
+			return -1;
+		if (flock(util_path_lock_fd, LOCK_EX)) {
+			util_path_unlock();
+			return -1;
+		}
+	}
+	return 0;
+}
+
 int util_create_path(struct udev *udev, const char *path)
 {
 	char p[UTIL_PATH_SIZE];
@@ -32,6 +55,8 @@ int util_create_path(struct udev *udev, const char *path)
 	struct stat stats;
 	int ret;
 
+	if (util_path_lock())
+		return -1;
 	util_strscpy(p, sizeof(p), path);
 	pos = strrchr(p, '/');
 	if (pos = NULL)
@@ -63,6 +88,11 @@ int util_create_path(struct udev *udev, const char *path)
 	return -1;
 }
 
+void util_create_path_done(void)
+{
+	util_path_unlock();
+}
+
 int util_delete_path(struct udev *udev, const char *path)
 {
 	char p[UTIL_PATH_SIZE];
@@ -75,6 +105,8 @@ int util_delete_path(struct udev *udev, const char *path)
 	if (pos = p || pos = NULL)
 		return 0;
 
+	if (util_path_lock())
+		return 0;
 	while (1) {
 		*pos = '\0';
 		pos = strrchr(p, '/');
@@ -88,13 +120,13 @@ int util_delete_path(struct udev *udev, const char *path)
 		if (errno = ENOENT)
 			retval = 0;
 		if (retval) {
-			if (errno = ENOTEMPTY)
-				return 0;
-			err(udev, "rmdir(%s) failed: %m\n", p);
+			if (errno != ENOTEMPTY)
+				err(udev, "rmdir(%s) failed: %m\n", p);
 			break;
 		}
 		dbg(udev, "removed '%s'\n", p);
 	}
+	util_path_unlock();
 	return 0;
 }
 
diff --git a/udev/udev-node.c b/udev/udev-node.c
index d73bc6c..74de922 100644
--- a/udev/udev-node.c
+++ b/udev/udev-node.c
@@ -48,6 +48,7 @@ static int name_index(struct udev *udev, const char *devpath, const char *name,
 		dbg(udev, "creating index: '%s'\n", filename);
 		util_create_path(udev, filename);
 		fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, 0644);
+		util_create_path_done();
 		if (fd > 0)
 			close(fd);
 	} else {
@@ -356,6 +357,7 @@ static int update_link(struct udev_device *dev, const char *slink)
 	info(udev, "'%s' with target '%s' has the highest priority %i, create it\n", slink, target, priority);
 	util_create_path(udev, slink);
 	node_symlink(udev, target, slink);
+	util_create_path_done();
 out:
 	return rc;
 }
@@ -456,6 +458,7 @@ int udev_node_add(struct udev_device *dev, mode_t mode, uid_t uid, gid_t gid)
 		update_link(dev, udev_list_entry_get_name(list_entry));
 	}
 exit:
+	util_create_path_done();
 	return err;
 }
 
diff --git a/udev/udev-rules.c b/udev/udev-rules.c
index 54a54c3..f19ead8 100644
--- a/udev/udev-rules.c
+++ b/udev/udev-rules.c
@@ -1733,6 +1733,7 @@ struct udev_rules *udev_rules_new(struct udev *udev, int resolve_names)
 			udev_selinux_setfscreatecon(udev, filename, S_IFDIR|0755);
 			mkdir(filename, 0755);
 			udev_selinux_resetfscreatecon(udev);
+			util_create_path_done();
 		}
 		udev_list_init(&sort_list);
 		add_matching_files(udev, &sort_list, filename, ".rules");
diff --git a/udev/udev-watch.c b/udev/udev-watch.c
index 5a49c96..3eb45a1 100644
--- a/udev/udev-watch.c
+++ b/udev/udev-watch.c
@@ -127,6 +127,7 @@ void udev_watch_begin(struct udev *udev, struct udev_device *dev)
 	util_create_path(udev, filename);
 	unlink(filename);
 	symlink(udev_device_get_devpath(dev), filename);
+	util_create_path_done();
 
 	udev_device_set_watch_handle(dev, wd);
 }

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Lennart Poettering]

On Fri, 04.09.09 21:27, Florian Zumbiehl (florz@florz.de) wrote:

> plus, it seems to be the simpler solution by far - which is why I went
> with flock() in my suggested fix you find below. Untested, as usual, and
> I don't really have a clue whether there are any unhealthy interactions
> with the rest of udev in this solution. Also, you most likely will want
> to change the path of the lock file ;-)

Never use flock.

All Unix locking APIs are broken in one way or the other. But if you
use one then at least use fcntl(F_SETLK) which is the least broken.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 9/4/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> That would limit paralellism to some degree.
>>
>> Retrys won't be needed very often; they would surely have a lower
>> overhead.  Less code too :-).  It's not completely out of left field;
>> both seqlocks and transactional memory use retries.  There's no risk
>> of livelock (i.e. infinite retries)...
>>
>> I'm lazy and don't want udev to run measurably slower.  The only
>> advantage I see of flock() is that it tells the reader "I'm a
>> synchronization primitive, solving a concurrency problem here".  I
>> think we can get close enough by explaining the race in a comment next
>> to the definition of create_path().
>
> plus, it seems to be the simpler solution by far - which is why I went
> with flock() in my suggested fix you find below. Untested, as usual, and
> I don't really have a clue whether there are any unhealthy interactions
> with the rest of udev in this solution. Also, you most likely will want
> to change the path of the lock file ;-)
>
> Florian


>  int util_create_path(struct udev *udev, const char *path)
>  {
>  	char p[UTIL_PATH_SIZE];
> @@ -32,6 +55,8 @@ int util_create_path(struct udev *udev, const char *path)
>  	struct stat stats;
>  	int ret;
>
> +	if (util_path_lock())
> +		return -1;

I think the patch missed some call sites.  Each create_path() needs to
be followed by a create_path_done(), but I can see nine calls to
create_path() calls and only about six to create_path_done().

>diff --git a/udev/udev-node.c b/udev/udev-node.c
> index d73bc6c..74de922 100644
> --- a/udev/udev-node.c
> +++ b/udev/udev-node.c
> @@ -48,6 +48,7 @@ static int name_index(struct udev *udev, const char *devpath, >const char *name,
>                dbg(udev, "creating index: '%s'\n", filename);
>                util_create_path(udev, filename);
>                fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, 0644);
> +               util_create_path_done();
>                if (fd > 0)
>                        close(fd);
>        } else {
> @@ -356,6 +357,7 @@ static int update_link(struct udev_device *dev, const char >*slink)
>        info(udev, "'%s' with target '%s' has the highest priority %i, create it\n", slink, >target, priority);
>        util_create_path(udev, slink);
>        node_symlink(udev, target, slink);
> +       util_create_path_done();
>  out:
>        return rc;
>  }
> @@ -456,6 +458,7 @@ int udev_node_add(struct udev_device *dev, mode_t mode, >uid_t uid, gid_t gid)
>                update_link(dev, udev_list_entry_get_name(list_entry));
>        }
>  exit:
> +       util_create_path_done();
>        return err;
>  }

I think there's locking recursion here, because we end up calling
name_index()  between create_path() and create_path_done().  Looking
at the definition of unlock(), that's not going to work.

Regards
Alan

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> On Fri, 04.09.09 21:27, Florian Zumbiehl (florz@florz.de) wrote:
> 
> > plus, it seems to be the simpler solution by far - which is why I went
> > with flock() in my suggested fix you find below. Untested, as usual, and
> > I don't really have a clue whether there are any unhealthy interactions
> > with the rest of udev in this solution. Also, you most likely will want
> > to change the path of the lock file ;-)
> 
> Never use flock.
> 
> All Unix locking APIs are broken in one way or the other. But if you
> use one then at least use fcntl(F_SETLK) which is the least broken.

What exactly is broken in flock() (that affects this use)?

Florian

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Lennart Poettering]

On Sat, 05.09.09 19:11, Florian Zumbiehl (florz@florz.de) wrote:

> > All Unix locking APIs are broken in one way or the other. But if you
> > use one then at least use fcntl(F_SETLK) which is the least broken.
> 
> What exactly is broken in flock() (that affects this use)?

The simple fact that fcntl() style locks are the ones that are least
broken and using everything else is confusing since the different lock
styles are independant of each other.

So if you do use locking then use fcntl. 

Now, fcntl is broken in many ways. For example it is unusable for
files that are accessible for other users. It is bound to a process
and not to an fd, and hence useless in threads, and in libraries. The
first close() breaks all locks of a process on a file even if it has
more fds open to it.

In this case you might be able to get away with a lot, but still
that leaves that fcntl is POSIX and the least broken and is the only
flavour of locking that should be used these days.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> >  int util_create_path(struct udev *udev, const char *path)
> >  {
> >  	char p[UTIL_PATH_SIZE];
> > @@ -32,6 +55,8 @@ int util_create_path(struct udev *udev, const char *path)
> >  	struct stat stats;
> >  	int ret;
> >
> > +	if (util_path_lock())
> > +		return -1;
> 
> I think the patch missed some call sites.  Each create_path() needs to
> be followed by a create_path_done(), but I can see nine calls to
> create_path() calls and only about six to create_path_done().

Hu? I see 8 call sites, and those should all be covered - maybe it's just
because I'm working on a somewhat old tree?

[...]
> I think there's locking recursion here, because we end up calling
> name_index()  between create_path() and create_path_done().  Looking
> at the definition of unlock(), that's not going to work.

Below is a new patch that should work with recursive locking (of limited
depth - I hope 2^31-1 levels of recursion is sufficient? :-)

Untested, uncompiled, etc., as usual ...

Florian

diff --git a/libudev/libudev-device-private.c b/libudev/libudev-device-private.c
index 68dc0a5..d386135 100644
--- a/libudev/libudev-device-private.c
+++ b/libudev/libudev-device-private.c
@@ -44,7 +44,8 @@ int udev_device_update_db(struct udev_device *udev_device)
 	int ret;
 
 	devpath_to_db_path(udev, udev_device_get_devpath(udev_device), filename, sizeof(filename));
-	util_create_path(udev, filename);
+	if (util_create_path(udev, filename))
+		return -1;
 	unlink(filename);
 
 	udev_list_entry_foreach(list_entry, udev_device_get_properties_list_entry(udev_device))
@@ -86,6 +87,7 @@ file:
 	f = fopen(filename, "w");
 	if (f = NULL) {
 		err(udev, "unable to create db file '%s': %m\n", filename);
+		util_create_path_done();
 		return -1;
 		}
 	info(udev, "created db file for '%s' in '%s'\n", udev_device_get_devpath(udev_device), filename);
@@ -114,6 +116,7 @@ file:
 	}
 	fclose(f);
 out:
+	util_create_path_done();
 	return 0;
 }
 
diff --git a/libudev/libudev-queue-private.c b/libudev/libudev-queue-private.c
index 0427b65..f2f3905 100644
--- a/libudev/libudev-queue-private.c
+++ b/libudev/libudev-queue-private.c
@@ -417,6 +417,7 @@ static void update_failed(struct udev_queue_export *udev_queue_export,
 		udev_selinux_setfscreatecon(udev, filename, S_IFLNK);
 		symlink(udev_device_get_devpath(udev_device), filename);
 		udev_selinux_resetfscreatecon(udev);
+		util_create_path_done();
 		break;
 
 	case DEVICE_QUEUED:
diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index c309945..26fbb5c 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -25,6 +25,28 @@
 #include "libudev.h"
 #include "libudev-private.h"
 
+static int util_path_lock_cnt,util_path_lock_fd;
+
+static int util_path_lock(void)
+{
+	if (!util_path_lock_cnt) {
+		if ((util_path_lock_fd = open("/dev/udev_util_path_lock", O_WRONLY | O_CREAT)) < 0)
+			return -1;
+		if (flock(util_path_lock_fd, LOCK_EX)) {
+			close(util_create_path_lock_fd);
+			return -1;
+		}
+		util_path_lock_cnt++;
+	}
+	return 0;
+}
+
+static void util_path_unlock(void)
+{
+	if (!--util_path_lock_cnt)
+		close(util_create_path_lock_fd);
+}
+
 int util_create_path(struct udev *udev, const char *path)
 {
 	char p[UTIL_PATH_SIZE];
@@ -32,6 +54,8 @@ int util_create_path(struct udev *udev, const char *path)
 	struct stat stats;
 	int ret;
 
+	if (util_path_lock())
+		return -1;
 	util_strscpy(p, sizeof(p), path);
 	pos = strrchr(p, '/');
 	if (pos = NULL)
@@ -47,8 +71,10 @@ int util_create_path(struct udev *udev, const char *path)
 	if (stat(p, &stats) = 0 && (stats.st_mode & S_IFMT) = S_IFDIR)
 		return 0;
 
-	if (util_create_path(udev, p) != 0)
+	if (util_create_path(udev, p) != 0) {
+		util_path_unlock();
 		return -1;
+	}
 
 	dbg(udev, "mkdir '%s'\n", p);
 	udev_selinux_setfscreatecon(udev, p, S_IFDIR|0755);
@@ -60,9 +86,15 @@ int util_create_path(struct udev *udev, const char *path)
 	if (ret = 0)
 		return 0;
 
+	util_path_unlock();
 	return -1;
 }
 
+void util_create_path_done(void)
+{
+	util_path_unlock();
+}
+
 int util_delete_path(struct udev *udev, const char *path)
 {
 	char p[UTIL_PATH_SIZE];
@@ -75,6 +107,8 @@ int util_delete_path(struct udev *udev, const char *path)
 	if (pos = p || pos = NULL)
 		return 0;
 
+	if (util_path_lock())
+		return 0;
 	while (1) {
 		*pos = '\0';
 		pos = strrchr(p, '/');
@@ -88,13 +122,13 @@ int util_delete_path(struct udev *udev, const char *path)
 		if (errno = ENOENT)
 			retval = 0;
 		if (retval) {
-			if (errno = ENOTEMPTY)
-				return 0;
-			err(udev, "rmdir(%s) failed: %m\n", p);
+			if (errno != ENOTEMPTY)
+				err(udev, "rmdir(%s) failed: %m\n", p);
 			break;
 		}
 		dbg(udev, "removed '%s'\n", p);
 	}
+	util_path_unlock();
 	return 0;
 }
 
diff --git a/udev/udev-node.c b/udev/udev-node.c
index d73bc6c..4124485 100644
--- a/udev/udev-node.c
+++ b/udev/udev-node.c
@@ -46,10 +46,12 @@ static int name_index(struct udev *udev, const char *devpath, const char *name,
 
 	if (add) {
 		dbg(udev, "creating index: '%s'\n", filename);
-		util_create_path(udev, filename);
-		fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, 0644);
-		if (fd > 0)
-			close(fd);
+		if (!util_create_path(udev, filename)) {
+			fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, 0644);
+			util_create_path_done();
+			if (fd > 0)
+				close(fd);
+		}
 	} else {
 		dbg(udev, "removing index: '%s'\n", filename);
 		unlink(filename);
@@ -354,8 +356,10 @@ static int update_link(struct udev_device *dev, const char *slink)
 
 	/* create symlink to the target with the highest priority */
 	info(udev, "'%s' with target '%s' has the highest priority %i, create it\n", slink, target, priority);
-	util_create_path(udev, slink);
-	node_symlink(udev, target, slink);
+	if (!util_create_path(udev, slink)) {
+		node_symlink(udev, target, slink);
+		util_create_path_done();
+	}
 out:
 	return rc;
 }
@@ -424,7 +428,8 @@ int udev_node_add(struct udev_device *dev, mode_t mode, uid_t uid, gid_t gid)
 	     major(udev_device_get_devnum(dev)), minor(udev_device_get_devnum(dev)),
 	     mode, uid, gid);
 
-	util_create_path(udev, udev_device_get_devnode(dev));
+	if (util_create_path(udev, udev_device_get_devnode(dev)))
+		return -1;
 	if (udev_node_mknod(dev, NULL, makedev(0,0), mode, uid, gid) != 0) {
 		err = -1;
 		goto exit;
@@ -456,6 +461,7 @@ int udev_node_add(struct udev_device *dev, mode_t mode, uid_t uid, gid_t gid)
 		update_link(dev, udev_list_entry_get_name(list_entry));
 	}
 exit:
+	util_create_path_done();
 	return err;
 }
 
diff --git a/udev/udev-rules.c b/udev/udev-rules.c
index 54a54c3..d8f3e7c 100644
--- a/udev/udev-rules.c
+++ b/udev/udev-rules.c
@@ -1729,10 +1729,12 @@ struct udev_rules *udev_rules_new(struct udev *udev, int resolve_names)
 		/* read dynamic/temporary rules */
 		util_strscpyl(filename, sizeof(filename), udev_get_dev_path(udev), "/.udev/rules.d", NULL);
 		if (stat(filename, &statbuf) != 0) {
-			util_create_path(udev, filename);
-			udev_selinux_setfscreatecon(udev, filename, S_IFDIR|0755);
-			mkdir(filename, 0755);
-			udev_selinux_resetfscreatecon(udev);
+			if (!util_create_path(udev, filename)) {
+				udev_selinux_setfscreatecon(udev, filename, S_IFDIR|0755);
+				mkdir(filename, 0755);
+				udev_selinux_resetfscreatecon(udev);
+				util_create_path_done();
+			}
 		}
 		udev_list_init(&sort_list);
 		add_matching_files(udev, &sort_list, filename, ".rules");
diff --git a/udev/udev-watch.c b/udev/udev-watch.c
index 5a49c96..744d9cd 100644
--- a/udev/udev-watch.c
+++ b/udev/udev-watch.c
@@ -124,9 +124,11 @@ void udev_watch_begin(struct udev *udev, struct udev_device *dev)
 	}
 
 	snprintf(filename, sizeof(filename), "%s/.udev/watch/%d", udev_get_dev_path(udev), wd);
-	util_create_path(udev, filename);
-	unlink(filename);
-	symlink(udev_device_get_devpath(dev), filename);
+	if (!util_create_path(udev, filename)) {
+		unlink(filename);
+		symlink(udev_device_get_devpath(dev), filename);
+		util_create_path_done();
+	}
 
 	udev_device_set_watch_handle(dev, wd);
 }

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 9/7/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> >  int util_create_path(struct udev *udev, const char *path)
>> >  {
>> >  	char p[UTIL_PATH_SIZE];
>> > @@ -32,6 +55,8 @@ int util_create_path(struct udev *udev, const char
>> > *path)
>> >  	struct stat stats;
>> >  	int ret;
>> >
>> > +	if (util_path_lock())
>> > +		return -1;
>>
>> I think the patch missed some call sites.  Each create_path() needs to
>> be followed by a create_path_done(), but I can see nine calls to
>> create_path() calls and only about six to create_path_done().
>
> Hu? I see 8 call sites, and those should all be covered - maybe it's just
> because I'm working on a somewhat old tree?

The ninth site was the recursive call within create_path().  I see
you've added unlock()s there now.  I re-counted on a per-file basis
and everything else looks covered; I probably miscounted before.

I'm still concerned/interested about the effect on performance. But
since I read the patch, here are some style suggestions.

> [...]
>> I think there's locking recursion here, because we end up calling
>> name_index()  between create_path() and create_path_done().  Looking
>> at the definition of unlock(), that's not going to work.
>
> Below is a new patch that should work with recursive locking (of limited
> depth - I hope 2^31-1 levels of recursion is sufficient? :-)
>
> Untested, uncompiled, etc., as usual ...
>
> Florian
>
> diff --git a/libudev/libudev-device-private.c
> b/libudev/libudev-device-private.c
> index 68dc0a5..d386135 100644
> --- a/libudev/libudev-device-private.c
> +++ b/libudev/libudev-device-private.c
> @@ -44,7 +44,8 @@ int udev_device_update_db(struct udev_device *udev_device)
>  	int ret;
>
>  	devpath_to_db_path(udev, udev_device_get_devpath(udev_device), filename,
> sizeof(filename));
> -	util_create_path(udev, filename);
> +	if (util_create_path(udev, filename))
> +		return -1;
>  	unlink(filename);
>
>  	udev_list_entry_foreach(list_entry,
> udev_device_get_properties_list_entry(udev_device))
> @@ -86,6 +87,7 @@ file:
>  	f = fopen(filename, "w");
>  	if (f = NULL) {
>  		err(udev, "unable to create db file '%s': %m\n", filename);
> +		util_create_path_done();
>  		return -1;
>  		}
>  	info(udev, "created db file for '%s' in '%s'\n",
> udev_device_get_devpath(udev_device), filename);
> @@ -114,6 +116,7 @@ file:
>  	}
>  	fclose(f);
>  out:
> +	util_create_path_done();
>  	return 0;
>  }
>
> diff --git a/libudev/libudev-queue-private.c
> b/libudev/libudev-queue-private.c
> index 0427b65..f2f3905 100644
> --- a/libudev/libudev-queue-private.c
> +++ b/libudev/libudev-queue-private.c
> @@ -417,6 +417,7 @@ static void update_failed(struct udev_queue_export
> *udev_queue_export,
>  		udev_selinux_setfscreatecon(udev, filename, S_IFLNK);
>  		symlink(udev_device_get_devpath(udev_device), filename);
>  		udev_selinux_resetfscreatecon(udev);
> +		util_create_path_done();
>  		break;
>
>  	case DEVICE_QUEUED:
> diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> index c309945..26fbb5c 100644
> --- a/libudev/libudev-util-private.c
> +++ b/libudev/libudev-util-private.c
> @@ -25,6 +25,28 @@
>  #include "libudev.h"
>  #include "libudev-private.h"
>
> +static int util_path_lock_cnt,util_path_lock_fd;

static int util_path_lock_cnt;
static int util_path_lock_fd;

> +static int util_path_lock(void)
> +{
> +	if (!util_path_lock_cnt) {
> +		if ((util_path_lock_fd = open("/dev/udev_util_path_lock", O_WRONLY |
> O_CREAT)) < 0)
> +			return -1;
> +		if (flock(util_path_lock_fd, LOCK_EX)) {
> +			close(util_create_path_lock_fd);
> +			return -1;
> +		}
> +		util_path_lock_cnt++;
> +	}
> +	return 0;
> +}
> +
> +static void util_path_unlock(void)
> +{
> +	if (!--util_path_lock_cnt)
> +		close(util_create_path_lock_fd);
> +}
> +
>  int util_create_path(struct udev *udev, const char *path)
>  {
>  	char p[UTIL_PATH_SIZE];
> @@ -32,6 +54,8 @@ int util_create_path(struct udev *udev, const char *path)
>  	struct stat stats;
>  	int ret;
>
> +	if (util_path_lock())
> +		return -1;
>
>  	util_strscpy(p, sizeof(p), path);
>  	pos = strrchr(p, '/');
>  	if (pos = NULL)
> @@ -47,8 +71,10 @@ int util_create_path(struct udev *udev, const char *path)
>  	if (stat(p, &stats) = 0 && (stats.st_mode & S_IFMT) = S_IFDIR)
>  		return 0;
>

> -	if (util_create_path(udev, p) != 0)
> +	if (util_create_path(udev, p) != 0) {
> +		util_path_unlock();
>  		return -1;
> +	}

Looks suspicious.  It'll try to unlock even If the recursive
util_create_path() bailed because it failed at util_path_lock().

I suspect that can't happen, but this seems needlessly tricky.  How
about just renaming the old create_path() to create_path_unlocked() or
something (updating the recursive call accordingly), and then

int util_create_path(struct udev *udev, const char *path)
{
	if (!util_path_lock())
		return -1;
	return util_path_create_unlocked(udev, path);
}

>  	dbg(udev, "mkdir '%s'\n", p);
>  	udev_selinux_setfscreatecon(udev, p, S_IFDIR|0755);
> @@ -60,9 +86,15 @@ int util_create_path(struct udev *udev, const char *path)
>  	if (ret = 0)
>  		return 0;
>
> +	util_path_unlock();
>  	return -1;
>  }
>
> +void util_create_path_done(void)
> +{
> +	util_path_unlock();
> +}
> +
>  int util_delete_path(struct udev *udev, const char *path)
>  {
>  	char p[UTIL_PATH_SIZE];
> @@ -75,6 +107,8 @@ int util_delete_path(struct udev *udev, const char *path)
>  	if (pos = p || pos = NULL)
>  		return 0;
>
> +	if (util_path_lock())
> +		return 0;
>  	while (1) {
>  		*pos = '\0';
>  		pos = strrchr(p, '/');
> @@ -88,13 +122,13 @@ int util_delete_path(struct udev *udev, const char
> *path)
>  		if (errno = ENOENT)
>  			retval = 0;
>  		if (retval) {
> -			if (errno = ENOTEMPTY)
> -				return 0;
> -			err(udev, "rmdir(%s) failed: %m\n", p);
> +			if (errno != ENOTEMPTY)
> +				err(udev, "rmdir(%s) failed: %m\n", p);
>  			break;
>  		}
>  		dbg(udev, "removed '%s'\n", p);
>  	}
> +	util_path_unlock();
>  	return 0;
>  }

Hmm, does this function ever return non-zero?  Is it time to admit
that it returns void?

Regards
Alan

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 9/7/09, Alan Jenkins <sourcejedi.lkml@googlemail.com> wrote:
>>  int util_create_path(struct udev *udev, const char *path)
>>  {
>>  	char p[UTIL_PATH_SIZE];
>> @@ -32,6 +54,8 @@ int util_create_path(struct udev *udev, const char
>> *path)
>>  	struct stat stats;
>>  	int ret;
>>
>> +	if (util_path_lock())
>> +		return -1;
>>
>>  	util_strscpy(p, sizeof(p), path);
>>  	pos = strrchr(p, '/');
>>  	if (pos = NULL)
>> @@ -47,8 +71,10 @@ int util_create_path(struct udev *udev, const char
>> *path)
>>  	if (stat(p, &stats) = 0 && (stats.st_mode & S_IFMT) = S_IFDIR)
>>  		return 0;
>>
>
>> -	if (util_create_path(udev, p) != 0)
>> +	if (util_create_path(udev, p) != 0) {
>> +		util_path_unlock();
>>  		return -1;
>> +	}
>
> Looks suspicious.  It'll try to unlock even If the recursive
> util_create_path() bailed because it failed at util_path_lock().
>
> I suspect that can't happen, but this seems needlessly tricky.  How
> about just renaming the old create_path() to create_path_unlocked() or
> something (updating the recursive call accordingly), and then
>
> int util_create_path(struct udev *udev, const char *path)
> {
> 	if (!util_path_lock())
> 		return -1;
> 	return util_path_create_unlocked(udev, path);
> }

Wups, that '!' shouldn't be there.

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> > Hu? I see 8 call sites, and those should all be covered - maybe it's just
> > because I'm working on a somewhat old tree?
> 
> The ninth site was the recursive call within create_path().  I see
> you've added unlock()s there now.  I re-counted on a per-file basis
> and everything else looks covered; I probably miscounted before.

That unlock wasn't necessary before.

> I'm still concerned/interested about the effect on performance. But

I guess that somebody should try it out ;-)

> > -	if (util_create_path(udev, p) != 0)
> > +	if (util_create_path(udev, p) != 0) {
> > +		util_path_unlock();
> >  		return -1;
> > +	}
> 
> Looks suspicious.  It'll try to unlock even If the recursive
> util_create_path() bailed because it failed at util_path_lock().

s/even/only/, and that's the whole point of it.

This is not for unlocking the recursive lock - if util_create_path()
returns non-zero, it is guaranteed that it hasn't acquired a lock,
which is also why we have to drop our own lock before returning -1.

> >  int util_delete_path(struct udev *udev, const char *path)
[...]
> Hmm, does this function ever return non-zero?  Is it time to admit
> that it returns void?

Just my thoughts ;-)

And even if it returned anything else, nobody cares anyhow ...

Florian

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Kay Sievers]

On Mon, Sep 7, 2009 at 19:27, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> > Hu? I see 8 call sites, and those should all be covered - maybe it's just
>> > because I'm working on a somewhat old tree?
>>
>> The ninth site was the recursive call within create_path().  I see
>> you've added unlock()s there now.  I re-counted on a per-file basis
>> and everything else looks covered; I probably miscounted before.
>
> That unlock wasn't necessary before.
>
>> I'm still concerned/interested about the effect on performance. But
>
> I guess that somebody should try it out ;-)

I'm pretty sure we should not introduce any sort of global
serialization locks here. We have to handle thousands events in
parallel on some boxes, and most of them don't need any of theses
locks.

Why can't we just create the subdir and the link or node inside one
and the same retry-loop? The link or node will pin the subdir and any
competing remove will fail after that. We might in theory loop a while
until we successfully create the dir and the node or link, but in real
world setups it seems it just does not happen. That way we don't put
possibly expensive stuff in the common code path.

Kay

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> >> I'm still concerned/interested about the effect on performance. But
> >
> > I guess that somebody should try it out ;-)
> 
> I'm pretty sure we should not introduce any sort of global
> serialization locks here. We have to handle thousands events in
> parallel on some boxes, and most of them don't need any of theses
> locks.
> 
> Why can't we just create the subdir and the link or node inside one
> and the same retry-loop? The link or node will pin the subdir and any

that certainly is _possible_ - it just seems to be pretty difficult
to do, at least for me. The syscall for creating the object is not
necessarily the immediate next thing after the call to
util_create_path(), ...

> competing remove will fail after that. We might in theory loop a while
> until we successfully create the dir and the node or link, but in real
> world setups it seems it just does not happen. That way we don't put
> possibly expensive stuff in the common code path.

... which, of course, possibly is exactly the problem in that regard,
too.

Florian

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

BTW ...

> until we successfully create the dir and the node or link, but in real
> world setups it seems it just does not happen. That way we don't put

How do you know that?

Florian

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Kay Sievers]

On Mon, Sep 7, 2009 at 22:23, Florian Zumbiehl <florz@florz.de> wrote:
>> until we successfully create the dir and the node or link, but in real
>> world setups it seems it just does not happen. That way we don't put
>
> How do you know that?

Competing events for the same subdir but different devices are pretty
rare. And it's very unlikely that shared directories ever get empty,
most of them have always some device which stays around and pins the
subdir. And there is no such known problem ever reported.

Kay

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Kay Sievers]

On Mon, Sep 7, 2009 at 22:13, Florian Zumbiehl <florz@florz.de> wrote:
>> >> I'm still concerned/interested about the effect on performance. But
>> >
>> > I guess that somebody should try it out ;-)
>>
>> I'm pretty sure we should not introduce any sort of global
>> serialization locks here. We have to handle thousands events in
>> parallel on some boxes, and most of them don't need any of theses
>> locks.
>>
>> Why can't we just create the subdir and the link or node inside one
>> and the same retry-loop? The link or node will pin the subdir and any
>
> that certainly is _possible_ - it just seems to be pretty difficult
> to do, at least for me. The syscall for creating the object is not
> necessarily the immediate next thing after the call to
> util_create_path(), ...

I guess we could do some:

  ensure_path(/dev/foo/bar) {
    selinux_something(...)
    mknod(/dev/foo/bar, ...);
    selinux_someting(...);
  }

macro hackery to wrap a bunch of instructions in a retry loop?

Kay

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Florian Zumbiehl]

Hi,

> On Mon, Sep 7, 2009 at 22:23, Florian Zumbiehl <florz@florz.de> wrote:
> >> until we successfully create the dir and the node or link, but in real
> >> world setups it seems it just does not happen. That way we don't put
> >
> > How do you know that?
> 
> Competing events for the same subdir but different devices are pretty
> rare. And it's very unlikely that shared directories ever get empty,
> most of them have always some device which stays around and pins the

It seems unlikely per call to those functions, sure. But given the
number of installations, and the number of calls per installation ...

> subdir. And there is no such known problem ever reported.

Now, how likely is it that when this happens once in a setup and it's
not reproducible, it would get reported?

As an argument for a retry loop instead of locking, that seems fine -
but to conclude from that that it seems not to happen in real world
setups, I think, is not appropriate.

Florian

Re: [PATCH] race between util_create_path() and util_delete_path()?

[Alan Jenkins]

On 9/7/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> On Mon, Sep 7, 2009 at 22:23, Florian Zumbiehl <florz@florz.de> wrote:
>> >> until we successfully create the dir and the node or link, but in real
>> >> world setups it seems it just does not happen. That way we don't put
>> >
>> > How do you know that?
>>
>> Competing events for the same subdir but different devices are pretty
>> rare. And it's very unlikely that shared directories ever get empty,
>> most of them have always some device which stays around and pins the
>
> It seems unlikely per call to those functions, sure. But given the
> number of installations, and the number of calls per installation ...
>
>> subdir. And there is no such known problem ever reported.
>
> Now, how likely is it that when this happens once in a setup and it's
> not reproducible, it would get reported?
>
> As an argument for a retry loop instead of locking, that seems fine -
> but to conclude from that that it seems not to happen in real world
> setups, I think, is not appropriate.
>
> Florian
>

One scenario which came to mind was quickly moving an SD card between
card readers, where (for some hardware) we rely on HAL polling to
detect media changes.

The by-disk/label links could then be subject to this race, assuming
thereare no other labelled filesystems.  And I believe linux systems
are often installed without labelling the filesystems.

So IMO this will occasionally happen on real-world systems.  Even
though it's a contrived sequence of events, and it's unlikely to
matter because no-one relies on disk/by-label links on removable
media.  There could be scenarios I've missed.  And they do seem even
less likely to be reported than they are to happen :-).

If this can be fixed as simply as adding a loop around create_path()
and mknod(), I think we should.  (Plus some re-arranging to avoid
nesting the loops in udev-node.c).

Regards
Alan