[Devel] [PATCH 0/5] dm/push_backup: event fd interface and fixes

[Devel] [PATCH 0/5] dm/push_backup: event fd interface and fixes

[Andrey Zhadchenko]

The push_backup target currently makes the backup tool wait for the next
cluster range inside the "push_backup_read" target message. dm runs
target_message() under dm_get_live_table(), i.e. with md->io_barrier SRCU
read-locked, so while the tool sleeps there anything that needs
synchronize_srcu(&md->io_barrier) on that device is stuck: suspend, table
reload/remove and dm_internal_suspend_fast() from "@stats_create". An
unrelated dm-stats user hangs, and everything that later queues behind
md->suspend_lock hangs with it.

Patch 5 adds a file descriptor based interface instead. The tool gets an
anon inode fd via the "push_backup_get_fd" message and then uses plain
read()/write() of struct push_backup_range to fetch the next range and to
acknowledge it, without going through dm messages on the data path. The
fd pins struct push_backup with a kref so it survives target removal and
simply returns -ENODEV afterwards. The legacy messages are kept for
compatibility.

Patches 1-4 are fixes found while working on this:

  1. init the lock, waitqueue and timer before anything that can fail so
     pb_destroy() is safe from any error path;
  2. clear pb->alive under pb->lock like every other user does;
  3. drop the redundant mod_timer() in push_backup_write(). The timer
     re-arms itself from pb->deadline_jiffies, so updating the deadline
     under the lock is enough. This was also the only place that armed
     the timer outside the window push_backup_stop() waits for, which
     matters once writes may arrive via the fd without pb->ctl_rwsem;
  4. pass absolute expiry to mod_timer(). Both remaining callers passed
     a relative value, so after ~5 min of uptime the timer fired and
     re-armed on every tick while a write was delayed.

https://virtuozzo.atlassian.net/browse/VSTOR-152912

Andrey Zhadchenko (5):
  drivers/md/push_backup: correct initialization
  drivers/md/push_backup: clear pb->alive under pb->lock
  drivers/md/push_backup: do not re-arm deadline timer in
    push_backup_write()
  drivers/md/push_backup: fix mod_timer() usage
  dm/push_backup: add event file descriptor interface

 drivers/md/dm-push-backup.c         | 231 ++++++++++++++++++++++------
 include/uapi/linux/dm-push-backup.h |  36 +++++
 2 files changed, 220 insertions(+), 47 deletions(-)
 create mode 100644 include/uapi/linux/dm-push-backup.h

-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 1/5] drivers/md/push_backup: correct initialization

[Andrey Zhadchenko]

Allocate pool and htable after lock, waitqueue and deadline
timer init so we are sure that pb_destroy() can be called
from anywhere. Otherwise we risk timer_delete_sync_try on
zeroed timer struct (currently luckily fine).

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-push-backup.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index 885ea7b31cf67..9a6252f2b21e2 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -658,24 +658,22 @@ static int pb_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	pb = kzalloc(sizeof(*pb), GFP_KERNEL);
 	if (!pb)
 		goto err;
-
-	pb->pbio_pool = mempool_create_slab_pool(PUSH_BACKUP_POOL_SIZE,
-						 pbio_cache);
-        pb->pending_htable = kcalloc(PB_HASH_TABLE_SIZE,
-				     sizeof(struct hlist_head),
-				     GFP_KERNEL);
-	if (!pb->pbio_pool || !pb->pending_htable)
-		goto err;
-
 	spin_lock_init(&pb->lock);
 	init_rwsem(&pb->ctl_rwsem);
 	bio_list_init(&pb->deferred_bios);
 	INIT_LIST_HEAD(&pb->pending);
 	timer_setup(&pb->deadline_timer, pb_timer_func, 0);
-
 	init_waitqueue_head(&pb->waitq);
 	INIT_WORK(&pb->worker, do_pb_work);
 
+	pb->pbio_pool = mempool_create_slab_pool(PUSH_BACKUP_POOL_SIZE,
+						 pbio_cache);
+	pb->pending_htable = kcalloc(PB_HASH_TABLE_SIZE,
+				     sizeof(struct hlist_head),
+				     GFP_KERNEL);
+	if (!pb->pbio_pool || !pb->pending_htable)
+		goto err;
+
 	ti->private = pb;
 	pb->ti = ti;
 
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 2/5] drivers/md/push_backup: clear pb->alive under pb->lock

