[pve-devel] [PATCH v4 many] Add container device passthrough

[pve-devel] [PATCH v4 many] Add container device passthrough

[Filip Schauer]

Changes since v1:
* mknod the devices in /var/lib/lxc/$vmid/passthrough and setup proper
  permissions instead of bind mounting the devices from /dev directly
* Add support for USB mapping
* Add foreach_passthrough_device helper function

Changes since v2:
* Remove support for USB mapping
* Add mknod syscall
* Setup a tmpfs to mknod the devices to instead of writing to the host
  file system
* Write passthrough dev list to /var/lib/lxc/$vmid/passthrough_devices
* Move sub foreach_passthrough_device out of the AbstractConfig into the
  container config
* Add mode, uid and gid properties to passthrough device config

Changes since v3:
* Allow only root user to configure device passthrough
* Verify format of device passthrough mode
* Add mount flag constants
* Remove redundant error checks
* Add missing error checks
* Optimize file creation by using mknod instead of touch
* Correctly handle id mapping with multiple ranges

pve-common:

Filip Schauer (2):
  tools: Add mknod syscall
  tools: Add mount flag constants

 src/PVE/Syscall.pm |  1 +
 src/PVE/Tools.pm   | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

pve-container:

Filip Schauer (1):
  Add device passthrough

 src/PVE/LXC.pm            | 57 +++++++++++++++++++++++-
 src/PVE/LXC/Config.pm     | 93 +++++++++++++++++++++++++++++++++++++++
 src/PVE/LXC/Tools.pm      | 23 +++++++---
 src/lxc-pve-autodev-hook  | 18 +++++++-
 src/lxc-pve-prestart-hook | 60 ++++++++++++++++++++++++-
 5 files changed, 242 insertions(+), 9 deletions(-)

[pve-devel] [PATCH v4 common 1/2] tools: Add mknod syscall

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Syscall.pm | 1 +
 src/PVE/Tools.pm   | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/src/PVE/Syscall.pm b/src/PVE/Syscall.pm
index 4c0b9cf..2a423e8 100644
--- a/src/PVE/Syscall.pm
+++ b/src/PVE/Syscall.pm
@@ -16,6 +16,7 @@ BEGIN {
 	openat => &SYS_openat,
 	close => &SYS_close,
 	mkdirat => &SYS_mkdirat,
+	mknod => &SYS_mknod,
 	faccessat => &SYS_faccessat,
 	setresuid => &SYS_setresuid,
 	fchownat => &SYS_fchownat,
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index c91e933..fbb6773 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1720,6 +1720,11 @@ sub mkdirat($$$) {
     return syscall(PVE::Syscall::mkdirat, int($dirfd), $name, int($mode)) == 0;
 }
 
+sub mknod($$$) {
+    my ($filename, $mode, $dev) = @_;
+    return syscall(PVE::Syscall::SYS_mknod, $filename, int($mode), int($dev)) == 0;
+}
+
 sub fchownat($$$$$) {
     my ($dirfd, $pathname, $owner, $group, $flags) = @_;
     return syscall(
-- 
2.39.2

[pve-devel] applied: [PATCH v4 common 1/2] tools: Add mknod syscall

[Thomas Lamprecht]

Am 13/11/2023 um 11:30 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Syscall.pm | 1 +
>  src/PVE/Tools.pm   | 5 +++++
>  2 files changed, 6 insertions(+)
> 
>

applied this one already, thanks!

[pve-devel] [PATCH v4 common 2/2] tools: Add mount flag constants

[Filip Schauer]

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

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index fbb6773..b3af2c6 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -62,6 +62,20 @@ CLONE_NEWIPC
 CLONE_NEWUSER
 CLONE_NEWPID
 CLONE_NEWNET
+MS_RDONLY
+MS_NOSUID
+MS_NODEV
+MS_NOEXEC
+MS_SYNCHRONOUS
+MS_REMOUNT
+MS_MANDLOCK
+MS_DIRSYNC
+MS_NOSYMFOLLOW
+MS_NOATIME
+MS_NODIRATIME
+MS_BIND
+MS_MOVE
+MS_REC
 );
 
 my $pvelogdir = "/var/log/pve";
@@ -110,6 +124,23 @@ use constant {RENAME_NOREPLACE => (1 << 0),
               RENAME_EXCHANGE  => (1 << 1),
               RENAME_WHITEOUT  => (1 << 2)};
 
+use constant {
+    MS_RDONLY      => (1),
+    MS_NOSUID      => (1 <<  1),
+    MS_NODEV       => (1 <<  2),
+    MS_NOEXEC      => (1 <<  3),
+    MS_SYNCHRONOUS => (1 <<  4),
+    MS_REMOUNT     => (1 <<  5),
+    MS_MANDLOCK    => (1 <<  6),
+    MS_DIRSYNC     => (1 <<  7),
+    MS_NOSYMFOLLOW => (1 <<  8),
+    MS_NOATIME     => (1 << 10),
+    MS_NODIRATIME  => (1 << 11),
+    MS_BIND        => (1 << 12),
+    MS_MOVE        => (1 << 13),
+    MS_REC         => (1 << 14),
+};
+
 sub run_with_timeout {
     my ($timeout, $code, @param) = @_;
 
-- 
2.39.2

[pve-devel] applied: [PATCH v4 common 2/2] tools: Add mount flag constants

[Thomas Lamprecht]

Am 13/11/2023 um 11:30 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Tools.pm | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
>

applied this one already too, thanks!

Albeit I'm not too happy with having those constants in PVE::Tools
in the first place, but there are so many pre-existing ones that
yours don't really make it worse. And just inventing some new
module that takes only yours is not that good either, if we need
to clean this up as a whole, and that's for a separate, future
patch series.

Re: [pve-devel] applied: [PATCH v4 common 2/2] tools: Add mount flag constants

[Wolfgang Bumiller]

On Mon, Nov 13, 2023 at 03:14:47PM +0100, Thomas Lamprecht wrote:
> Am 13/11/2023 um 11:30 schrieb Filip Schauer:
> > Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> > ---
> >  src/PVE/Tools.pm | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> >
> 
> applied this one already too, thanks!
> 
> Albeit I'm not too happy with having those constants in PVE::Tools
> in the first place, but there are so many pre-existing ones that
> yours don't really make it worse. And just inventing some new
> module that takes only yours is not that good either, if we need
> to clean this up as a whole, and that's for a separate, future
> patch series.

We can probably improve the situation a bit with rust. Or generate such
things from header or something...
pve-container (the only current user of this) is probably a good
candidate to start moving some more chunks over to rust anyway...

The only thing I would have done differently here is copy-pasta the
values from /usr/include/linux/mount.h without turning them into bit
shifts, that way they're more "easily"™ validated (for some
interpretation of "easy"), unless the bit shifted version already comes
from some other trusted source?
But doesn't matter much.

[pve-devel] [PATCH v4 container 1/1] Add device passthrough

[Filip Schauer]

Add a dev[n] argument to the container config to pass devices through to
a container. A device can be passed by its path. Additionally the access
mode, uid and gid can be specified through their respective properties.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/LXC.pm            | 57 +++++++++++++++++++++++-
 src/PVE/LXC/Config.pm     | 93 +++++++++++++++++++++++++++++++++++++++
 src/PVE/LXC/Tools.pm      | 23 +++++++---
 src/lxc-pve-autodev-hook  | 18 +++++++-
 src/lxc-pve-prestart-hook | 60 ++++++++++++++++++++++++-
 5 files changed, 242 insertions(+), 9 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7ec816b..242b2a0 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -5,7 +5,7 @@ use warnings;
 
 use Cwd qw();
 use Errno qw(ELOOP ENOTDIR EROFS ECONNREFUSED EEXIST);
-use Fcntl qw(O_RDONLY O_WRONLY O_NOFOLLOW O_DIRECTORY);
+use Fcntl qw(O_RDONLY O_WRONLY O_NOFOLLOW O_DIRECTORY :mode);
 use File::Path;
 use File::Spec;
 use IO::Poll qw(POLLIN POLLHUP);
@@ -639,6 +639,31 @@ sub update_lxc_config {
 	$raw .= "lxc.mount.auto = sys:mixed\n";
     }
 
+    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};
+
+	die "Device $absolute_path does not exist\n"
+		unless (-e $absolute_path);
+
+	die "$absolute_path is not a device\n"
+		unless (-b $absolute_path || -c $absolute_path);
+
+	my ($mode, $rdev) = (stat($absolute_path))[2, 6];
+
+	die "Could not get mode or device ID of $absolute_path\n"
+	    if (!defined($mode) || !defined($rdev));
+
+	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';
+	$raw .= "lxc.cgroup2.devices.allow = $device_type_char $major:$minor rw\n";
+    });
+
     # WARNING: DO NOT REMOVE this without making sure that loop device nodes
     # cannot be exposed to the container with r/w access (cgroup perms).
     # When this is enabled mounts will still remain in the monitor's namespace
