Re: [PATCH pve-storage 2/2] fix #6050: cifs: normalize share and subdir paths for mount and status

[David Riley <d.riley@proxmox.com>]

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:
/<subdir>
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 <d.riley@proxmox.com>
>> ---
>>   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 <address>) 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'];
>>