[pve-devel] [PATCH common/container v2 0/2] fix invalid device passthrough being added to config

[pve-devel] [PATCH common/container v2 0/2] fix invalid device passthrough being added to config

[Filip Schauer]

Fix a bug that allows a device passthrough entry to be added to the
config despite the device path not pointing to a device. Previously,
adding an invalid device passthrough entry would throw an error, but the
entry would still be added to the config. This is fixed by moving the
respective checks from update_lxc_config to update_pct_config, which is
run before the entry is written to the config file.

Changes since v1:
* Use "if" instead of "unless"
* Move device path validation and stat to seperate helper function

pve-common:

Filip Schauer (1):
  add get_device_stat helper subroutine

 src/PVE/Tools.pm | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)


pve-container:

Filip Schauer (1):
  fix invalid device passthrough being added to config

 src/PVE/LXC.pm        | 18 ++++--------------
 src/PVE/LXC/Config.pm | 11 ++++++++++-
 2 files changed, 14 insertions(+), 15 deletions(-)


Summary over all repositories:
  3 files changed, 31 insertions(+), 16 deletions(-)

-- 
Generated by git-murpp 0.6.0

[pve-devel] [PATCH common v2 1/2] add get_device_stat helper subroutine

[Filip Schauer]

The get_device_stat subroutine gets a device path, validates it, and
returns the file mode and the device identifier.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Tools.pm | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 766c809..d1b53e5 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -7,7 +7,8 @@ use Date::Format qw(time2str);
 use Digest::MD5;
 use Digest::SHA;
 use Encode;
-use Fcntl qw(:DEFAULT :flock);
+use Errno qw(ENOENT);
+use Fcntl qw(:DEFAULT :flock :mode);
 use File::Basename;
 use File::Path qw(make_path);
 use Filesys::Df (); # don't overwrite our df()
@@ -1853,6 +1854,21 @@ sub dev_t_minor($) {
     return (($dev_t >> 12) & 0xfff00) | ($dev_t & 0xff);
 }
 