[Andrey Zhadchenko]

Although it is harmless in practice, it may potentially trigger
KCSAN. Besides, most other users handle this only under the pb->lock.

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-push-backup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index 9a6252f2b21e2..1ae4ccc208ffa 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -581,7 +581,9 @@ static int pb_message(struct dm_target *ti, unsigned int argc, char **argv,
 		if (!pb->map)
 			return -EBADF;
 
+		spin_lock_irq(&pb->lock);
 		pb->alive = false;
+		spin_unlock_irq(&pb->lock);
 		wake_up_interruptible(&pb->waitq); /* pb->alive = false */
 	}
 
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 3/5] drivers/md/push_backup: do not re-arm deadline timer in push_backup_write()

[Andrey Zhadchenko]

pb_timer_func() checks pb->deadline_jiffies every time it fires,
so we actually don't need to re-arm the timer after setting the
filed.

This is also the only place that arms the timer outside the window
push_backup_stop() waits for with synchronize_rcu(). Today it is
harmless because write and stop are serialized by pb->ctl_rwsem,
but it would re-arm the timer behind timer_delete_sync_try() once
the write path is reachable without the rwsem (next patches).

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-push-backup.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index 1ae4ccc208ffa..8ce84a1a696e5 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -467,8 +467,8 @@ static int push_backup_write(struct push_backup *pb,
 			     unsigned int clu, unsigned int nr)
 {
 	u64 i, key, nr_clus = pb->nr_clus;
-	bool finished, has_more = false;
 	LIST_HEAD(pbio_list);
+	bool finished;
 	struct pb_bio *pbio;
 
 	if (!pb)
@@ -519,8 +519,7 @@ static int push_backup_write(struct push_backup *pb,
 		}
 	}
 
-	has_more = (pb->nr_delayed != 0);
-	if (has_more)
+	if (pb->nr_delayed)
 		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
 	else
 		pb->deadline_jiffies = S64_MAX;
@@ -529,11 +528,8 @@ static int push_backup_write(struct push_backup *pb,
 	if (finished)
 		wake_up_interruptible(&pb->waitq);
 
-	if (!list_empty(&pbio_list)) {
+	if (!list_empty(&pbio_list))
 		resubmit_pbios(pb, &pbio_list);
-		if (has_more)
-			mod_timer(&pb->deadline_timer, pb->timeout_in_jiffies + 1);
-	}
 
 	return 0;
 }
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 4/5] drivers/md/push_backup: fix mod_timer() usage

[Andrey Zhadchenko]

mod_timer() takes an absolute jiffies value, but both callers pass a
relative one. After about 5 min uptime this causes the timer to fire
and re-arm on every tick. This has not exploded because we still have
checked pb->deadline_jiffies each time.

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-push-backup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index 8ce84a1a696e5..b1861204f6e3e 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -244,7 +244,7 @@ static void pb_timer_func(struct timer_list *timer)
 	spin_unlock_irqrestore(&pb->lock, flags);
 
 	if (unlikely(time_before64(now, deadline)))
-		mod_timer(timer, deadline - now + 1);
+		mod_timer(timer, jiffies + (deadline - now) + 1);
 	else
 		queue_work(pb->wq, &pb->worker);
 }
@@ -278,7 +278,7 @@ static int postpone_if_required_for_backup(struct push_backup *pb,
 	spin_unlock_irqrestore(&pb->lock, flags);
 
 	if (queue_timer)
-		mod_timer(&pb->deadline_timer, pb->timeout_in_jiffies + 1);
+		mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
 	rcu_read_unlock();
 
 	if (queue_timer)
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 5/5] dm/push_backup: add event file descriptor interface

[Andrey Zhadchenko]

The backup tool currently waits for the next cluster range to back up
by sleeping inside the "push_backup_read" target message. target_message()
calls into the target under dm_get_live_table(), i.e. with md->io_barrier
SRCU read-locked, so for the whole time the tool waits for a write to be
intercepted anything doing synchronize_srcu(&md->io_barrier) on this device
is stuck: dm suspend, table reload/remove and in particular
dm_internal_suspend_fast() called by "@stats_create". An unrelated dm-stats
consumer therefore hangs together with everything that later queues behind
md->suspend_lock.

