[PATCH master 1/3] cdev: return error code from cdev_close

[PATCH master 1/3] cdev: return error code from cdev_close

[Ahmad Fatoum]

cdev_operations::close can fail and thus cdev_close should pass along
the error code to the users instead of discarding it.

Do that, so users like devfs close() can start propagating this error.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs-core.c  | 11 ++++++++---
 include/driver.h |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 376c62be9eab..21e5c2dc969a 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -224,12 +224,17 @@ struct cdev *cdev_open_by_name(const char *name, unsigned long flags)
 	return cdev;
 }
 
-void cdev_close(struct cdev *cdev)
+int cdev_close(struct cdev *cdev)
 {
-	if (cdev->ops->close)
-		cdev->ops->close(cdev);
+	if (cdev->ops->close) {
+		int ret = cdev->ops->close(cdev);
+		if (ret)
+			return ret;
+	}
 
 	cdev->open--;
+
+	return 0;
 }
 
 ssize_t cdev_read(struct cdev *cdev, void *buf, size_t count, loff_t offset, ulong flags)
diff --git a/include/driver.h b/include/driver.h
index 5a3e46e99e14..3401ab4f07f4 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -529,7 +529,7 @@ struct cdev *cdev_create_loop(const char *path, ulong flags, loff_t offset);
 void cdev_remove_loop(struct cdev *cdev);
 int cdev_open(struct cdev *, unsigned long flags);
 int cdev_fdopen(struct cdev *cdev, unsigned long flags);
-void cdev_close(struct cdev *cdev);
+int cdev_close(struct cdev *cdev);
 int cdev_flush(struct cdev *cdev);
 ssize_t cdev_read(struct cdev *cdev, void *buf, size_t count, loff_t offset, ulong flags);
 ssize_t cdev_write(struct cdev *cdev, const void *buf, size_t count, loff_t offset, ulong flags);
-- 
2.39.2

[PATCH master 2/3] fs: devfs: restore cdev reference count maintenance

[Ahmad Fatoum]

barebox v2024.03.0 moves the reference count maintenance into
cdev_open/cdev_close, but missed to switch devfs_open and devfs_close to
actually use cdev_open/cdev_close. The result was that the cdev could be
deleted, e.g. due to partition reparsing, and further use of the file
descriptor would result in a use-after-free.

Fix this by doing what the commit introducing the breakage was purported
to do.

Fixes: d84c3daf1314 ("fs: move cdev open count to cdev_open()/cdev_close()")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/devfs.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/fs/devfs.c b/fs/devfs.c
index c8ddbbdab04d..f5bad5aa9bf2 100644
--- a/fs/devfs.c
+++ b/fs/devfs.c
@@ -102,33 +102,19 @@ static int devfs_open(struct device *_dev, FILE *f, const char *filename)
 	struct inode *inode = f->f_inode;
 	struct devfs_inode *node = container_of(inode, struct devfs_inode, inode);
 	struct cdev *cdev = node->cdev;
-	int ret;
 
 	f->size = cdev->flags & DEVFS_IS_CHARACTER_DEV ?
 			FILE_SIZE_STREAM : cdev->size;
 	f->priv = cdev;
 
-	if (cdev->ops->open) {
-		ret = cdev->ops->open(cdev, f->flags);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return cdev_open(cdev, f->flags);
 }
 
 static int devfs_close(struct device *_dev, FILE *f)
 {
 	struct cdev *cdev = f->priv;
-	int ret;
 
-	if (cdev->ops->close) {
-		ret = cdev->ops->close(cdev);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return cdev_close(cdev);
 }
 
 static int devfs_flush(struct device *_dev, FILE *f)
-- 
2.39.2

[PATCH master 3/3] partition: reparse tables only if CONFIG_PARTITION_MANIPULATION=y

[Ahmad Fatoum]

reparse_partition_table() will delete a device's existing partition
cdevs and allocate new ones according to the new partition table.

Holding a reference to a cdev prevents it from bring removed to avoid
dangling pointers and use-after-frees.

Unfortunately, not all users call cdev_open on the cdev and increment
the usage count, instead using functions like cdev_by_name, which don't
call cdev_open.

The proper solution for this is probably to change all functions that
return a cdev to actually take a reference to the cdev.

This will involve quite a bit of work though, so for now, restore old
behavior by making reparsing of the partition tables dependent on
CONFIG_PARTITION_MANIPULATION=y.

The option isn't fully appropriate as partition manipulation can happen
by writing to the parent block device, but not noticing changes of
partitions is the behavior we had before.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/partitions.c |  2 ++
 include/disks.h     | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/common/partitions.c b/common/partitions.c
index 17c2f1eb281a..0b10377e7327 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -266,6 +266,7 @@ int parse_partition_table(struct block_device *blk)
 	return rc;
 }
 
