From: Karel Zak <kzak@redhat.com>
Date: Fri, 19 Jan 2024 11:58:50 +0000 (+0100)
Subject: libmount: cleanup locking in table update code
X-Git-Tag: v2.40-rc1~34^2~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b94b5929a63df9786020e0329e94d2f35f3993ae;p=thirdparty%2Futil-linux.git

libmount: cleanup locking in table update code

* use reference counting for libmnt_lock

* remove lock use from mnt_update_already_done(), it's read-only
  operation and utab update is atomic (based on rename(2))

* keep the lock instance mandatory for all low-level update_*
  functions. The lock is always initialized, so all the 'if(lc)'
  are unnecessary.

Signed-off-by: Karel Zak <kzak@redhat.com>
---

diff --git a/libmount/src/context.c b/libmount/src/context.c
index ed05e7ca39..62cd349481 100644
--- a/libmount/src/context.c
+++ b/libmount/src/context.c
@@ -2207,7 +2207,7 @@ int mnt_context_update_tabs(struct libmnt_context *cxt)
 	    && mnt_context_get_helper_status(cxt) == 0
 	    && mnt_context_utab_writable(cxt)) {
 
-		if (mnt_update_already_done(cxt->update, cxt->lock)) {
+		if (mnt_update_already_done(cxt->update)) {
 			DBG(CXT, ul_debugobj(cxt, "don't update: error evaluate or already updated"));
 			goto emit;
 		}
diff --git a/libmount/src/mountP.h b/libmount/src/mountP.h
index 704e940827..08d08caa14 100644
--- a/libmount/src/mountP.h
+++ b/libmount/src/mountP.h
@@ -665,8 +665,7 @@ extern struct libmnt_optlist *mnt_context_get_optlist(struct libmnt_context *cxt
 /* tab_update.c */
 extern int mnt_update_emit_event(struct libmnt_update *upd);
 extern int mnt_update_set_filename(struct libmnt_update *upd, const char *filename);
-extern int mnt_update_already_done(struct libmnt_update *upd,
-				   struct libmnt_lock *lc);
+extern int mnt_update_already_done(struct libmnt_update *upd);
 
 #if __linux__
 /* btrfs.c */
diff --git a/libmount/src/tab_update.c b/libmount/src/tab_update.c
index 6a2c77b39c..753e22c083 100644
--- a/libmount/src/tab_update.c
+++ b/libmount/src/tab_update.c
@@ -40,6 +40,7 @@ struct libmnt_update {
 			missing_options : 1;
 
 	struct libmnt_table *mountinfo;
+	struct libmnt_lock  *lock;
 };
 
 static int set_fs_root(struct libmnt_update *upd, struct libmnt_fs *fs, unsigned long mountflags);
@@ -75,6 +76,7 @@ void mnt_free_update(struct libmnt_update *upd)
 
 	DBG(UPDATE, ul_debugobj(upd, "free"));
 
+	mnt_unref_lock(upd->lock);
 	mnt_unref_fs(upd->fs);
 	mnt_unref_table(upd->mountinfo);
 	free(upd->target);
@@ -668,43 +670,42 @@ static int add_file_entry(struct libmnt_table *tb, struct libmnt_update *upd)
 	return update_table(upd, tb);
 }
 
-static int update_add_entry(struct libmnt_update *upd, struct libmnt_lock *lc)
+static int update_add_entry(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb;
 	int rc = 0;
 
 	assert(upd);
 	assert(upd->fs);
+	assert(upd->lock);
 
 	DBG(UPDATE, ul_debugobj(upd, "%s: add entry", upd->filename));
 
-	if (lc)
-		rc = mnt_lock_file(lc);
+	rc = mnt_lock_file(upd->lock);
 	if (rc)
 		return -MNT_ERR_LOCK;
 
 	tb = __mnt_new_table_from_file(upd->filename, MNT_FMT_UTAB, 1);
 	if (tb)
 		rc = add_file_entry(tb, upd);
-	if (lc)
-		mnt_unlock_file(lc);
 
+	mnt_unlock_file(upd->lock);
 	mnt_unref_table(tb);
 	return rc;
 }
 
-static int update_remove_entry(struct libmnt_update *upd, struct libmnt_lock *lc)
+static int update_remove_entry(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb;
 	int rc = 0;
 
 	assert(upd);
 	assert(upd->target);
+	assert(upd->lock);
 
 	DBG(UPDATE, ul_debugobj(upd, "%s: remove entry", upd->filename));
 
-	if (lc)
-		rc = mnt_lock_file(lc);
+	rc = mnt_lock_file(upd->lock);
 	if (rc)
 		return -MNT_ERR_LOCK;
 
@@ -716,23 +717,22 @@ static int update_remove_entry(struct libmnt_update *upd, struct libmnt_lock *lc
 			rc = update_table(upd, tb);
 		}
 	}
-	if (lc)
-		mnt_unlock_file(lc);
 
+	mnt_unlock_file(upd->lock);
 	mnt_unref_table(tb);
 	return rc;
 }
 
-static int update_modify_target(struct libmnt_update *upd, struct libmnt_lock *lc)
+static int update_modify_target(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb = NULL;
 	int rc = 0;
 
 	assert(upd);
+	assert(upd->lock);
 	DBG(UPDATE, ul_debugobj(upd, "%s: modify target", upd->filename));
 
-	if (lc)
-		rc = mnt_lock_file(lc);
+	rc = mnt_lock_file(upd->lock);
 	if (rc)
 		return -MNT_ERR_LOCK;
 
@@ -781,15 +781,13 @@ static int update_modify_target(struct libmnt_update *upd, struct libmnt_lock *l
 	}
 
 done:
-	if (lc)
-		mnt_unlock_file(lc);
-
+	mnt_unlock_file(upd->lock);
 	mnt_unref_table(tb);
 	return rc;
 }
 
 /* replaces option in the table entry by new options from @udp */
-static int update_modify_options(struct libmnt_update *upd, struct libmnt_lock *lc)
+static int update_modify_options(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb = NULL;
 	int rc = 0;
@@ -797,13 +795,13 @@ static int update_modify_options(struct libmnt_update *upd, struct libmnt_lock *
 
 	assert(upd);
 	assert(upd->fs);
+	assert(upd->lock);
 
 	DBG(UPDATE, ul_debugobj(upd, "%s: modify options", upd->filename));
 
 	fs = upd->fs;
 
-	if (lc)
-		rc = mnt_lock_file(lc);
+	rc = mnt_lock_file(upd->lock);
 	if (rc)
 		return -MNT_ERR_LOCK;
 
@@ -822,15 +820,13 @@ static int update_modify_options(struct libmnt_update *upd, struct libmnt_lock *
 			rc = add_file_entry(tb, upd);	/* not found, add new */
 	}
 
-	if (lc)
-		mnt_unlock_file(lc);
-
+	mnt_unlock_file(upd->lock);
 	mnt_unref_table(tb);
 	return rc;
 }
 
 /* add user options missing in the table entry by new options from @udp */
-static int update_add_options(struct libmnt_update *upd, struct libmnt_lock *lc)
+static int update_add_options(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb = NULL;
 	int rc = 0;
@@ -838,6 +834,7 @@ static int update_add_options(struct libmnt_update *upd, struct libmnt_lock *lc)
 
 	assert(upd);
 	assert(upd->fs);
+	assert(upd->lock);
 
 	if (!upd->fs->user_optstr)
 		return 0;
@@ -846,8 +843,7 @@ static int update_add_options(struct libmnt_update *upd, struct libmnt_lock *lc)
 
 	fs = upd->fs;
 
-	if (lc)
-		rc = mnt_lock_file(lc);
+	rc = mnt_lock_file(upd->lock);
 	if (rc)
 		return -MNT_ERR_LOCK;
 
@@ -873,14 +869,30 @@ static int update_add_options(struct libmnt_update *upd, struct libmnt_lock *lc)
 			rc = add_file_entry(tb, upd);	/* not found, add new */
 	}
 
-	if (lc)
-		mnt_unlock_file(lc);
-
+	mnt_unlock_file(upd->lock);
 	mnt_unref_table(tb);
 	return rc;
 
 }
 
+static int update_init_lock(struct libmnt_update *upd, struct libmnt_lock *lc)
+{
+	assert(upd);
+
+	if (lc) {
+		mnt_unref_lock(upd->lock);
+		mnt_ref_lock(lc);
+		upd->lock = lc;
+	} else if (!upd->lock) {
+		upd->lock = mnt_new_lock(upd->filename, 0);
+		if (!upd->lock)
+			return -ENOMEM;
+		mnt_lock_block_signals(lc, TRUE);
+	}
+
+	return 0;
+}
+
 /**
  * mnt_update_table:
  * @upd: update
@@ -897,7 +909,6 @@ static int update_add_options(struct libmnt_update *upd, struct libmnt_lock *lc)
  */
 int mnt_update_table(struct libmnt_update *upd, struct libmnt_lock *lc)
 {
-	struct libmnt_lock *lc0 = lc;
 	int rc = -EINVAL;
 
 	if (!upd || !upd->filename)
@@ -909,35 +920,32 @@ int mnt_update_table(struct libmnt_update *upd, struct libmnt_lock *lc)
 	if (upd->fs) {
 		DBG(UPDATE, mnt_fs_print_debug(upd->fs, stderr));
 	}
-	if (!lc) {
-		lc = mnt_new_lock(upd->filename, 0);
-		if (lc)
-			mnt_lock_block_signals(lc, TRUE);
-	}
+
+	rc = update_init_lock(upd, lc);
+	if (rc)
+		goto done;
 
 	if (!upd->fs && upd->target)
-		rc = update_remove_entry(upd, lc);	/* umount */
+		rc = update_remove_entry(upd);		/* umount */
 	else if (upd->mountflags & MS_MOVE)
-		rc = update_modify_target(upd, lc);	/* move */
+		rc = update_modify_target(upd);		/* move */
 	else if (upd->mountflags & MS_REMOUNT)
-		rc = update_modify_options(upd, lc);	/* remount */
+		rc = update_modify_options(upd);	/* remount */
 	else if (upd->fs && upd->missing_options)
-		rc = update_add_options(upd, lc);	/* mount by externel helper */
+		rc = update_add_options(upd);		/* mount by externel helper */
 	else if (upd->fs)
-		rc = update_add_entry(upd, lc);		/* mount */
+		rc = update_add_entry(upd);		/* mount */
 
 	upd->ready = 1;
+done:
 	DBG(UPDATE, ul_debugobj(upd, "%s: update tab: done [rc=%d]",
 				upd->filename, rc));
-	if (lc != lc0)
-		 mnt_free_lock(lc);
 	return rc;
 }
 
-int mnt_update_already_done(struct libmnt_update *upd, struct libmnt_lock *lc)
+int mnt_update_already_done(struct libmnt_update *upd)
 {
 	struct libmnt_table *tb = NULL;
-	struct libmnt_lock *lc0 = lc;
 	int rc = 0;
 
 	if (!upd || !upd->filename || (!upd->fs && !upd->target))
@@ -945,22 +953,7 @@ int mnt_update_already_done(struct libmnt_update *upd, struct libmnt_lock *lc)
 
 	DBG(UPDATE, ul_debugobj(upd, "%s: checking for previous update", upd->filename));
 
-	if (!lc) {
-		lc = mnt_new_lock(upd->filename, 0);
-		if (lc)
-			mnt_lock_block_signals(lc, TRUE);
-	}
-	if (lc) {
-		rc = mnt_lock_file(lc);
-		if (rc) {
-			rc = -MNT_ERR_LOCK;
-			goto done;
-		}
-	}
-
 	tb = __mnt_new_table_from_file(upd->filename, MNT_FMT_UTAB, 1);
-	if (lc)
-		mnt_unlock_file(lc);
 	if (!tb)
 		goto done;
 
@@ -997,8 +990,6 @@ int mnt_update_already_done(struct libmnt_update *upd, struct libmnt_lock *lc)
 
 	mnt_unref_table(tb);
 done:
-	if (lc && lc != lc0)
-		mnt_free_lock(lc);
 	DBG(UPDATE, ul_debugobj(upd, "%s: previous update check done [rc=%d]",
 				upd->filename, rc));
 	return rc;