@@ -1318,6 +1343,8 @@ sub check_ct_modify_config_perm {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
 	    check_bridge_access($rpcenv, $authuser, $oldconf->{$opt}) if $oldconf->{$opt};
 	    check_bridge_access($rpcenv, $authuser, $newconf->{$opt}) if $newconf->{$opt};
+	} elsif ($opt =~ m/^dev\d+$/) {
+	    raise_perm_exc("configuring device passthrough is only allowed for root\@pam");
 	} elsif ($opt eq 'nameserver' || $opt eq 'searchdomain' || $opt eq 'hostname') {
 	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
 	} elsif ($opt eq 'features') {
@@ -2367,6 +2394,34 @@ sub validate_id_maps {
     }
 }
 
+sub map_ct_id_to_host {
+    my ($id, $id_map, $id_type) = @_;
+
+    foreach my $mapping (@$id_map) {
+	my ($type, $ct, $host, $length) = @$mapping;
+
+	next if ($type ne $id_type);
+
+	if ($id >= int($ct) && $id < ($ct + $length)) {
+	    return $host - $ct + $id;
+	}
+    }
+
+    return $id;
+}
+
+sub map_ct_uid_to_host {
+    my ($uid, $id_map) = @_;
+
+    return map_ct_id_to_host($uid, $id_map, 'u');
+}
+
+sub map_ct_gid_to_host {
+    my ($gid, $id_map) = @_;
+
+    return map_ct_id_to_host($gid, $id_map, 'g');
+}
+
 sub userns_command {
     my ($id_map) = @_;
     if (@$id_map) {
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 56e1f10..9f325f2 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -29,6 +29,7 @@ mkdir $lockdir;
 mkdir "/etc/pve/nodes/$nodename/lxc";
 my $MAX_MOUNT_POINTS = 256;
 my $MAX_UNUSED_DISKS = $MAX_MOUNT_POINTS;
+my $MAX_DEVICES = 256;
 
 # BEGIN implemented abstract methods from PVE::AbstractConfig
 
@@ -908,6 +909,71 @@ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
     }
 }
 