+#ifdef CONFIG_PARTITION_MANIPULATION
 int reparse_partition_table(struct block_device *blk)
 {
 	struct cdev *cdev = &blk->cdev;
@@ -285,6 +286,7 @@ int reparse_partition_table(struct block_device *blk)
 
 	return parse_partition_table(blk);
 }
+#endif
 
 int partition_parser_register(struct partition_parser *p)
 {
diff --git a/include/disks.h b/include/disks.h
index ccb50d3ce94f..a3673d1e276f 100644
--- a/include/disks.h
+++ b/include/disks.h
@@ -3,6 +3,9 @@
 #ifndef DISKS_H
 #define DISKS_H
 
+#include <linux/types.h>
+#include <linux/errno.h>
+
 struct block_device;
 
 /** Size of one sector in bytes */
@@ -24,6 +27,14 @@ struct partition_entry {
 } __attribute__ ((packed));
 
 extern int parse_partition_table(struct block_device*);
+
+#ifdef CONFIG_PARTITION_MANIPULATION
 int reparse_partition_table(struct block_device *blk);
+#else
+static inline int reparse_partition_table(struct block_device *blk)
+{
+	return -ENOSYS;
+}
+#endif
 
 #endif /* DISKS_H */
-- 
2.39.2

[PATCH] fixup! partition: reparse tables only if CONFIG_PARTITION_MANIPULATION=y

[Ahmad Fatoum]

partitions: add prompt and help text for CONFIG_PARTITION_MANIPULATION

So far, CONFIG_PARTITION_MANIPULATION was only selected by
CONFIG_CMD_PARTED and influenced repartitioning support.

Now that it's used to guard the feature of reparsing partition tables,
it makes sense to give it a help text and a prompt for devlopment usage.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/common/Kconfig b/common/Kconfig
index 97a03217eac9..67cbbf5197da 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -888,7 +888,15 @@ config PARTITION
 	prompt "Enable Partitions"
 
 config PARTITION_MANIPULATION
-	bool
+	bool "Runtime reparsing of partition table" if COMPILE_TEST
+	help
+	  Say y here to have barebox reparse the partition table automatically
+	  when it's rewritten. This is useful when using the parted command
+	  or when writing full disk images that change the existing partitions.
+
+	  Reparsing the partition table will delete existing partitions and thus
+	  may break users that don't do proper reference counting. For this
+	  reason, this option is currently disabled by default.
 
 source "common/partitions/Kconfig"
 
-- 
2.39.2

Re: [PATCH master 1/3] cdev: return error code from cdev_close

[Sascha Hauer]

On Wed, 15 May 2024 10:05:05 +0200, Ahmad Fatoum wrote:
> cdev_operations::close can fail and thus cdev_close should pass along
> the error code to the users instead of discarding it.
> 
> Do that, so users like devfs close() can start propagating this error.
> 
> 

Applied, thanks!

[1/3] cdev: return error code from cdev_close
      https://git.pengutronix.de/cgit/barebox/commit/?id=490dab2db018 (link may not be stable)
[2/3] fs: devfs: restore cdev reference count maintenance
      https://git.pengutronix.de/cgit/barebox/commit/?id=dd6ac1612b4b (link may not be stable)
[3/3] partition: reparse tables only if CONFIG_PARTITION_MANIPULATION=y
      https://git.pengutronix.de/cgit/barebox/commit/?id=dfdfc732ba6f (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>