Add a file descriptor based interface that does not involve dm messages
on the data path:

  push_backup_get_fd        - returns an anon inode fd in the message
                              result
  read(fd, &range, 16)      - next struct push_backup_range to back up;
                              0 when the backup is finished,
                              -ESTALE when not running,
                              -ENODEV after the target is destroyed
  write(fd, &range, 16)     - acknowledge the range, same as
                              push_backup_write

This way we will avoid aforementioned problems.

We can't access/uninstall fd across all processes that have it, so add kref
on the push_backup struct and tie it to number of fds. Free only when
everything is gone.

The legacy messages keep working unchanged for compatibility but we will
clean it up later.

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/md/dm-push-backup.c         | 197 ++++++++++++++++++++++++----
 include/uapi/linux/dm-push-backup.h |  36 +++++
 2 files changed, 205 insertions(+), 28 deletions(-)
 create mode 100644 include/uapi/linux/dm-push-backup.h

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index b1861204f6e3e..1ea71cb82b9ed 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -15,6 +15,10 @@
 #include <linux/ctype.h>
 #include <linux/dm-io.h>
 #include <linux/blk-mq.h>
+#include <linux/anon_inodes.h>
+#include <linux/kref.h>
+#include <linux/uaccess.h>
+#include <linux/dm-push-backup.h>
 
 
 #define DM_MSG_PREFIX "push-backup"
