From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 233E81FF0A7 for ; Tue, 18 Aug 2026 12:23:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B7421214F5; Tue, 18 Aug 2026 12:23:40 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 18 Aug 2026 12:23:33 +0200 Message-Id: Subject: Re: [PATCH pve-storage 2/2] fix #6050: cifs: normalize share and subdir paths for mount and status From: "Jakob Klocker" To: "David Riley" , X-Mailer: aerc 0.20.0 References: <20260807093657.23649-1-d.riley@proxmox.com> <20260807093657.23649-3-d.riley@proxmox.com> In-Reply-To: <20260807093657.23649-3-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787048592185 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.763 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) 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: HANYWDHGA2UTYT7CRE7ETXGC277N4LYB X-Message-ID-Hash: HANYWDHGA2UTYT7CRE7ETXGC277N4LYB X-MailFrom: j.klocker@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: Comments inline. I'd also suggest mentioning why you didn't reuse the `canonpath` function you used in the previous patch here. 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-op= tions > > Link: https://bugzilla.proxmox.com/show_bug.cgi?id=3D6050 > 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.p= m > index 54f0f4e..78ed88c 100644 > --- a/src/PVE/Storage/CIFSPlugin.pm > +++ b/src/PVE/Storage/CIFSPlugin.pm > @@ -2,15 +2,20 @@ package PVE::Storage::CIFSPlugin; > =20 > 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); > =20 > use base qw(PVE::Storage::Plugin); > =20 > +my $RM_LEADING_SLASHES =3D qr{^/+}; > +my $RM_TRAILING_SLASHES =3D 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? > + > # CIFS helper functions > =20 > sub cifs_is_mounted : prototype($$) { > @@ -19,8 +24,16 @@ sub cifs_is_mounted : prototype($$) { > my ($mountpoint, $server, $share) =3D $scfg->@{ 'path', 'server', 's= hare' }; > my $subdir =3D $scfg->{subdir} // ''; > =20 > + for my $path ($share, $subdir) { Is stripping share actually needed? The documented workflow=20 (pvesm scan cifs
) never returns slashes, and since share=20 is a single name rather than a path, slashes there look more like invalid input than a formatting variant. > + $path =3D~ s/$RM_LEADING_SLASHES// if $path; > + $path =3D~ s/$RM_TRAILING_SLASHES// if $path; > + } > + > $server =3D "[$server]" if Net::IP::ip_is_ipv6($server); > - my $source =3D "//${server}/$share$subdir"; > + my $source =3D "//${server}"; > + $source .=3D "/$share"; > + $source .=3D "/$subdir" if $subdir; > + > $mountdata =3D PVE::ProcFSTools::parse_proc_mounts() if !$mountdata; > =20 > return $mountpoint if grep { > @@ -83,8 +96,15 @@ sub cifs_mount : prototype($$$$$) { > my ($mountpoint, $server, $share, $options) =3D $scfg->@{ 'path', 's= erver', 'share', 'options' }; > my $subdir =3D $scfg->{subdir} // ''; > =20 > + for my $path ($share, $subdir) { > + $path =3D~ s/$RM_LEADING_SLASHES// if $path; > + $path =3D~ s/$RM_TRAILING_SLASHES// if $path; > + } > + > $server =3D "[$server]" if Net::IP::ip_is_ipv6($server); > - my $source =3D "//${server}/$share$subdir"; > + my $source =3D "//${server}"; > + $source .=3D "/$share"; > + $source .=3D "/$subdir" if $subdir; > =20 > my $cmd =3D ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o',= 'soft']; > =20 > @@ -285,7 +305,11 @@ sub deactivate_storage { > sub check_connection { > my ($class, $storeid, $scfg) =3D @_; > =20 > - my $servicename =3D '//' . $scfg->{server} . '/' . $scfg->{share}; > + my $share =3D $scfg->{share}; > + $share =3D~ s/$RM_LEADING_SLASHES// if $share; > + $share =3D~ s/$RM_TRAILING_SLASHES// if $share; > + > + my $servicename =3D '//' . $scfg->{server} . '/' . $share; > =20 > my $cmd =3D ['/usr/bin/smbclient', $servicename, '-d', '0']; > =20