On Mon, Nov 29, 2021 at 01:28:09PM -0700, Andreas Dilger wrote:

On Nov 12, 2021, at 1:20 AM, Lukas Czerner [off-list ref] wrote:

quoted

Implement support for FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls for
online reading and setting of file system label.

ext4_ioctl_getlabel() is simple, just get the label from the primary
superblock bh. This might not be the first sb on the file system if
'sb=' mount option is used.

In ext4_ioctl_setlabel() we update what ext4 currently views as a
primary superblock and then proceed to update backup superblocks. There
are two caveats:
- the primary superblock might not be the first superblock and so it
  might not be the one used by userspace tools if read directly
  off the disk.
- because the primary superblock might not be the first superblock we
  potentialy have to update it as part of backup superblock update.
  However the first sb location is a bit more complicated than the rest
  so we have to account for that.

Tested with generic/492 with various configurations. I also checked the
behavior with 'sb=' mount options, including very large file systems
with and without sparse_super/sparse_super2.

Signed-off-by: Lukas Czerner <redacted>
---

One minor issue/question inline.

quoted

+static int ext4_ioctl_setlabel(struct file *filp, const char __user *user_label)
+{
+	size_t len;
+	handle_t *handle;
+	ext4_group_t ngroups;
+	ext4_fsblk_t sb_block;
+	struct buffer_head *bh;
+	int ret = 0, ret2, grp;
+	unsigned long offset = 0;
+	char new_label[EXT4_LABEL_MAX + 1];
+	struct super_block *sb = file_inode(filp)->i_sb;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+
+	/* Sanity check, this should never happen */
+	BUILD_BUG_ON(sizeof(es->s_volume_name) < EXT4_LABEL_MAX);
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	/*
+	 * Copy the maximum length allowed for ext4 label with one more to
+	 * find the required terminating null byte in order to test the
+	 * label length. The on disk label doesn't need to be null terminated.
+	 */
+	if (copy_from_user(new_label, user_label, EXT4_LABEL_MAX + 1))
+		return -EFAULT;
+
+	len = strnlen(new_label, EXT4_LABEL_MAX + 1);
+	if (len > EXT4_LABEL_MAX)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;
+
+	handle = ext4_journal_start_sb(sb, EXT4_HT_MISC, EXT4_MAX_TRANS_DATA);
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		goto err_out;
+	}
+	/* Update the primary superblock first */
+	ret = ext4_journal_get_write_access(handle, sb,
+					    sbi->s_sbh,
+					    EXT4_JTR_NONE);
+	if (ret)
+		goto err_journal;
+
+	lock_buffer(sbi->s_sbh);
+	memset(es->s_volume_name, 0, sizeof(es->s_volume_name));
+	memcpy(es->s_volume_name, new_label, len);

(minor) this introduces a very small window where s_volume_name is unset.
Since "new_label" is already a temporary buffer of the correct size, it
would be better IMHO to zero it out, copy the new label from userspace
into it, and then copy EXT4_LABEL_MAX bytes of new_label to s_volume_name.

It still isn't perfect, but reduces the window significantly.

Very good point, I'll fix that in the next version.

Thanks!
-Lukas

quoted

+	/* Update backup superblocks */
+	ngroups = ext4_get_groups_count(sb);
+	for (grp = 0; grp < ngroups; grp++) {

		:
		:

quoted

+		ext4_debug("update backup superblock %llu\n", sb_block);
+		BUFFER_TRACE(bh, "get_write_access");
+		ret = ext4_journal_get_write_access(handle, sb,
+						    bh,
+						    EXT4_JTR_NONE);
+		if (ret) {
+			brelse(bh);
+			break;
+		}
+
+		es = (struct ext4_super_block *) (bh->b_data + offset);
+		lock_buffer(bh);
+		if (ext4_has_metadata_csum(sb) &&
+		    es->s_checksum != ext4_superblock_csum(sb, es)) {
+			ext4_msg(sb, KERN_ERR, "Invalid checksum for backup "
+				 "superblock %llu\n", sb_block);
+			unlock_buffer(bh);
+			brelse(bh);
+			ret = -EFSBADCRC;
+			break;
+		}
+		memset(es->s_volume_name, 0, sizeof(es->s_volume_name));
+		memcpy(es->s_volume_name, new_label, len);

Same here.

The rest looks fine.

Cheers, Andreas