@@ -46,6 +50,9 @@ struct push_backup {
 	u64 clu_size;
 	u64 nr_clus;
 
+	struct kref ref;
+	bool dead;
+
 	bool alive;
 	void *map;
 	u64 map_bits;
@@ -69,6 +76,16 @@ struct push_backup {
 	struct rw_semaphore ctl_rwsem;
 };
 
+static void pb_release(struct kref *ref)
+{
+	kfree(container_of(ref, struct push_backup, ref));
+}
+
+static void pb_put(struct push_backup *pb)
+{
+	kref_put(&pb->ref, pb_release);
+}
+
 static u64 pbio_first_required_for_backup_clu(struct push_backup *pb, struct pb_bio *pbio)
 {
 	u64 clu;
@@ -415,35 +432,41 @@ static int push_backup_stop(struct push_backup *pb,
 	return 0;
 }
 
-static int push_backup_read(struct push_backup *pb,
-			    char *result, unsigned int maxlen)
+static bool pb_has_event(struct push_backup *pb)
+{
+	return READ_ONCE(pb->dead) || !READ_ONCE(pb->map) ||
+	       !READ_ONCE(pb->alive) || !READ_ONCE(pb->map_bits) ||
+	       !list_empty_careful(&pb->pending);
+}
+
+/*
+ * Returns:
+ *   1       - next range to back up is stored in @clu/@nr
+ *   0       - backup is finished (nothing left to back up)
+ *   -EAGAIN - nothing pending at the moment
+ *   -ESTALE - backup is not running (not started, stopped or expired)
+ *   -ENODEV - target was destroyed
+ */
+static int pb_next_range(struct push_backup *pb, u64 *clu, u64 *nr)
 {
-	unsigned int left, right, sz = 0;
 	struct pb_bio *pbio;
+	u64 left, right;
 	int ret;
 
-	if (!pb)
-		return -EBADF;
-	if (!pb->map)
-		return -ESTALE;
-again:
-	if (wait_event_interruptible(pb->waitq,
-				     !list_empty_careful(&pb->pending) ||
-				     !pb->alive || !pb->map_bits))
-		return -EINTR;
-
 	spin_lock_irq(&pb->lock);
+	ret = -ENODEV;
+	if (pb->dead)
+		goto unlock;
 	ret = -ESTALE;
-	if (!pb->alive)
+	if (!pb->map || !pb->alive)
 		goto unlock;
 	ret = 0;
 	if (!pb->map_bits)
 		goto unlock;
+	ret = -EAGAIN;
 	pbio = list_first_entry_or_null(&pb->pending, typeof(*pbio), list);
-	if (unlikely(!pbio)) {
-		spin_unlock_irq(&pb->lock);
-		goto again;
-	}
+	if (!pbio)
+		goto unlock;
 
 	ret = -EBADMSG;
 	left = pbio_first_required_for_backup_clu(pb, pbio);
@@ -456,15 +479,47 @@ static int push_backup_read(struct push_backup *pb,
 	else
 		right = pb->nr_clus - 1;
 
-	DMEMIT("%u:%u", left, right - left + 1);
+	*clu = left;
+	*nr = right - left + 1;
 	ret = 1;
 unlock:
 	spin_unlock_irq(&pb->lock);
 	return ret;
 }
 
-static int push_backup_write(struct push_backup *pb,
-			     unsigned int clu, unsigned int nr)
+static int pb_wait_range(struct push_backup *pb, bool nonblock,
+			 u64 *clu, u64 *nr)
+{
+	int ret;
+
+	while (1) {
+		ret = pb_next_range(pb, clu, nr);
+		if (ret != -EAGAIN || nonblock)
+			return ret;
+		if (wait_event_interruptible(pb->waitq, pb_has_event(pb)))
+			return -ERESTARTSYS;
+	}
+}
+
+static int push_backup_read(struct push_backup *pb,
+			    char *result, unsigned int maxlen)
+{
+	unsigned int sz = 0;
+	u64 clu, nr;
+	int ret;
+
+	if (!pb)
+		return -EBADF;
+
+	ret = pb_wait_range(pb, false, &clu, &nr);
+	if (ret == -ERESTARTSYS)
+		ret = -EINTR;
+	if (ret == 1)
+		DMEMIT("%llu:%llu", clu, nr);
+	return ret;
+}
+
+static int push_backup_write(struct push_backup *pb, u64 clu, u64 nr)
 {
 	u64 i, key, nr_clus = pb->nr_clus;
 	LIST_HEAD(pbio_list);
@@ -475,11 +530,13 @@ static int push_backup_write(struct push_backup *pb,
 		return -EBADF;
 	if (clu >= nr_clus || nr > nr_clus - clu)
 		return -E2BIG;
-	if (!pb->map)
-		return -ESTALE;
 
 	spin_lock_irq(&pb->lock);
-	if (!pb->alive) {
+	if (pb->dead) {
+		spin_unlock_irq(&pb->lock);
+		return -ENODEV;
+	}
+	if (!pb->map || !pb->alive) {
 		spin_unlock_irq(&pb->lock);
 		return -ESTALE;
 	}
@@ -534,6 +591,73 @@ static int push_backup_write(struct push_backup *pb,
 	return 0;
 }
 
+static ssize_t pb_fd_read(struct file *file, char __user *buf,
+			  size_t count, loff_t *ppos)
+{
+	struct push_backup *pb = file->private_data;
+	struct push_backup_range range;
+	int ret;
+
+	if (count < sizeof(range))
+		return -EINVAL;
+
+	ret = pb_wait_range(pb, file->f_flags & O_NONBLOCK,
+			    &range.clu, &range.nr);
+	if (ret <= 0)
+		return ret;
+
+	if (copy_to_user(buf, &range, sizeof(range)))
+		return -EFAULT;
+	return sizeof(range);
+}
+
+static ssize_t pb_fd_write(struct file *file, const char __user *buf,
+			   size_t count, loff_t *ppos)
+{
+	struct push_backup *pb = file->private_data;
+	struct push_backup_range range;
+	int ret;
+
+	if (count != sizeof(range))
+		return -EINVAL;
+	if (copy_from_user(&range, buf, sizeof(range)))
+		return -EFAULT;
+
+	ret = push_backup_write(pb, range.clu, range.nr);
+	return ret ? : sizeof(range);
+}
+
+static int pb_fd_release(struct inode *inode, struct file *file)
+{
+	pb_put(file->private_data);
+	return 0;
+}
+
+static const struct file_operations pb_fops = {
+	.owner		= THIS_MODULE,
+	.read		= pb_fd_read,
+	.write		= pb_fd_write,
+	.release	= pb_fd_release,
+};
+
+static int push_backup_get_fd(struct push_backup *pb,
+			      char *result, unsigned int maxlen)
+{
+	unsigned int sz = 0;
+	int fd;
+
+	kref_get(&pb->ref);
+	fd = anon_inode_getfd("[push_backup]", &pb_fops, pb,
+			      O_RDWR | O_CLOEXEC);
+	if (fd < 0) {
+		pb_put(pb);
+		return fd;
+	}
+
+	DMEMIT("%d", fd);
+	return 1;
+}
+
 static int push_backup_statistics(struct push_backup *pb, char *result,
 				  unsigned int maxlen)
 {
@@ -612,6 +736,10 @@ static int pb_message(struct dm_target *ti, unsigned int argc, char **argv,
 		ret = push_backup_write(pb, val, val2);
 	} else if (!strcmp(argv[0], "push_backup_statistics")) {
 		ret = push_backup_statistics(pb, result, maxlen);
+	} else if (!strcmp(argv[0], "push_backup_get_fd")) {
+		if (argc != 1)
+			goto unlock;
+		ret = push_backup_get_fd(pb, result, maxlen);
 	} else {
 		ret = -ENOTSUPP;
 	}
@@ -626,18 +754,30 @@ static int pb_message(struct dm_target *ti, unsigned int argc, char **argv,
 }
 static void pb_destroy(struct push_backup *pb)
 {
+	void *map, *pending_map;
+
 	WARN_ON_ONCE(pb->nr_delayed);
 
+	spin_lock_irq(&pb->lock);
+	pb->dead = true;
+	map = pb->map; /* Is's not zero if stop was not called */
+	pb->map = NULL;
+	pending_map = pb->pending_map;
+	pb->pending_map = NULL;
+	spin_unlock_irq(&pb->lock);
+
+	wake_up_interruptible(&pb->waitq); /* pb->dead = true */
+
 	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
 	if (pb->wq)
 		destroy_workqueue(pb->wq);
-	kvfree(pb->map); /* Is's not zero if stop was not called */
-	kvfree(pb->pending_map);
+	kvfree(map);
+	kvfree(pending_map);
 	kvfree(pb->pending_htable);
 	mempool_destroy(pb->pbio_pool);
 	if (pb->origin_dev)
 		dm_put_device(pb->ti, pb->origin_dev);
-	kfree(pb);
+	pb_put(pb);
 }
 
 /*
@@ -656,6 +796,7 @@ static int pb_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	pb = kzalloc(sizeof(*pb), GFP_KERNEL);
 	if (!pb)
 		goto err;
+	kref_init(&pb->ref);
 	spin_lock_init(&pb->lock);
 	init_rwsem(&pb->ctl_rwsem);
 	bio_list_init(&pb->deferred_bios);
@@ -764,7 +905,7 @@ static loff_t pb_llseek_hole(struct dm_target *ti, loff_t offset,
 
 static struct target_type pb_target = {
 	.name = "push_backup",
-	.version = {1, 0, 0},
+	.version = {1, 1, 0},
 	.features = DM_TARGET_SINGLETON,
 	.module = THIS_MODULE,
 	.ctr = pb_ctr,
diff --git a/include/uapi/linux/dm-push-backup.h b/include/uapi/linux/dm-push-backup.h
new file mode 100644
index 0000000000000..3a56f851a2d56
--- /dev/null
+++ b/include/uapi/linux/dm-push-backup.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/*
+ *  Copyright (c) 2026 Virtuozzo International GmbH. All rights reserved.
+ */
+
+/*
+ * Userspace interface of the dm-push-backup target's event file descriptor.
+ *
+ * The fd is obtained with the "push_backup_get_fd" target message; the fd
+ * number is returned in the message result buffer.
+ *
+ * read(fd, &range, sizeof(range)):
+ *   sizeof(range) - @range holds the next cluster range that must be backed
+ *                   up before the writes postponed on it may proceed.
+ *                   Reading does not consume anything: the same (or a wider)
+ *                   range is returned until it is acknowledged via write().
+ *   0             - backup has finished, nothing is left to back up.
+ *   -EAGAIN       - O_NONBLOCK is set and no writes are pending.
+ *   -ESTALE       - backup is not running (not started, stopped or expired).
+ *   -ENODEV       - the target has been destroyed.
+ *
+ * write(fd, &range, sizeof(range)):
+ *   acknowledge that @range has been backed up; equivalent to the
+ *   "push_backup_write clu:nr" message.
+ */
+#ifndef _UAPI_LINUX_DM_PUSH_BACKUP_H
+#define _UAPI_LINUX_DM_PUSH_BACKUP_H
+
+#include <linux/types.h>
+
+struct push_backup_range {
+	__u64 clu;	/* first cluster */
+	__u64 nr;	/* number of clusters */
+};
+
+#endif /* _UAPI_LINUX_DM_PUSH_BACKUP_H */
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/5] drivers/md/push_backup: correct initialization

[no-reply]

> Allocate pool and htable after lock, waitqueue and deadline
> timer init so we are sure that pb_destroy() can be called
> from anywhere. Otherwise we risk timer_delete_sync_try on
> zeroed timer struct (currently luckily fine).
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-152912
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102233849.613155-1-andrey.zhadchenko@virtuozzo.com?part=1


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 2/5] drivers/md/push_backup: clear pb->alive under pb->lock

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-push-backup: pb->alive KCSAN race only partly fixed, plain accesses remain
--

commit 9d9907f2e7f049fda97985539a4920b9e2e49764
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/push_backup: clear pb->alive under pb->lock

This patch makes pb_message() take pb->lock around the store that clears
pb->alive when handling push_backup_stop. The aim is to avoid a possible
KCSAN report and to match other users of pb->alive.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> index 9a6252f2b21e..1ae4ccc208ff 100644
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c
> @@ -581,7 +581,9 @@ static int pb_message(struct dm_target *ti, unsigned int argc, char **argv,
>  		if (!pb->map)
>  			return -EBADF;
>  
> +		spin_lock_irq(&pb->lock);
>  		pb->alive = false;
> +		spin_unlock_irq(&pb->lock);
>  		wake_up_interruptible(&pb->waitq); /* pb->alive = false */
>  	}

[Severity: Low]
Does this fully avoid the KCSAN report described in the commit message
("it may potentially trigger KCSAN")?

This store in pb_message() runs before ctl_rwsem is taken, so it holds
pb->lock but not ctl_rwsem. A concurrent push_backup_start() holds
ctl_rwsem but not pb->lock, so the two do not exclude each other:

push_backup_start()
    if (pb->alive)
        return -EEXIST;

That read is a plain read with no pb->lock and no READ_ONCE(), so it can
still race with the store above.

The wait_event condition used by push_backup_read() is also evaluated
without pb->lock. It reads the flag through pb_has_event():

    !READ_ONCE(pb->alive) || !READ_ONCE(pb->map_bits) || ...

Here the reader is marked, but none of the writers use WRITE_ONCE(). The
stores in pb_message() (this patch), push_backup_stop(), and the
pb->alive = true store in the push_backup_start() setup path are all
still plain stores.

KCSAN can report a plain store racing with a lockless read, so taking
pb->lock around only this one store may not remove the report.

Would it make sense to use WRITE_ONCE() on the stores to pb->alive and
READ_ONCE() on the read in push_backup_start() (or take pb->lock for that
read)? The commit message could also be adjusted if the intent is only to
make the locking consistent.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102233849.613155-1-andrey.zhadchenko@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 4/5] drivers/md/push_backup: fix mod_timer() usage

[no-reply]

> mod_timer() takes an absolute jiffies value, but both callers pass a
> relative one. After about 5 min uptime this causes the timer to fire
> and re-arm on every tick. This has not exploded because we still have
> checked pb->deadline_jiffies each time.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-152912
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102233849.613155-1-andrey.zhadchenko@virtuozzo.com?part=4


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 3/5] drivers/md/push_backup: do not re-arm deadline timer in push_backup_write()

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-push-backup: stale-deadline re-arm in pb_timer_func() can leave timer far in future
--

commit 4c99ff329d7b8e6b90df5269d7233e727e68dc3a
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/push_backup: do not re-arm deadline timer in push_backup_write()

This patch removes the mod_timer() call from push_backup_write(). The
commit message says pb_timer_func() re-checks pb->deadline_jiffies each
time it fires, so re-arming after updating the field is unnecessary. It
also says this prepares for making the write path reachable without
pb->ctl_rwsem.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> index 1ae4ccc208ff..8ce84a1a696e 100644
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c

[ ... ]

> @@ -519,8 +519,7 @@ static int push_backup_write(struct push_backup *pb,
>  		}
>  	}
>  
> -	has_more = (pb->nr_delayed != 0);
> -	if (has_more)
> +	if (pb->nr_delayed)
>  		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
>  	else
>  		pb->deadline_jiffies = S64_MAX;

[ ... ]

> -	if (!list_empty(&pbio_list)) {
> +	if (!list_empty(&pbio_list))
>  		resubmit_pbios(pb, &pbio_list);
> -		if (has_more)
> -			mod_timer(&pb->deadline_timer, pb->timeout_in_jiffies + 1);
> -	}

[Severity: Medium]
Can the deadline timer be left armed far in the future after this change?

The commit message relies on pb_timer_func() re-checking the deadline, but
that callback samples the deadline under pb->lock and then calls mod_timer()
after dropping the lock:

drivers/md/dm-push-backup.c:pb_timer_func() {
    ...
	spin_lock_irqsave(&pb->lock, flags);
	deadline = pb->deadline_jiffies;
	spin_unlock_irqrestore(&pb->lock, flags);

	if (unlikely(time_before64(now, deadline)))
		mod_timer(timer, deadline - now + 1);
    ...
}

Consider this sequence:

1. push_backup_write() completes the last delayed pbio and sets
   pb->deadline_jiffies = S64_MAX, while the timer is still armed for the
   old expiry.

2. The timer fires. pb_timer_func() reads deadline = S64_MAX under
   pb->lock and drops the lock.

3. On another CPU, postpone_if_required_for_backup() takes pb->lock, moves
   pb->nr_delayed from 0 to 1, and sets a fresh deadline_jiffies. It then
   calls mod_timer(&pb->deadline_timer, ...) after dropping the lock, so
   the timer is armed for the short timeout.

4. pb_timer_func() then runs its stale mod_timer(timer, deadline - now + 1)
   with deadline = S64_MAX. This overwrites the short expiry with a
   far-future one.

After step 4, the only arming site left is the pb->nr_delayed == 1
transition in postpone_if_required_for_backup(). push_backup_write() no
longer re-arms the timer, so nothing corrects the expiry.

pb->nr_delayed stays above zero with the timer far in the future. Delayed
requests would then not be released by the backup timeout if userspace
stops servicing the backup.

The window is narrow. It spans the gap between spin_unlock_irqrestore() and
mod_timer() in pb_timer_func(). Before this patch, a later
push_backup_write() with pending pbios could re-arm the timer and recover.

Could the re-arm in pb_timer_func() be done in a way that cannot overwrite a
newer expiry? One option is to re-read or re-arm the deadline under
pb->lock. Another is to have the arming sites in
postpone_if_required_for_backup() and pb_timer_func() order their updates
consistently.

Later patches in this series do not appear to close this window. The patch
described as fixing mod_timer() usage only changes the expiry arithmetic.
The event file descriptor patch does not change this path.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102233849.613155-1-andrey.zhadchenko@virtuozzo.com?part=3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 5/5] dm/push_backup: add event file descriptor interface

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dm-push-backup: fd write races pb_destroy() and uses freed pbio_pool in resubmit_pbios()
- [Low] dm-push-backup: push_backup_get_fd() installs fd before result is delivered
- [Low] dm-push-backup: event fd has no .poll handler, so it cannot be polled
--

commit 013b902b0f480dc76bc700b18a002953deb80967
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

dm/push_backup: add event file descriptor interface

This patch adds an anon-inode file descriptor interface to dm-push-backup.
The fd is obtained with a new push_backup_get_fd target message. Userspace
then uses read() to get the next cluster range to back up and write() to
acknowledge it. A kref on struct push_backup is tied to the number of open
fds.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c

[ ... ]

> @@ -534,6 +591,73 @@ static int push_backup_write(struct push_backup *pb,
>  	return 0;
>  }
>  
> +static ssize_t pb_fd_write(struct file *file, const char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	struct push_backup *pb = file->private_data;
> +	struct push_backup_range range;
> +	int ret;
> +
> +	if (count != sizeof(range))
> +		return -EINVAL;
> +	if (copy_from_user(&range, buf, sizeof(range)))
> +		return -EFAULT;
> +
> +	ret = push_backup_write(pb, range.clu, range.nr);
> +	return ret ? : sizeof(range);
> +}

[Severity: High]
Can this race with pb_destroy() and use freed memory?

Before this patch, push_backup_write() was reachable only through
pb_message(), which runs under dm_get_live_table() and ctl_rwsem, so it
could not overlap the target destructor. Now pb_fd_write() calls it with
neither.

The dead check in push_backup_write() is made under pb->lock, but
resubmit_pbios() runs after the lock is dropped:

push_backup_write()
    spin_unlock_irq(&pb->lock);
    ...
    if (!list_empty(&pbio_list))
        resubmit_pbios(pb, &pbio_list);

resubmit_pbios()
    list_del_init(&pbio->list);
    dm_requeue_original_rq(pbio->rq);
    mempool_free(pbio, pb->pbio_pool);

If pb_destroy() runs on another CPU after the writer has unlinked the
pbios and dropped the lock, it can proceed as follows:

pb_destroy()
    spin_lock_irq(&pb->lock);
    pb->dead = true;
    ...
    spin_unlock_irq(&pb->lock);
    ...
    destroy_workqueue(pb->wq);
    kvfree(pb->pending_htable);
    mempool_destroy(pb->pbio_pool);
    ...
    pb_put(pb);

The writer would then call mempool_free() on a pbio_pool that
mempool_destroy() has already freed. WARN_ON_ONCE(pb->nr_delayed) in
pb_destroy() would not fire in this case, because the writer has already
decremented nr_delayed.

The new kref keeps only the struct push_backup allocation alive. It does not
keep pbio_pool, pending_htable, wq or origin_dev alive. pb_destroy() does
not wait for fd readers or writers that have already passed the dead check.

The fd can also be inherited or passed to a process other than the one that
opened it. Also, __dm_destroy() suspends the device without draining
postponed requests, so pending pbios can exist when dtr runs.

Is there something that serializes pb_fd_write() against pb_destroy(), for
example draining in-flight fd operations before freeing the sub-resources?

> +static int pb_fd_release(struct inode *inode, struct file *file)
> +{
> +	pb_put(file->private_data);
> +	return 0;
> +}
> +
> +static const struct file_operations pb_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= pb_fd_read,
> +	.write		= pb_fd_write,
> +	.release	= pb_fd_release,
> +};

