From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f172.google.com (mail-qt1-f172.google.com [209.85.160.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6A8101D432D for ; Tue, 2 Sep 2025 20:23:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.160.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756844627; cv=none; b=ZB4co76QFiVeb8K7dlNtuTjKcHxCmXtFUgWV5ZT1ZDw24ZJ0+AmSBA2eRLJZZJ92zoEiNiirkxLu+B034kRE3V0Auf77Ukz8SzqsYOAq1u/k54TYf6Ql3+fahK9KVBQxs7piiUsYY8loHjYD9yHaN2hCq5s8wM+I4v+K2ID34C8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756844627; c=relaxed/simple; bh=dldwsN7JwF159qfL19SWMfGJuEIEBMeOJ/lW1xsfG64=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=PbMQvWggeu3q3dETt2VC14/ISssXL5oT6qH7mxHNJPD/JFa3AU4mTev7Arw93GrJtQ4Z+bYgBnf6VmqTlPhtjngNn0iEGsiAZ36DveTJu6NRu46Z68o+eUFdXvIxNgJRceumUZf1nYVFDbpAU4niGOr3/POl/X8tP5aVq6ueeJI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zetier.com; spf=pass smtp.mailfrom=zetier.com; dkim=pass (2048-bit key) header.d=zetier.com header.i=@zetier.com header.b=B9uMA0y7; arc=none smtp.client-ip=209.85.160.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zetier.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=zetier.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=zetier.com header.i=@zetier.com header.b="B9uMA0y7" Received: by mail-qt1-f172.google.com with SMTP id d75a77b69052e-4b326b6c189so36678401cf.2 for ; Tue, 02 Sep 2025 13:23:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=zetier.com; s=gm; t=1756844624; x=1757449424; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=ti1dNaEX5RAgN5lpAttUlDndIhL/boOtgWoq584IIFM=; b=B9uMA0y7ULitKSg4vCq9rcrnBaTKI2SOLqzi18WEIsspcPI7pytUjlP7GMDhClBPy6 Nw1dShQIvyXB8TatAp34klaDzs4kOgoOSdsHi5fiLvVQNucz0uJG7JnVOocQhWrRuxen 7BLAW4Xaa42s4CttJ321eIQEdQvfBSEGJek0W/9ArxlRmLAB8fVeIoH4j3zor5D54hMb DSWNIzWRCZ5+F+1eHMZa9ncKsv4AY7QmxSO/HYT+l84urk/xr2g1osN0PAiFg8K/UYjA c9ifi+k4fEIJHiRQXtEdS6o+aGGoM8xUk3kk2YPoL31SjLfRkivRpzSUyS4dNS4YLXbI PT2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1756844624; x=1757449424; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ti1dNaEX5RAgN5lpAttUlDndIhL/boOtgWoq584IIFM=; b=Ru/RWTpK1I53/3KRkHLrjxMOUPt0INWGLYMe0q3vBBc/JyiN/xHIVJQfTINcN85chs 5Z+80LPkAn09l4JdNpk43Y0Xa7hL5rakaC7ZyBe/r3wtevM49HZOZAl/nloyR3RS/fbk P8l+2J5h4p0miQgpaSRt5A0/iQrTp+8MHLeQi6teWx23nDfh/3Ta6HLz5Zzdi0jIOIP9 n0rPlBz936XNYKk4IVDu4skUZZzJT9dstBCeyBxeAxq87be70j994FRZ0EkcJKbnXmM7 hKM40dji3sXk7di9EbQNP/bf84Bj8+hien9HUQFyZSJfNalwZy8D+C+tTfiwd9EsJ8MK +IQw== X-Forwarded-Encrypted: i=1; AJvYcCUYwOpHtgUTCv+PEV+/F+vcRkDJH4/4MZ8i8qA89QmieA/Y1uxKV2NRMfX4A9sioO7Dw6sH1Vj6tDmJQss=@vger.kernel.org X-Gm-Message-State: AOJu0Yzl9gWKjF5Mkv783VxrZbsW0cbCd4Ksihox8evLw6zOeaOTYpOU 15hs+70uiHGCGdfWg+ckpIGy2MkkLSODQEkst6xhyNkKb9N/i1Q7b7VqxRY6Jo2t0ys= X-Gm-Gg: ASbGncs7HzswHg6NeLpDuOWq8sYo3tUDxjYA/q0hMLEAAFquy1DI837MyhbmS8NLWo5 GW24Uu9t2J9gv83erznieLnDi40K1g/hhnby5jGTbzPVvtZ+aea11DHBJ6cFGaBOCn630eDJS7Z dG/9I3rQYF31cRHWqUoFJblb1OVr7NbKgaWgnwScTEoSN8yEOYTU5zzR3MYPODuH7EY8wD5HnfT nxLA0vyfEsTYLubjReuHjONW5Jai73TBGCTAypwDsWrkqerxszZpOpG+lDzf3kd9k/CbaaDJ1SM g9/owrLDAdPsSZK/INMfN8MZ6TSB97yabh8vDVX2zrRPgDXDfeAu4kUbYYAwraAGA/BbanXKdaY 1ONNSsc2lEbacrmCu0bxBMc7hkaAdhYWnOCiLwLIft6/sr7+e X-Google-Smtp-Source: AGHT+IHUsaSnoZSo/var0yLCu9AO/fvoiQsemCFj3Gw1dysdd2u/8NiVBo76Ya0LxI9XAMdMj2Fr8g== X-Received: by 2002:a05:622a:4d97:b0:4b3:c25:7280 with SMTP id d75a77b69052e-4b31dd5679cmr186031381cf.71.1756844624181; Tue, 02 Sep 2025 13:23:44 -0700 (PDT) Received: from ethanf.zetier.com ([65.222.209.234]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-4b48f7a1738sm280951cf.46.2025.09.02.13.23.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Sep 2025 13:23:43 -0700 (PDT) From: Ethan Ferguson To: yuezhang.mo@sony.com Cc: ethan.ferguson@zetier.com, linkinjeon@kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, sj1557.seo@samsung.com Subject: RE: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL Date: Tue, 2 Sep 2025 16:23:06 -0400 Message-Id: <20250902202306.86404-1-ethan.ferguson@zetier.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit On 9/2/25 00:55, Yuezhang.Mo@sony.com wrote: > Hi, > > I have 3 more comments. > >> Add support for reading / writing to the exfat volume label from the >> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls >> >> Signed-off-by: Ethan Ferguson >> --- >> fs/exfat/exfat_fs.h | 5 + >> fs/exfat/exfat_raw.h | 6 ++ >> fs/exfat/file.c | 88 +++++++++++++++++ >> fs/exfat/super.c | 224 +++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 323 insertions(+) >> >> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h >> index f8ead4d47ef0..ed4b5ecb952b 100644 >> --- a/fs/exfat/exfat_fs.h >> +++ b/fs/exfat/exfat_fs.h >> @@ -267,6 +267,7 @@ struct exfat_sb_info { >> struct buffer_head **vol_amap; /* allocation bitmap */ >> >> unsigned short *vol_utbl; /* upcase table */ >> + unsigned short *volume_label; /* volume name */ >> >> unsigned int clu_srch_ptr; /* cluster search pointer */ >> unsigned int used_clusters; /* number of used clusters */ >> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct >> inode *inode) > [snip] >> diff --git a/fs/exfat/file.c b/fs/exfat/file.c >> index 538d2b6ac2ec..970e3ee57c43 100644 >> --- a/fs/exfat/file.c >> +++ b/fs/exfat/file.c >> @@ -12,6 +12,7 @@ >> #include >> #include >> #include >> +#include "../nls/nls_ucs2_utils.h" >> >> #include "exfat_raw.h" >> #include "exfat_fs.h" >> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block >> *sb, unsigned long arg) >> return exfat_force_shutdown(sb, flags); >> } >> >> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned >> long arg) >> +{ >> + int ret; >> + char utf8[FSLABEL_MAX] = {0}; >> + struct exfat_uni_name *uniname; >> + struct exfat_sb_info *sbi = EXFAT_SB(sb); >> + >> + uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL); >> + if (!uniname) >> + return -ENOMEM; >> + >> + ret = exfat_read_volume_label(sb); >> + if (ret < 0) >> + goto cleanup; >> + >> + memcpy(uniname->name, sbi->volume_label, >> + EXFAT_VOLUME_LABEL_LEN * sizeof(short)); >> + uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000; >> + uniname->name_len = UniStrnlen(uniname->name, >> EXFAT_VOLUME_LABEL_LEN); >> + >> + ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX); >> + if (ret < 0) >> + goto cleanup; >> + >> + if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) { >> + ret = -EFAULT; >> + goto cleanup; >> + } >> + >> + ret = 0; >> + >> +cleanup: >> + kfree(uniname); >> + return ret; >> +} >> + >> +static int exfat_ioctl_set_volume_label(struct super_block *sb, >> + unsigned long arg, >> + struct inode *root_inode) >> +{ >> + int ret, lossy; >> + char utf8[FSLABEL_MAX]; >> + struct exfat_uni_name *uniname; >> + >> + if (!capable(CAP_SYS_ADMIN)) >> + return -EPERM; >> + >> + uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL); >> + if (!uniname) >> + return -ENOMEM; >> + >> + if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) { >> + ret = -EFAULT; >> + goto cleanup; >> + } >> + >> + if (utf8[0]) { >> + ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8, >> FSLABEL_MAX), >> + uniname, &lossy); >> + if (ret < 0) >> + goto cleanup; >> + else if (lossy & NLS_NAME_LOSSY) { >> + ret = -EINVAL; >> + goto cleanup; >> + } >> + } else { >> + uniname->name[0] = 0x0000; >> + uniname->name_len = 0; >> + } >> + >> + if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) { >> + exfat_info(sb, "Volume label length too long, truncating"); >> + uniname->name_len = EXFAT_VOLUME_LABEL_LEN; >> + } >> + >> + ret = exfat_write_volume_label(sb, uniname, root_inode); >> + >> +cleanup: >> + kfree(uniname); >> + return ret; >> +} >> + >> long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) >> { >> struct inode *inode = file_inode(filp); >> u32 __user *user_attr = (u32 __user *)arg; >> + struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode; >> >> switch (cmd) { >> case FAT_IOCTL_GET_ATTRIBUTES: >> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd, >> unsigned long arg) >> return exfat_ioctl_shutdown(inode->i_sb, arg); >> case FITRIM: >> return exfat_ioctl_fitrim(inode, arg); >> + case FS_IOC_GETFSLABEL: >> + return exfat_ioctl_get_volume_label(inode->i_sb, arg); >> + case FS_IOC_SETFSLABEL: >> + return exfat_ioctl_set_volume_label(inode->i_sb, arg, >> root_inode); >> default: >> return -ENOTTY; >> } >> diff --git a/fs/exfat/super.c b/fs/exfat/super.c >> index 8926e63f5bb7..7931cdb4a1d1 100644 >> --- a/fs/exfat/super.c >> +++ b/fs/exfat/super.c >> @@ -18,6 +18,7 @@ >> #include >> #include >> #include >> +#include "../nls/nls_ucs2_utils.h" >> >> #include "exfat_raw.h" >> #include "exfat_fs.h" >> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct >> super_block *sb) >> return 0; >> } >> >> +static int exfat_get_volume_label_ptrs(struct super_block *sb, >> + struct buffer_head **out_bh, >> + struct exfat_dentry **out_dentry, >> + struct inode *root_inode) >> +{ >> + int i, ret; >> + unsigned int type, old_clu; >> + struct exfat_sb_info *sbi = EXFAT_SB(sb); >> + struct exfat_chain clu; >> + struct exfat_dentry *ep, *deleted_ep = NULL; >> + struct buffer_head *bh, *deleted_bh; >> + >> + clu.dir = sbi->root_dir; >> + clu.flags = ALLOC_FAT_CHAIN; >> + >> + while (clu.dir != EXFAT_EOF_CLUSTER) { >> + for (i = 0; i < sbi->dentries_per_clu; i++) { >> + ep = exfat_get_dentry(sb, &clu, i, &bh); >> + >> + if (!ep) { >> + ret = -EIO; >> + goto end; >> + } >> + >> + type = exfat_get_entry_type(ep); >> + if (type == TYPE_DELETED && !deleted_ep && root_inode) >> { >> + deleted_ep = ep; >> + deleted_bh = bh; >> + continue; >> + } >> + >> + if (type == TYPE_UNUSED) { >> + if (!root_inode) { >> + brelse(bh); >> + ret = -ENOENT; >> + goto end; >> + } >> + >> + if (deleted_ep) { >> + brelse(bh); >> + goto end; >> + } >> + >> + if (i < sbi->dentries_per_clu - 1) { >> + deleted_ep = ep; >> + deleted_bh = bh; >> + >> + ep = exfat_get_dentry(sb, &clu, >> + i + 1, &bh); >> + memset(ep, 0, >> + sizeof(struct exfat_dentry)); >> + ep->type = EXFAT_UNUSED; >> + exfat_update_bh(bh, true); >> + brelse(bh); >> + >> + goto end; >> + } >> + >> + // Last dentry in cluster > > Please use /* */ to comment. > >> + clu.size = 0; >> + old_clu = clu.dir; >> + ret = exfat_alloc_cluster(root_inode, 1, >> + &clu, true); >> + if (ret < 0) { >> + brelse(bh); >> + goto end; >> + } > > In exFAT, directory size is limited to 256MB. Please add a check to return -ENOSPC > instead of allocating a new cluster if the root directory size had reached this limit. > Noted. I am switching over to using exfat_find_empty_entry, which checks for this. >> + >> + ret = exfat_ent_set(sb, old_clu, clu.dir); >> + if (ret < 0) { >> + exfat_free_cluster(root_inode, &clu); >> + brelse(bh); >> + goto end; >> + } >> + >> + ret = exfat_zeroed_cluster(root_inode, clu.dir); >> + if (ret < 0) { >> + exfat_free_cluster(root_inode, &clu); >> + brelse(bh); >> + goto end; >> + } > > After allocating a new cluster for the root directory, its size needs to be updated. > Where would I update the size? I don't think the root directory has a Stream Extension dentry, would I increment the exfat_inode_info.dir.size field? >> + >> + deleted_ep = ep; >> + deleted_bh = bh; >> + goto end; >> + } >> + >> + if (type == TYPE_VOLUME) { >> + *out_bh = bh; >> + *out_dentry = ep; >> + >> + if (deleted_ep) >> + brelse(deleted_bh); >> + >> + return 0; >> + } >> + >> + brelse(bh); >> + } >> + >> + if (exfat_get_next_cluster(sb, &(clu.dir))) { >> + ret = -EIO; >> + goto end; >> + } >> + } >> + >> + ret = -EIO; >> + >> +end: >> + if (deleted_ep) { >> + *out_bh = deleted_bh; >> + *out_dentry = deleted_ep; >> + memset((*out_dentry), 0, sizeof(struct exfat_dentry)); >> + (*out_dentry)->type = EXFAT_VOLUME; >> + return 0; >> + } >> + >> + *out_bh = NULL; >> + *out_dentry = NULL; >> + return ret; >> +} >> + >> +static int exfat_alloc_volume_label(struct super_block *sb) >> +{ >> + struct exfat_sb_info *sbi = EXFAT_SB(sb); >> + >> + if (sbi->volume_label) >> + return 0; >> + >> + >> + mutex_lock(&sbi->s_lock); >> + sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN, >> + sizeof(short), GFP_KERNEL); >> + mutex_unlock(&sbi->s_lock); >> + >> + if (!sbi->volume_label) >> + return -ENOMEM; >> + >> + return 0; >> +} >> + >> +int exfat_read_volume_label(struct super_block *sb) >> +{ >> + int ret, i; >> + struct exfat_sb_info *sbi = EXFAT_SB(sb); >> + struct buffer_head *bh = NULL; >> + struct exfat_dentry *ep = NULL; >> + >> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL); >> + // ENOENT signifies that a volume label dentry doesn't exist >> + // We will treat this as an empty volume label and not fail. >> + if (ret < 0 && ret != -ENOENT) >> + goto cleanup; >> + >> + ret = exfat_alloc_volume_label(sb); >> + if (ret < 0) >> + goto cleanup; >> + >> + mutex_lock(&sbi->s_lock); >> + if (!ep) >> + memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN); >> + else >> + for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++) >> + sbi->volume_label[i] = le16_to_cpu(ep- >>> dentry.volume_label.volume_label[i]); >> + mutex_unlock(&sbi->s_lock); >> + >> + ret = 0; >> + >> +cleanup: >> + if (bh) >> + brelse(bh); >> + >> + return ret; >> +} >> + >> +int exfat_write_volume_label(struct super_block *sb, >> + struct exfat_uni_name *uniname, >> + struct inode *root_inode) >> +{ >> + int ret, i; >> + struct exfat_sb_info *sbi = EXFAT_SB(sb); >> + struct buffer_head *bh = NULL; >> + struct exfat_dentry *ep = NULL; >> + >> + if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) { >> + ret = -EINVAL; >> + goto cleanup; >> + } >> + >> + ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode); >> + if (ret < 0) >> + goto cleanup; >> + >> + ret = exfat_alloc_volume_label(sb); >> + if (ret < 0) >> + goto cleanup; >> + >> + memcpy(sbi->volume_label, uniname->name, >> + uniname->name_len * sizeof(short)); >> + >> + mutex_lock(&sbi->s_lock); >> + for (i = 0; i < uniname->name_len; i++) >> + ep->dentry.volume_label.volume_label[i] = >> + cpu_to_le16(sbi->volume_label[i]); >> + // Fill the rest of the str with 0x0000 >> + for (; i < EXFAT_VOLUME_LABEL_LEN; i++) >> + ep->dentry.volume_label.volume_label[i] = 0x0000; >> + >> + ep->dentry.volume_label.char_count = uniname->name_len; >> + mutex_unlock(&sbi->s_lock); >> + >> + ret = 0; >> + >> +cleanup: >> + if (bh) { >> + exfat_update_bh(bh, true); >> + brelse(bh); >> + } >> + >> + return ret; >> +} >> + >> /* mount the file system volume */ >> static int __exfat_fill_super(struct super_block *sb, >> struct exfat_chain *root_clu) >> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p) >> >> unload_nls(sbi->nls_io); >> exfat_free_upcase_table(sbi); >> + kfree(sbi->volume_label); >> exfat_free_sbi(sbi); >> } >> >> -- >> 2.34.1