+sub get_device_stat {
+    my ($path) = @_;
+
+    die "Path is not defined\n" if !defined($path);
+
+    my ($mode, $rdev) = (stat($path))[2, 6];
+    die "Device $path does not exist\n" if $! == ENOENT;
+    die "Error accessing device $path\n"
+	if (!defined($mode) || !defined($rdev));
+    die "$path is not a device\n"
+	if (!S_ISBLK($mode) && !S_ISCHR($mode));
+
+    return ($mode, $rdev);
+}
+
 # Given an array of array refs [ \[a b c], \[a b b], \[e b a] ]
 # Returns the intersection of elements as a single array [a b]
 sub array_intersect {
-- 
2.39.2

[pve-devel] [PATCH container v2 2/2] fix invalid device passthrough being added to config

[Filip Schauer]

Fix a bug that allows a device passthrough entry to be added to the
config despite the device path not pointing to a device. Previously,
adding an invalid device passthrough entry would throw an error, but the
entry would still be added to the config. This is fixed by moving the
respective checks from update_lxc_config to update_pct_config, which is
run before the entry is written to the config file.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/LXC.pm        | 18 ++++--------------
 src/PVE/LXC/Config.pm | 11 ++++++++++-
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 9681d74..933b7f7 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use Cwd qw();
-use Errno qw(ELOOP ENOENT ENOTDIR EROFS ECONNREFUSED EEXIST);
+use Errno qw(ELOOP ENOTDIR EROFS ECONNREFUSED EEXIST);
 use Fcntl qw(O_RDONLY O_WRONLY O_NOFOLLOW O_DIRECTORY :mode);
 use File::Path;
 use File::Spec;
@@ -643,20 +643,10 @@ sub update_lxc_config {
     PVE::LXC::Config->foreach_passthrough_device($conf, sub {
 	my ($key, $device) = @_;
 
-	die "Path is not defined for passthrough device $key"
-	    unless (defined($device->{path}));
-
-	my $absolute_path = $device->{path};
-	my ($mode, $rdev) = (stat($absolute_path))[2, 6];
-
-	die "Device $absolute_path does not exist\n" if $! == ENOENT;
-
-	die "Error accessing device $absolute_path\n"
-	    if (!defined($mode) || !defined($rdev));
-
-	die "$absolute_path is not a device\n"
-	    if (!S_ISBLK($mode) && !S_ISCHR($mode));
+	die "Path is not defined for passthrough device $key\n"
+	    if !defined($device->{path});
 
+	my ($mode, $rdev) = PVE::Tools::get_device_stat($device->{path});
 	my $major = PVE::Tools::dev_t_major($rdev);
 	my $minor = PVE::Tools::dev_t_minor($rdev);
 	my $device_type_char = S_ISBLK($mode) ? 'b' : 'c';
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 5ac1446..fba20a1 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -3,7 +3,8 @@ package PVE::LXC::Config;
 use strict;
 use warnings;
 
-use Fcntl qw(O_RDONLY);
+use Errno qw(ENOENT);
+use Fcntl qw(O_RDONLY :mode);
 
 use PVE::AbstractConfig;
 use PVE::Cluster qw(cfs_register_file);
@@ -1193,6 +1194,14 @@ sub update_pct_config {
 		die "$opt: MTU size '$mtu' is bigger than bridge MTU '$bridge_mtu'\n"
 		    if ($mtu > $bridge_mtu);
 	    }
+	} elsif ($opt =~ m/^dev(\d+)$/) {
+	    my $device = $class->parse_device($value);
+
+	    die "Path is not defined for passthrough device $opt"
+		if !defined($device->{path});
+
+            # Validate device
+            PVE::Tools::get_device_stat($device->{path});
 	}
 	$conf->{pending}->{$opt} = $value;
 	$class->remove_from_pending_delete($conf, $opt);
-- 
2.39.2

Re: [pve-devel] [PATCH common v2 1/2] add get_device_stat helper subroutine

[Fiona Ebner]

Am 15.04.24 um 15:17 schrieb Filip Schauer:
> The get_device_stat subroutine gets a device path, validates it, and
> returns the file mode and the device identifier.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Tools.pm | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 766c809..d1b53e5 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -7,7 +7,8 @@ use Date::Format qw(time2str);
>  use Digest::MD5;
>  use Digest::SHA;
>  use Encode;
> -use Fcntl qw(:DEFAULT :flock);
> +use Errno qw(ENOENT);

We already have "use POSIX" with various error codes, so should be added
there.

> +use Fcntl qw(:DEFAULT :flock :mode);
>  use File::Basename;
>  use File::Path qw(make_path);
>  use Filesys::Df (); # don't overwrite our df()
> @@ -1853,6 +1854,21 @@ sub dev_t_minor($) {
>      return (($dev_t >> 12) & 0xfff00) | ($dev_t & 0xff);
>  }
>  
> +sub get_device_stat {

It's not the whole stat, so the name is a bit misleading. Tools.pm is
already very big and mixes different things, so not super happy about
adding new things to it. If it would return the whole stat, it could
still be fine IMHO, because it's more likely to be reusable later.
Alternatively, we can keep this code in pve-container.

> +    my ($path) = @_;
> +
> +    die "Path is not defined\n" if !defined($path);
> +
> +    my ($mode, $rdev) = (stat($path))[2, 6];
> +    die "Device $path does not exist\n" if $! == ENOENT;
> +    die "Error accessing device $path\n"
> +	if (!defined($mode) || !defined($rdev));
> +    die "$path is not a device\n"
> +	if (!S_ISBLK($mode) && !S_ISCHR($mode));

Style nit: useless parentheses, post-if can fit on the same line.

> +
> +    return ($mode, $rdev);
> +}
> +
>  # Given an array of array refs [ \[a b c], \[a b b], \[e b a] ]
>  # Returns the intersection of elements as a single array [a b]
>  sub array_intersect {

Re: [pve-devel] [PATCH common v2 1/2] add get_device_stat helper subroutine

[Filip Schauer]

Patch v3 is available:
https://lists.proxmox.com/pipermail/pve-devel/2024-April/062989.html

On 15/04/2024 15:54, Fiona Ebner wrote:
> Am 15.04.24 um 15:17 schrieb Filip Schauer:
>> The get_device_stat subroutine gets a device path, validates it, and
>> returns the file mode and the device identifier.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>>   src/PVE/Tools.pm | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
>> index 766c809..d1b53e5 100644
>> --- a/src/PVE/Tools.pm
>> +++ b/src/PVE/Tools.pm
>> @@ -7,7 +7,8 @@ use Date::Format qw(time2str);
>>   use Digest::MD5;
>>   use Digest::SHA;
>>   use Encode;
>> -use Fcntl qw(:DEFAULT :flock);
>> +use Errno qw(ENOENT);
> We already have "use POSIX" with various error codes, so should be added
> there.
>
>> +use Fcntl qw(:DEFAULT :flock :mode);
>>   use File::Basename;
>>   use File::Path qw(make_path);
>>   use Filesys::Df (); # don't overwrite our df()
>> @@ -1853,6 +1854,21 @@ sub dev_t_minor($) {
>>       return (($dev_t >> 12) & 0xfff00) | ($dev_t & 0xff);
>>   }
>>   
>> +sub get_device_stat {
> It's not the whole stat, so the name is a bit misleading. Tools.pm is
> already very big and mixes different things, so not super happy about
> adding new things to it. If it would return the whole stat, it could
> still be fine IMHO, because it's more likely to be reusable later.
> Alternatively, we can keep this code in pve-container.
>
>> +    my ($path) = @_;
>> +
>> +    die "Path is not defined\n" if !defined($path);
>> +
>> +    my ($mode, $rdev) = (stat($path))[2, 6];
>> +    die "Device $path does not exist\n" if $! == ENOENT;
>> +    die "Error accessing device $path\n"
>> +	if (!defined($mode) || !defined($rdev));
>> +    die "$path is not a device\n"
>> +	if (!S_ISBLK($mode) && !S_ISCHR($mode));
> Style nit: useless parentheses, post-if can fit on the same line.
>
>> +
>> +    return ($mode, $rdev);
>> +}
>> +
>>   # Given an array of array refs [ \[a b c], \[a b b], \[e b a] ]
>>   # Returns the intersection of elements as a single array [a b]
>>   sub array_intersect {