+PVE::JSONSchema::register_format('pve-lxc-dev-string', \&verify_lxc_dev_string);
+sub verify_lxc_dev_string {
+    my ($dev, $noerr) = @_;
+
+    if (
+	$dev =~ m@/\.\.?/@ ||
+	$dev =~ m@/\.\.?$@ ||
+	$dev !~ m!^/dev/!
+    ) {
+	return undef if $noerr;
+	die "$dev is not a valid device path\n";
+    }
+
+    return $dev;
+}
+
+PVE::JSONSchema::register_format('file-access-mode-string', \&verify_file_access_mode);
+sub verify_file_access_mode {
+    my ($mode, $noerr) = @_;
+
+    if ($mode !~ /^[0-7]*$/) {
+	return undef if $noerr;
+	die "$mode is not a valid file access mode\n";
+    }
+
+    return $mode;
+}
+
+my $dev_desc = {
+    path => {
+	optional => 1,
+	type => 'string',
+	default_key => 1,
+	format => 'pve-lxc-dev-string',
+	format_description => 'Path',
+	description => 'Device to pass through to the container',
+	verbose_description => 'Path to the device to pass through to the container',
+    },
+    mode => {
+	optional => 1,
+	type => 'integer',
+	format => 'file-access-mode-string',
+	format_description => 'Octal access mode',
+	description => 'Access mode to be set on the device node',
+    },
+    uid => {
+	optional => 1,
+	type => 'integer',
+	description => 'User ID to be assigned to the device node',
+    },
+    gid => {
+	optional => 1,
+	type => 'integer',
+	description => 'Group ID to be assigned to the device node',
+    },
+};
+
+for (my $i = 0; $i < $MAX_DEVICES; $i++) {
+    $confdesc->{"dev$i"} = {
+	optional => 1,
+	type => 'string', format => $dev_desc,
+	description => "Device to pass through to the container",
+    }
+}
+
 sub parse_pct_config {
     my ($filename, $raw, $strict) = @_;
 
@@ -1255,6 +1321,21 @@ sub parse_volume {
     return;
 }
 
+sub parse_device {
+    my ($class, $device_string, $noerr) = @_;
+
+    my $res;
+    eval { $res = PVE::JSONSchema::parse_property_string($dev_desc, $device_string) };
+    if ($@) {
+	return undef if $noerr;
+	die $@;
+    }
+
+    die "Path has to be defined" unless (defined($res->{path}));
+
+    return $res;
+}
+
 sub print_volume {
     my ($class, $key, $volume) = @_;
 
@@ -1762,4 +1843,16 @@ sub get_derived_property {
     }
 }
 
+sub foreach_passthrough_device {
+    my ($class, $conf, $func, @param) = @_;
+
+    foreach my $key (keys %$conf) {
+	next if $key !~ m/^dev(\d+)$/;
+
+	my $device = $class->parse_device($conf->{$key});
+
+	$func->($key, $device, @param);
+    }
+}
+
 1;
diff --git a/src/PVE/LXC/Tools.pm b/src/PVE/LXC/Tools.pm
index 62cdbc1..c9fb834 100644
--- a/src/PVE/LXC/Tools.pm
+++ b/src/PVE/LXC/Tools.pm
@@ -81,10 +81,9 @@ sub lxc_hook($$&) {
     $code->($ct_name, $common_vars, $namespaces, $args);
 }
 
-sub for_current_devices($&) {
-    my ($vmid, $code) = @_;
+sub for_devices {
+    my ($devlist_file, $vmid, $code) = @_;
 
-    my $devlist_file = "/var/lib/lxc/$vmid/devices";
     my $fd;
 
     if (! open $fd, '<', $devlist_file) {
@@ -93,8 +92,8 @@ sub for_current_devices($&) {
     }
 
     while (defined(my $line = <$fd>)) {
-	if ($line !~ m@^(b):(\d+):(\d+):/dev/(\S+)\s*$@) {
-	    warn "invalid .pve-devices entry: $line\n";
+	if ($line !~ m@^(b|c):(\d+):(\d+):/dev/(\S+)\s*$@) {
+	    warn "invalid $devlist_file entry: $line\n";
 	    next;
 	}
 
@@ -117,6 +116,20 @@ sub for_current_devices($&) {
     close $fd;
 }
 
+sub for_current_devices($&) {
+    my ($vmid, $code) = @_;
+
+    my $devlist_file = "/var/lib/lxc/$vmid/devices";
+    for_devices($devlist_file, $vmid, $code);
+}
+
+sub for_passthrough_devices($&) {
+    my ($vmid, $code) = @_;
+
+    my $passthrough_devlist_file = "/var/lib/lxc/$vmid/passthrough_devices";
+    for_devices($passthrough_devlist_file, $vmid, $code);
+}
+
 sub cgroup_do_write($$) {
     my ($path, $value) = @_;
     my $fd;
diff --git a/src/lxc-pve-autodev-hook b/src/lxc-pve-autodev-hook
index 3c45949..105465f 100755
--- a/src/lxc-pve-autodev-hook
+++ b/src/lxc-pve-autodev-hook
@@ -3,17 +3,31 @@
 use strict;
 use warnings;
 
-use File::Path;
+use Fcntl qw(S_IFREG);
 use File::Basename;
+use File::Path;
 
 use PVE::LXC::Tools;
-use PVE::Tools;
+use PVE::Tools qw(MS_BIND);
 
 PVE::LXC::Tools::lxc_hook('autodev', 'lxc', sub {
     my ($vmid, $vars, undef, undef) = @_;
 
     my $root = $vars->{ROOTFS_MOUNT};
 
+    PVE::LXC::Tools::for_passthrough_devices($vmid, sub {
+	my ($type, $major, $minor, $dev) = @_;
+
+	my $rel_devpath = "/dev/$dev";
+	my $rel_dir = dirname($rel_devpath);
+	File::Path::mkpath("$root/$rel_dir");
+	PVE::Tools::mknod("$root/dev/$dev", S_IFREG, 0)
+	    or die("Could not mknod $root/dev/$dev");
+
+	PVE::Tools::mount("/var/lib/lxc/$vmid/passthrough/dev/$dev", "$root/dev/$dev", 0, MS_BIND, 0)
+	    or die("Bind mount of device $dev into container failed\n");
+    });
+
     PVE::LXC::Tools::for_current_devices($vmid, sub {
 	my ($type, $major, $minor, $dev) = @_;
 
diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
index 936d0bf..d8b0106 100755
--- a/src/lxc-pve-prestart-hook
+++ b/src/lxc-pve-prestart-hook
@@ -6,6 +6,7 @@ use strict;
 use warnings;
 
 use Fcntl qw(O_DIRECTORY :mode);
+use File::Basename;
 use File::Path;
 use POSIX;
 
@@ -62,7 +63,7 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
     unlink $devlist_file;
     my $devices = [];
 
-    my (undef, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
+    my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
 
     # Unmount first when the user mounted the container with "pct mount".
     eval {
@@ -118,6 +119,51 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
 
     PVE::LXC::Config->foreach_volume($conf, $setup_mountpoint);
 
+    # Device passthrough
+    my $passthrough_devlist_file = "/var/lib/lxc/$vmid/passthrough_devices";
+    unlink $passthrough_devlist_file;
+    my $passthrough_devices = [];
+
+    my $passthrough_dir = "/var/lib/lxc/$vmid/passthrough";
+    File::Path::make_path($passthrough_dir);
+    PVE::Tools::mount("none", $passthrough_dir, "tmpfs", 0, "size=8k")
+        or die ("Could not mount tmpfs for device passthrough at $passthrough_dir");
+
+    my $setup_passthrough_device = sub {
+	my ($key, $device) = @_;
+
+	my $absolute_path = $device->{path};
+	my ($mode, $rdev) = (stat($absolute_path))[2, 6];
+
+	die "Could not get mode or device ID of $absolute_path\n"
+	    if (!defined($mode) || !defined($rdev));
+
+	my $passthrough_device_path = $passthrough_dir . $absolute_path;
+	File::Path::make_path(dirname($passthrough_device_path));
+	PVE::Tools::mknod($passthrough_device_path, $mode, $rdev)
+	    or die("failed to mknod $passthrough_device_path\n");
+
+	# Use chmod because umask could mess with the access mode on mknod
+	my $passthrough_mode = 0660;
+	$passthrough_mode = oct($device->{mode}) if (defined($device->{mode}));
+	chmod $passthrough_mode, $passthrough_device_path
+	    or die "failed to chmod $passthrough_mode $passthrough_device_path: $!\n";
+
+	# Set uid and gid of the device node
+	my $uid = 0;
+	my $gid = 0;
+	$uid = $device->{uid} if (defined($device->{uid}));
+	$gid = $device->{gid} if (defined($device->{gid}));
+	$uid = PVE::LXC::map_ct_uid_to_host($uid, $id_map);
+	$gid = PVE::LXC::map_ct_gid_to_host($gid, $id_map);
+	chown $uid, $gid, $passthrough_device_path
+	    or die("failed to chown $uid:$gid $passthrough_device_path: $!\n");
+
+	push @$passthrough_devices, [$absolute_path, $mode, $rdev];
+    };
+
+    PVE::LXC::Config->foreach_passthrough_device($conf, $setup_passthrough_device);
+
     my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
     $lxc_setup->pre_start_hook();
 
@@ -140,6 +186,18 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
 	}
 	PVE::Tools::file_set_contents($devlist_file, $devlist);
     }
+
+    if (@$passthrough_devices) {
+	my $devlist = '';
+	foreach my $dev (@$passthrough_devices) {
+	    my ($path, $mode, $rdev) = @$dev;
+	    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';
+	    $devlist .= "$device_type_char:$major:$minor:$path\n";
+	}
+	PVE::Tools::file_set_contents($passthrough_devlist_file, $devlist);
+    }
 });
 
 # Leftover cgroups prevent lxc from starting without any useful information
-- 
2.39.2

Re: [pve-devel] [PATCH v4 container 1/1] Add device passthrough

[Thomas Lamprecht]

concept wise this looks pretty much OK, but a few (mostly code-style) comments in line

Am 13/11/2023 um 11:30 schrieb Filip Schauer:
> Add a dev[n] argument to the container config to pass devices through to
> a container. A device can be passed by its path. Additionally the access
> mode, uid and gid can be specified through their respective properties.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/LXC.pm            | 57 +++++++++++++++++++++++-
>  src/PVE/LXC/Config.pm     | 93 +++++++++++++++++++++++++++++++++++++++
>  src/PVE/LXC/Tools.pm      | 23 +++++++---
>  src/lxc-pve-autodev-hook  | 18 +++++++-
>  src/lxc-pve-prestart-hook | 60 ++++++++++++++++++++++++-
>  5 files changed, 242 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 7ec816b..242b2a0 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -5,7 +5,7 @@ use warnings;
>  
>  use Cwd qw();
>  use Errno qw(ELOOP ENOTDIR EROFS ECONNREFUSED EEXIST);
> -use Fcntl qw(O_RDONLY O_WRONLY O_NOFOLLOW O_DIRECTORY);
> +use Fcntl qw(O_RDONLY O_WRONLY O_NOFOLLOW O_DIRECTORY :mode);
>  use File::Path;
>  use File::Spec;
>  use IO::Poll qw(POLLIN POLLHUP);
> @@ -639,6 +639,31 @@ sub update_lxc_config {
>  	$raw .= "lxc.mount.auto = sys:mixed\n";
>      }
>  
> +    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};
> +
> +	die "Device $absolute_path does not exist\n"
> +		unless (-e $absolute_path);
> +
> +	die "$absolute_path is not a device\n"
> +		unless (-b $absolute_path || -c $absolute_path);

Those are basically doing a stat each, but you do one below anyway,
so maybe use that one for the checks

> +
> +	my ($mode, $rdev) = (stat($absolute_path))[2, 6];
> +
> +	die "Could not get mode or device ID of $absolute_path\n"
> +	    if (!defined($mode) || !defined($rdev));
> +
> +	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';
> +	$raw .= "lxc.cgroup2.devices.allow = $device_type_char $major:$minor rw\n";
> +    });
> +
>      # WARNING: DO NOT REMOVE this without making sure that loop device nodes
>      # cannot be exposed to the container with r/w access (cgroup perms).
>      # When this is enabled mounts will still remain in the monitor's namespace
> @@ -1318,6 +1343,8 @@ sub check_ct_modify_config_perm {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
>  	    check_bridge_access($rpcenv, $authuser, $oldconf->{$opt}) if $oldconf->{$opt};
>  	    check_bridge_access($rpcenv, $authuser, $newconf->{$opt}) if $newconf->{$opt};
> +	} elsif ($opt =~ m/^dev\d+$/) {
> +	    raise_perm_exc("configuring device passthrough is only allowed for root\@pam");
>  	} elsif ($opt eq 'nameserver' || $opt eq 'searchdomain' || $opt eq 'hostname') {
>  	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Network']);
>  	} elsif ($opt eq 'features') {
> @@ -2367,6 +2394,34 @@ sub validate_id_maps {
>      }
>  }
>  
> +sub map_ct_id_to_host {
> +    my ($id, $id_map, $id_type) = @_;
> +
> +    foreach my $mapping (@$id_map) {

use for over foreach

> +	my ($type, $ct, $host, $length) = @$mapping;
> +
> +	next if ($type ne $id_type);
> +
> +	if ($id >= int($ct) && $id < ($ct + $length)) {
> +	    return $host - $ct + $id;

do we want to throw around some int() here too for good measure?

> +	}
> +    }
> +
> +    return $id;
> +}
> +
> +sub map_ct_uid_to_host {
> +    my ($uid, $id_map) = @_;
> +
> +    return map_ct_id_to_host($uid, $id_map, 'u');
> +}
> +
> +sub map_ct_gid_to_host {
> +    my ($gid, $id_map) = @_;
> +
> +    return map_ct_id_to_host($gid, $id_map, 'g');
> +}
> +
>  sub userns_command {
>      my ($id_map) = @_;
>      if (@$id_map) {
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 56e1f10..9f325f2 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -29,6 +29,7 @@ mkdir $lockdir;
>  mkdir "/etc/pve/nodes/$nodename/lxc";
>  my $MAX_MOUNT_POINTS = 256;
>  my $MAX_UNUSED_DISKS = $MAX_MOUNT_POINTS;
> +my $MAX_DEVICES = 256;
>  
>  # BEGIN implemented abstract methods from PVE::AbstractConfig
>  
> @@ -908,6 +909,71 @@ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
>      }
>  }
>  
> +PVE::JSONSchema::register_format('pve-lxc-dev-string', \&verify_lxc_dev_string);
> +sub verify_lxc_dev_string {
> +    my ($dev, $noerr) = @_;
> +
> +    if (
> +	$dev =~ m@/\.\.?/@ ||
> +	$dev =~ m@/\.\.?$@ ||

could be a single regex:

$dev =~ @/\.\.?(?:/|$)@

but no hard feelings, all variant are not easily readable and need close
checking anyway (iow. like most regexes)

> +	$dev !~ m!^/dev/!
> +    ) {
> +	return undef if $noerr;
> +	die "$dev is not a valid device path\n";
> +    }
> +
> +    return $dev;
> +}
> +
> +PVE::JSONSchema::register_format('file-access-mode-string', \&verify_file_access_mode);
> +sub verify_file_access_mode {
> +    my ($mode, $noerr) = @_;
> +
> +    if ($mode !~ /^[0-7]*$/) {

this would allow an empty mode though? Also, not sure if we want to allow
partial modes like 77 ?

> +	return undef if $noerr;
> +	die "$mode is not a valid file access mode\n";
> +    }
> +
> +    return $mode;
> +}
> +
> +my $dev_desc = {
> +    path => {
> +	optional => 1,
> +	type => 'string',
> +	default_key => 1,
> +	format => 'pve-lxc-dev-string',
> +	format_description => 'Path',
> +	description => 'Device to pass through to the container',
> +	verbose_description => 'Path to the device to pass through to the container',
> +    },
> +    mode => {
> +	optional => 1,
> +	type => 'integer',
> +	format => 'file-access-mode-string',
> +	format_description => 'Octal access mode',
> +	description => 'Access mode to be set on the device node',
> +    },
> +    uid => {
> +	optional => 1,
> +	type => 'integer',
> +	description => 'User ID to be assigned to the device node',
> +    },
> +    gid => {
> +	optional => 1,
> +	type => 'integer',
> +	description => 'Group ID to be assigned to the device node',
> +    },
> +};
> +
> +for (my $i = 0; $i < $MAX_DEVICES; $i++) {
> +    $confdesc->{"dev$i"} = {
> +	optional => 1,
> +	type => 'string', format => $dev_desc,
> +	description => "Device to pass through to the container",
> +    }
> +}
> +
>  sub parse_pct_config {
>      my ($filename, $raw, $strict) = @_;
>  
> @@ -1255,6 +1321,21 @@ sub parse_volume {
>      return;
>  }
>  
> +sub parse_device {
> +    my ($class, $device_string, $noerr) = @_;
> +
> +    my $res;
> +    eval { $res = PVE::JSONSchema::parse_property_string($dev_desc, $device_string) };

you can write above as:

my $res = eval { PVE::JSONSchema::parse_property_string($dev_desc, $device_string) };

which makes it slightly nicer

> +    if ($@) {
> +	return undef if $noerr;
> +	die $@;
> +    }
> +
> +    die "Path has to be defined" unless (defined($res->{path}));

We do not use `unless`, it's basically one of the few things our code universally
(well almost, nobody is perfect..) agrees too, so lets keep it that way and use

... if !defined(...);

also, shouldn't a set $noerr also disarm this die? I.e.:

die "..." if !$noerr && !defined(...);

> +
> +    return $res;
> +}
> +
>  sub print_volume {
>      my ($class, $key, $volume) = @_;
>  
> @@ -1762,4 +1843,16 @@ sub get_derived_property {
>      }
>  }
>  
> +sub foreach_passthrough_device {
> +    my ($class, $conf, $func, @param) = @_;
> +
> +    foreach my $key (keys %$conf) {

prefer for over foreach for new code (no semantic reason, just for consistency)

> +	next if $key !~ m/^dev(\d+)$/;
> +
> +	my $device = $class->parse_device($conf->{$key});
> +
> +	$func->($key, $device, @param);
> +    }
> +}
> +
>  1;
> diff --git a/src/PVE/LXC/Tools.pm b/src/PVE/LXC/Tools.pm
> index 62cdbc1..c9fb834 100644
> --- a/src/PVE/LXC/Tools.pm
> +++ b/src/PVE/LXC/Tools.pm
> @@ -81,10 +81,9 @@ sub lxc_hook($$&) {
>      $code->($ct_name, $common_vars, $namespaces, $args);
>  }
>  
> -sub for_current_devices($&) {
> -    my ($vmid, $code) = @_;
> +sub for_devices {
> +    my ($devlist_file, $vmid, $code) = @_;
>  
> -    my $devlist_file = "/var/lib/lxc/$vmid/devices";
>      my $fd;
>  
>      if (! open $fd, '<', $devlist_file) {
> @@ -93,8 +92,8 @@ sub for_current_devices($&) {
>      }
>  
>      while (defined(my $line = <$fd>)) {
> -	if ($line !~ m@^(b):(\d+):(\d+):/dev/(\S+)\s*$@) {
> -	    warn "invalid .pve-devices entry: $line\n";
> +	if ($line !~ m@^(b|c):(\d+):(\d+):/dev/(\S+)\s*$@) {
> +	    warn "invalid $devlist_file entry: $line\n";
>  	    next;
>  	}
>  
> @@ -117,6 +116,20 @@ sub for_current_devices($&) {
>      close $fd;
>  }
>  
> +sub for_current_devices($&) {
> +    my ($vmid, $code) = @_;
> +
> +    my $devlist_file = "/var/lib/lxc/$vmid/devices";
> +    for_devices($devlist_file, $vmid, $code);
> +}
> +
> +sub for_passthrough_devices($&) {
> +    my ($vmid, $code) = @_;
> +
> +    my $passthrough_devlist_file = "/var/lib/lxc/$vmid/passthrough_devices";
> +    for_devices($passthrough_devlist_file, $vmid, $code);
> +}
> +
>  sub cgroup_do_write($$) {
>      my ($path, $value) = @_;
>      my $fd;
> diff --git a/src/lxc-pve-autodev-hook b/src/lxc-pve-autodev-hook
> index 3c45949..105465f 100755
> --- a/src/lxc-pve-autodev-hook
> +++ b/src/lxc-pve-autodev-hook
> @@ -3,17 +3,31 @@
>  use strict;
>  use warnings;
>  
> -use File::Path;
> +use Fcntl qw(S_IFREG);
>  use File::Basename;
> +use File::Path;
>  
>  use PVE::LXC::Tools;
> -use PVE::Tools;
> +use PVE::Tools qw(MS_BIND);
>  
>  PVE::LXC::Tools::lxc_hook('autodev', 'lxc', sub {
>      my ($vmid, $vars, undef, undef) = @_;
>  
>      my $root = $vars->{ROOTFS_MOUNT};
>  
> +    PVE::LXC::Tools::for_passthrough_devices($vmid, sub {
> +	my ($type, $major, $minor, $dev) = @_;
> +
> +	my $rel_devpath = "/dev/$dev";
> +	my $rel_dir = dirname($rel_devpath);
> +	File::Path::mkpath("$root/$rel_dir");
> +	PVE::Tools::mknod("$root/dev/$dev", S_IFREG, 0)
> +	    or die("Could not mknod $root/dev/$dev");

Please include $! in the error

> +
> +	PVE::Tools::mount("/var/lib/lxc/$vmid/passthrough/dev/$dev", "$root/dev/$dev", 0, MS_BIND, 0)
> +	    or die("Bind mount of device $dev into container failed\n");

same here, add $! in error

> +    });
> +
>      PVE::LXC::Tools::for_current_devices($vmid, sub {
>  	my ($type, $major, $minor, $dev) = @_;
>  
> diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
> index 936d0bf..d8b0106 100755
> --- a/src/lxc-pve-prestart-hook
> +++ b/src/lxc-pve-prestart-hook
> @@ -6,6 +6,7 @@ use strict;
>  use warnings;
>  
>  use Fcntl qw(O_DIRECTORY :mode);
> +use File::Basename;
>  use File::Path;
>  use POSIX;
>  
> @@ -62,7 +63,7 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
>      unlink $devlist_file;
>      my $devices = [];
>  
> -    my (undef, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
> +    my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
>  
>      # Unmount first when the user mounted the container with "pct mount".
>      eval {
> @@ -118,6 +119,51 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
>  
>      PVE::LXC::Config->foreach_volume($conf, $setup_mountpoint);
>  
> +    # Device passthrough
> +    my $passthrough_devlist_file = "/var/lib/lxc/$vmid/passthrough_devices";

would favor kebab-case for filenames too

> +    unlink $passthrough_devlist_file;

Would be good to have error handling there:

unlink or $!{ENOENT} or die "failed to clean-up passthrough device list file - $!\n"



> +    my $passthrough_devices = [];
> +
> +    my $passthrough_dir = "/var/lib/lxc/$vmid/passthrough";

no hard feelings, but what about using a common directory for the device
list file and the mountpoint:

/var/lib/lxc/$vmid/passthrough/devices
/var/lib/lxc/$vmid/passthrough/mount

But I did not really check the overall picture, so this can be fine as is
(well, with snake_case -> kebab-case)

> +    File::Path::make_path($passthrough_dir);
> +    PVE::Tools::mount("none", $passthrough_dir, "tmpfs", 0, "size=8k")
> +        or die ("Could not mount tmpfs for device passthrough at $passthrough_dir");

please include $! in the error message so that we have a better idea what
may be going on if a user reports running into this error.

> +
> +    my $setup_passthrough_device = sub {
> +	my ($key, $device) = @_;
> +
> +	my $absolute_path = $device->{path};
> +	my ($mode, $rdev) = (stat($absolute_path))[2, 6];
> +
> +	die "Could not get mode or device ID of $absolute_path\n"
> +	    if (!defined($mode) || !defined($rdev));
> +
> +	my $passthrough_device_path = $passthrough_dir . $absolute_path;
> +	File::Path::make_path(dirname($passthrough_device_path));
> +	PVE::Tools::mknod($passthrough_device_path, $mode, $rdev)
> +	    or die("failed to mknod $passthrough_device_path\n");

please include $! in the error

> +
> +	# Use chmod because umask could mess with the access mode on mknod
> +	my $passthrough_mode = 0660;
> +	$passthrough_mode = oct($device->{mode}) if (defined($device->{mode}));
> +	chmod $passthrough_mode, $passthrough_device_path
> +	    or die "failed to chmod $passthrough_mode $passthrough_device_path: $!\n";
> +
> +	# Set uid and gid of the device node
> +	my $uid = 0;
> +	my $gid = 0;
> +	$uid = $device->{uid} if (defined($device->{uid}));
> +	$gid = $device->{gid} if (defined($device->{gid}));

you can drop the outer parenthesis in the post-if's expression for both of above lines
for consistency.

> +	$uid = PVE::LXC::map_ct_uid_to_host($uid, $id_map);
> +	$gid = PVE::LXC::map_ct_gid_to_host($gid, $id_map);
> +	chown $uid, $gid, $passthrough_device_path
> +	    or die("failed to chown $uid:$gid $passthrough_device_path: $!\n");
> +
> +	push @$passthrough_devices, [$absolute_path, $mode, $rdev];
> +    };
> +
> +    PVE::LXC::Config->foreach_passthrough_device($conf, $setup_passthrough_device);
> +
>      my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
>      $lxc_setup->pre_start_hook();
>  
> @@ -140,6 +186,18 @@ PVE::LXC::Tools::lxc_hook('pre-start', 'lxc', sub {
>  	}
>  	PVE::Tools::file_set_contents($devlist_file, $devlist);
>      }
> +
> +    if (@$passthrough_devices) {
> +	my $devlist = '';
> +	foreach my $dev (@$passthrough_devices) {

s/foreach/for/

> +	    my ($path, $mode, $rdev) = @$dev;
> +	    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';
> +	    $devlist .= "$device_type_char:$major:$minor:$path\n";
> +	}
> +	PVE::Tools::file_set_contents($passthrough_devlist_file, $devlist);
> +    }
>  });
>  
>  # Leftover cgroups prevent lxc from starting without any useful information

Re: [pve-devel] [PATCH v4 container 1/1] Add device passthrough

[Wolfgang Bumiller]

On Wed, Nov 15, 2023 at 03:14:50PM +0100, Thomas Lamprecht wrote:
> concept wise this looks pretty much OK, but a few (mostly code-style) comments in line
> 
> Am 13/11/2023 um 11:30 schrieb Filip Schauer:
> > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> > index 56e1f10..9f325f2 100644
> > --- a/src/PVE/LXC/Config.pm
> > +++ b/src/PVE/LXC/Config.pm
> > @@ -29,6 +29,7 @@ mkdir $lockdir;
> >  mkdir "/etc/pve/nodes/$nodename/lxc";
> >  my $MAX_MOUNT_POINTS = 256;
> >  my $MAX_UNUSED_DISKS = $MAX_MOUNT_POINTS;
> > +my $MAX_DEVICES = 256;
> >  
> >  # BEGIN implemented abstract methods from PVE::AbstractConfig
> >  
> > @@ -908,6 +909,71 @@ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
> >      }
> >  }
> >  
> > +PVE::JSONSchema::register_format('pve-lxc-dev-string', \&verify_lxc_dev_string);
> > +sub verify_lxc_dev_string {
> > +    my ($dev, $noerr) = @_;
> > +
> > +    if (
> > +	$dev =~ m@/\.\.?/@ ||
> > +	$dev =~ m@/\.\.?$@ ||
> 
> could be a single regex:
> 
> $dev =~ @/\.\.?(?:/|$)@
> 
> but no hard feelings, all variant are not easily readable and need close
> checking anyway (iow. like most regexes)
> 
> > +	$dev !~ m!^/dev/!
> > +    ) {
> > +	return undef if $noerr;
> > +	die "$dev is not a valid device path\n";
> > +    }
> > +
> > +    return $dev;
> > +}
> > +
> > +PVE::JSONSchema::register_format('file-access-mode-string', \&verify_file_access_mode);
> > +sub verify_file_access_mode {
> > +    my ($mode, $noerr) = @_;
> > +
> > +    if ($mode !~ /^[0-7]*$/) {
> 
> this would allow an empty mode though? Also, not sure if we want to allow
> partial modes like 77 ?

Yeah, an empty mode should not be allowed. Not sure what you mean by
partial though, other than the missing leading zero.

For octal we should definitely enforce the leading zero.

> 
> > +	return undef if $noerr;
> > +	die "$mode is not a valid file access mode\n";
> > +    }
> > +
> > +    return $mode;
> > +}
> > +
> > +my $dev_desc = {
> > +    path => {
> > +	optional => 1,
> > +	type => 'string',
> > +	default_key => 1,
> > +	format => 'pve-lxc-dev-string',
> > +	format_description => 'Path',
> > +	description => 'Device to pass through to the container',
> > +	verbose_description => 'Path to the device to pass through to the container',
> > +    },
> > +    mode => {
> > +	optional => 1,
> > +	type => 'integer',
> > +	format => 'file-access-mode-string',

... this should be `type => 'string'` (integer just doesn't enforce a
format),
the `format` should be dropped (including the registered sub above), and
instead use 'pattern' as:

    pattern => '0[0-7]*',

JSONSchema anchors the pattern, so no need to include '^' and '$',
although I also wouldn't mind using

    pattern => qr/^0[0-7]*$/,

> > +	format_description => 'Octal access mode',
> > +	description => 'Access mode to be set on the device node',
> > +    },
> > +    uid => {
> > +	optional => 1,
> > +	type => 'integer',
> > +	description => 'User ID to be assigned to the device node',
> > +    },
> > +    gid => {
> > +	optional => 1,
> > +	type => 'integer',
> > +	description => 'Group ID to be assigned to the device node',

Add `minimum => 0`, to both uid and gid.

Re: [pve-devel] [PATCH v4 container 1/1] Add device passthrough

[Filip Schauer]

Patch v5 available

https://lists.proxmox.com/pipermail/pve-devel/2023-November/060285.html

On 16/11/2023 09:32, Wolfgang Bumiller wrote:
> On Wed, Nov 15, 2023 at 03:14:50PM +0100, Thomas Lamprecht wrote:
>> concept wise this looks pretty much OK, but a few (mostly code-style) comments in line
>>
>> Am 13/11/2023 um 11:30 schrieb Filip Schauer:
>>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>>> index 56e1f10..9f325f2 100644
>>> --- a/src/PVE/LXC/Config.pm
>>> +++ b/src/PVE/LXC/Config.pm
>>> @@ -29,6 +29,7 @@ mkdir $lockdir;
>>>   mkdir "/etc/pve/nodes/$nodename/lxc";
>>>   my $MAX_MOUNT_POINTS = 256;
>>>   my $MAX_UNUSED_DISKS = $MAX_MOUNT_POINTS;
>>> +my $MAX_DEVICES = 256;
>>>   
>>>   # BEGIN implemented abstract methods from PVE::AbstractConfig
>>>   
>>> @@ -908,6 +909,71 @@ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
>>>       }
>>>   }
>>>   
>>> +PVE::JSONSchema::register_format('pve-lxc-dev-string', \&verify_lxc_dev_string);
>>> +sub verify_lxc_dev_string {
>>> +    my ($dev, $noerr) = @_;
>>> +
>>> +    if (
>>> +	$dev =~ m@/\.\.?/@ ||
>>> +	$dev =~ m@/\.\.?$@ ||
>> could be a single regex:
>>
>> $dev =~ @/\.\.?(?:/|$)@
>>
>> but no hard feelings, all variant are not easily readable and need close
>> checking anyway (iow. like most regexes)
>>
>>> +	$dev !~ m!^/dev/!
>>> +    ) {
>>> +	return undef if $noerr;
>>> +	die "$dev is not a valid device path\n";
>>> +    }
>>> +
>>> +    return $dev;
>>> +}
>>> +
>>> +PVE::JSONSchema::register_format('file-access-mode-string', \&verify_file_access_mode);
>>> +sub verify_file_access_mode {
>>> +    my ($mode, $noerr) = @_;
>>> +
>>> +    if ($mode !~ /^[0-7]*$/) {
>> this would allow an empty mode though? Also, not sure if we want to allow
>> partial modes like 77 ?
> Yeah, an empty mode should not be allowed. Not sure what you mean by
> partial though, other than the missing leading zero.
>
> For octal we should definitely enforce the leading zero.
>
>>> +	return undef if $noerr;
>>> +	die "$mode is not a valid file access mode\n";
>>> +    }
>>> +
>>> +    return $mode;
>>> +}
>>> +
>>> +my $dev_desc = {
>>> +    path => {
>>> +	optional => 1,
>>> +	type => 'string',
>>> +	default_key => 1,
>>> +	format => 'pve-lxc-dev-string',
>>> +	format_description => 'Path',
>>> +	description => 'Device to pass through to the container',
>>> +	verbose_description => 'Path to the device to pass through to the container',
>>> +    },
>>> +    mode => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	format => 'file-access-mode-string',
> ... this should be `type => 'string'` (integer just doesn't enforce a
> format),
> the `format` should be dropped (including the registered sub above), and
> instead use 'pattern' as:
>
>      pattern => '0[0-7]*',
>
> JSONSchema anchors the pattern, so no need to include '^' and '$',
> although I also wouldn't mind using
>
>      pattern => qr/^0[0-7]*$/,
>
>>> +	format_description => 'Octal access mode',
>>> +	description => 'Access mode to be set on the device node',
>>> +    },
>>> +    uid => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	description => 'User ID to be assigned to the device node',
>>> +    },
>>> +    gid => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	description => 'Group ID to be assigned to the device node',
> Add `minimum => 0`, to both uid and gid.