[Severity: Low]
The interface is described as an event file descriptor, but pb_fops has no
.poll callback.

Can a backup tool multiplex this fd with its other I/O? As written, it can
only block in read() or busy-poll with O_NONBLOCK and get -EAGAIN.

pb_has_event() and pb->waitq already exist and look like a natural basis
for a poll handler. Would it make sense to add .poll before the UAPI is
fixed? Adding it later is harder because read() does not consume anything
and returns 0 to mean "finished".

> +static int push_backup_get_fd(struct push_backup *pb,
> +			      char *result, unsigned int maxlen)
> +{
> +	unsigned int sz = 0;
> +	int fd;
> +
> +	kref_get(&pb->ref);
> +	fd = anon_inode_getfd("[push_backup]", &pb_fops, pb,
> +			      O_RDWR | O_CLOEXEC);
> +	if (fd < 0) {
> +		pb_put(pb);
> +		return fd;
> +	}
> +
> +	DMEMIT("%d", fd);
> +	return 1;
> +}

[Severity: Low]
Can this leak the fd when the result is not delivered?

anon_inode_getfd() does fd_install() irrevocably. DMEMIT() never reports
truncation, so push_backup_get_fd() returns 1 even if the result buffer is
too small and the fd number is truncated or omitted. The same applies if the
dm-ioctl copy-out fails afterwards.

In those cases the caller never learns the fd number, and the open fd
continues to hold a pb kref and a module reference through pb_fops.owner
until the process exits.

Would it be better to check maxlen before creating the fd, or to use
get_unused_fd_flags() and call fd_install() only after the result has been
formatted?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102233849.613155-1-andrey.zhadchenko@virtuozzo.com?part=5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel