From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 62D971FF0A7 for ; Tue, 18 Aug 2026 14:33:22 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A6401214F5; Tue, 18 Aug 2026 14:33:19 +0200 (CEST) Message-ID: <3734d9e1-f603-4afc-a3dc-38fa5240562d@proxmox.com> Date: Tue, 18 Aug 2026 14:33:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-storage 2/2] fix #6050: cifs: normalize share and subdir paths for mount and status To: Jakob Klocker , pve-devel@lists.proxmox.com References: <20260807093657.23649-1-d.riley@proxmox.com> <20260807093657.23649-3-d.riley@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787056371790 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.691 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 5KNWIIXVQ3IKNVSVZG63JBUJIZPQJCOC X-Message-ID-Hash: 5KNWIIXVQ3IKNVSVZG63JBUJIZPQJCOC X-MailFrom: d.riley@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 8/18/26 12:23 PM, Jakob Klocker wrote: > Comments inline. I'd also suggest mentioning why you didn't reuse the > `canonpath` function you used in the previous patch here. Thanks for taking a look. You are right I should have given some context as to why I did not use canonpath for this patch. The main reason is that subdir for example could be written as: / and canonpath [0] is totally fine with this as this is a valid path and therefore I opted to use regexes for this. [0] https://perldoc.perl.org/File::Spec#canonpath > > On Fri Aug 7, 2026 at 11:36 AM CEST, David Riley wrote: >> Manual edits to the storage configuration bypass strict API >> validation. If a user adds extra or trailing slashes to the share or >> subdirectory paths, it causes two distinct issues for CIFS storages: >> >> * The Linux VFS CIFS module expects strict UNC [0] formatting and >> will reject the mount attempt with a 'Malformed UNC in devname' >> error. >> * The internal path used by the status daemon fails to match the >> normalized path reported in /proc/mounts, causing working storages >> to continuously report as 'inactive'. >> >> Sanitize the share and subdirectory strings by stripping leading and >> trailing slashes before executing mounts, verifying mount status, and >> testing the SMB connection. This ensures working storages mount >> successfully and are correctly recognized as active. >> >> [0] https://docs.kernel.org/admin-guide/cifs/usage.html#cifs-vfs-mount-options >> >> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6050 >> Signed-off-by: David Riley >> --- >> src/PVE/Storage/CIFSPlugin.pm | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm >> index 54f0f4e..78ed88c 100644 >> --- a/src/PVE/Storage/CIFSPlugin.pm >> +++ b/src/PVE/Storage/CIFSPlugin.pm >> @@ -2,15 +2,20 @@ package PVE::Storage::CIFSPlugin; >> >> use strict; >> use warnings; >> + >> +use File::Path; >> use Net::IP; >> + >> use PVE::Tools qw(run_command); >> use PVE::ProcFSTools; >> -use File::Path; >> use PVE::Storage::Plugin; >> use PVE::JSONSchema qw(get_standard_option); >> >> use base qw(PVE::Storage::Plugin); >> >> +my $RM_LEADING_SLASHES = qr{^/+}; >> +my $RM_TRAILING_SLASHES = qr{/+$}; > Since you're removing the slashes at three different locations in this > file, I'd prefere having a function which does that. Would make the code > easier to read. You could also think about moving that function into the > common module, since I can imagine this is something we could use from > time to time. What do you think? Fair. I considered this initially but was on the fence. I agree though, and I'll pack this into a function in v2. > >> + >> # CIFS helper functions >> >> sub cifs_is_mounted : prototype($$) { >> @@ -19,8 +24,16 @@ sub cifs_is_mounted : prototype($$) { >> my ($mountpoint, $server, $share) = $scfg->@{ 'path', 'server', 'share' }; >> my $subdir = $scfg->{subdir} // ''; >> >> + for my $path ($share, $subdir) { > Is stripping share actually needed? The documented workflow > (pvesm scan cifs
) never returns slashes, and since share > is a single name rather than a path, slashes there look more like > invalid input than a formatting variant. You are right that, strictly speaking, the share parameter is an entity name and not a file path. However, I opted to sanitize it as well to handle manual configuration errors gracefully. I intuitively wrote 'share /share' out of habit, especially since it ultimately gets concatenated into a path-like string (//server/share/subdir). I'll add a brief note about this to the commit message in v2 as well. Example: cifs: smb-debian         path /mnt/pve/smb-debian         server 172.16.99.4         share /share         content import,rootdir,vztmpl,backup,images,iso,snippets         prune-backups keep-all=1         subdir /some/path/ > >> + $path =~ s/$RM_LEADING_SLASHES// if $path; >> + $path =~ s/$RM_TRAILING_SLASHES// if $path; >> + } >> + >> $server = "[$server]" if Net::IP::ip_is_ipv6($server); >> - my $source = "//${server}/$share$subdir"; >> + my $source = "//${server}"; >> + $source .= "/$share"; >> + $source .= "/$subdir" if $subdir; >> + >> $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata; >> >> return $mountpoint if grep { >> @@ -83,8 +96,15 @@ sub cifs_mount : prototype($$$$$) { >> my ($mountpoint, $server, $share, $options) = $scfg->@{ 'path', 'server', 'share', 'options' }; >> my $subdir = $scfg->{subdir} // ''; >> >> + for my $path ($share, $subdir) { >> + $path =~ s/$RM_LEADING_SLASHES// if $path; >> + $path =~ s/$RM_TRAILING_SLASHES// if $path; >> + } >> + >> $server = "[$server]" if Net::IP::ip_is_ipv6($server); >> - my $source = "//${server}/$share$subdir"; >> + my $source = "//${server}"; >> + $source .= "/$share"; >> + $source .= "/$subdir" if $subdir; >> >> my $cmd = ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o', 'soft']; >> >> @@ -285,7 +305,11 @@ sub deactivate_storage { >> sub check_connection { >> my ($class, $storeid, $scfg) = @_; >> >> - my $servicename = '//' . $scfg->{server} . '/' . $scfg->{share}; >> + my $share = $scfg->{share}; >> + $share =~ s/$RM_LEADING_SLASHES// if $share; >> + $share =~ s/$RM_TRAILING_SLASHES// if $share; >> + >> + my $servicename = '//' . $scfg->{server} . '/' . $share; >> >> my $cmd = ['/usr/bin/smbclient', $servicename, '-d', '0']; >>