[PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO

[PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO

[Anatoly Burakov]

Currently, when binding a device to VFIO, the UID/GID for the device will
always stay as system default (`root`). Yet, when running DPDK as non-root
user, one has to change the UID/GID of the device to match the user's
UID/GID to use the device.

This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
the device when binding it to VFIO.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 41 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 078e8c387b..37e2b9972d 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -8,6 +8,8 @@
 import subprocess
 import argparse
 import platform
+import grp
+import pwd
 
 from glob import glob
 from os.path import exists, basename
@@ -107,6 +109,8 @@
 b_flag = None
 status_flag = False
 force_flag = False
+vfio_uid = ""
+vfio_gid = ""
 args = []
 
 
@@ -462,6 +466,22 @@ def bind_one(dev_id, driver, force):
                      % (dev_id, filename, err))
 
 
+def own_one(dev_id, uid, gid):
+    """Set the IOMMU group ownership for a device"""
+    # find IOMMU group for a particular device
+    iommu_grp_base_path = os.path.join("/sys/bus/pci/devices", dev_id, "iommu_group")
+    try:
+        iommu_grp = os.path.basename(os.readlink(iommu_grp_base_path))
+        # we found IOMMU group, now find the device
+        dev_path = os.path.join("/dev/vfio", iommu_grp)
+        # set the ownership
+        _uid = pwd.getpwnam(uid).pw_uid if uid else -1
+        _gid = grp.getgrnam(gid).gr_gid if gid else -1
+        os.chown(dev_path, _uid, _gid)
+    except OSError as err:
+        sys.exit(f"Error: failed to read IOMMU group for {dev_id}: {err}")
+
+
 def unbind_all(dev_list, force=False):
     """Unbind method, takes a list of device locations"""
 
@@ -482,7 +502,7 @@ def unbind_all(dev_list, force=False):
         unbind_one(d, force)
 
 
-def bind_all(dev_list, driver, force=False):
+def bind_all(dev_list, driver, uid, gid, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
@@ -510,6 +530,9 @@ def bind_all(dev_list, driver, force=False):
 
     for d in dev_list:
         bind_one(d, driver, force)
+        # if we're binding to vfio-pci, set the IOMMU user/group ownership if one was specified
+        if driver == "vfio-pci" and (uid or gid):
+            own_one(d, uid, gid)
 
     # For kernels < 3.15 when binding devices to a generic driver
     # (i.e. one that doesn't have a PCI ID table) using new_id, some devices
@@ -662,6 +685,8 @@ def parse_args():
     global status_dev
     global force_flag
     global args
+    global vfio_uid
+    global vfio_gid
 
     parser = argparse.ArgumentParser(
         description='Utility to bind and unbind devices from Linux kernel',
@@ -707,6 +732,12 @@ def parse_args():
         '--unbind',
         action='store_true',
         help="Unbind a device (equivalent to \"-b none\")")
+    parser.add_argument(
+        "-U", "--uid", help="For VFIO, specify the UID to set IOMMU group ownership"
+    )
+    parser.add_argument(
+        "-G", "--gid", help="For VFIO, specify the GID to set IOMMU group ownership"
+    )
     parser.add_argument(
         '--force',
         action='store_true',
@@ -737,6 +768,10 @@ def parse_args():
         b_flag = opt.bind
     elif opt.unbind:
         b_flag = "none"
+    if opt.uid:
+        vfio_uid = opt.uid
+    if opt.gid:
+        vfio_gid = opt.gid
     args = opt.devices
 
     if not b_flag and not status_flag:
@@ -764,11 +799,13 @@ def do_arg_actions():
     global status_flag
     global force_flag
     global args
+    global vfio_uid
+    global vfio_gid
 
     if b_flag in ["none", "None"]:
         unbind_all(args, force_flag)
     elif b_flag is not None:
-        bind_all(args, b_flag, force_flag)
+        bind_all(args, b_flag, vfio_uid, vfio_gid, force_flag)
     if status_flag:
         if b_flag is not None:
             clear_data()
-- 
2.43.5

Re: [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO

[Burakov, Anatoly]

On 9/2/2024 3:57 PM, Anatoly Burakov wrote:
> Currently, when binding a device to VFIO, the UID/GID for the device will
> always stay as system default (`root`). Yet, when running DPDK as non-root
> user, one has to change the UID/GID of the device to match the user's
> UID/GID to use the device.
> 
> This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
> the device when binding it to VFIO.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

<snip>

>   
> +def own_one(dev_id, uid, gid):
> +    """Set the IOMMU group ownership for a device"""
> +    # find IOMMU group for a particular device
> +    iommu_grp_base_path = os.path.join("/sys/bus/pci/devices", dev_id, "iommu_group")
> +    try:
> +        iommu_grp = os.path.basename(os.readlink(iommu_grp_base_path))
> +        # we found IOMMU group, now find the device
> +        dev_path = os.path.join("/dev/vfio", iommu_grp)
> +        # set the ownership
> +        _uid = pwd.getpwnam(uid).pw_uid if uid else -1
> +        _gid = grp.getgrnam(gid).gr_gid if gid else -1
> +        os.chown(dev_path, _uid, _gid)
> +    except OSError as err:
> +        sys.exit(f"Error: failed to read IOMMU group for {dev_id}: {err}")

On another thought, perhaps sys.exit() here is a bit too drastic... Will 
replace with error message in v2

-- 
Thanks,
Anatoly

[PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO

[Anatoly Burakov]

Currently, when binding a device to VFIO, the UID/GID for the device will
always stay as system default (`root`). Yet, when running DPDK as non-root
user, one has to change the UID/GID of the device to match the user's
UID/GID to use the device.

This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
the device when binding it to VFIO.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v1 -> v2:
    - Replaced hard exit with an error printout

 usertools/dpdk-devbind.py | 41 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index f2a2a9a12f..496d0e90e8 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -8,6 +8,8 @@
 import subprocess
 import argparse
 import platform
+import grp
+import pwd
 
 from glob import glob
 from os.path import exists, basename
@@ -108,6 +110,8 @@
 status_flag = False
 force_flag = False
 noiommu_flag = False
+vfio_uid = ""
+vfio_gid = ""
 args = []
 
 
@@ -463,6 +467,22 @@ def bind_one(dev_id, driver, force):
                      % (dev_id, filename, err))
 
 
+def own_one(dev_id, uid, gid):
+    """Set the IOMMU group ownership for a device"""
+    # find IOMMU group for a particular device
+    iommu_grp_base_path = os.path.join("/sys/bus/pci/devices", dev_id, "iommu_group")
+    try:
+        iommu_grp = os.path.basename(os.readlink(iommu_grp_base_path))
+        # we found IOMMU group, now find the device
+        dev_path = os.path.join("/dev/vfio", iommu_grp)
+        # set the ownership
+        _uid = pwd.getpwnam(uid).pw_uid if uid else -1
+        _gid = grp.getgrnam(gid).gr_gid if gid else -1
+        os.chown(dev_path, _uid, _gid)
+    except OSError as err:
+        print(f"Error: failed to read IOMMU group for {dev_id}: {err}")
+
+
 def unbind_all(dev_list, force=False):
     """Unbind method, takes a list of device locations"""
 
@@ -512,7 +532,7 @@ def check_noiommu_mode():
     print("Warning: enabling unsafe no IOMMU mode for VFIO drivers")
 
 
-def bind_all(dev_list, driver, force=False):
+def bind_all(dev_list, driver, uid, gid, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
@@ -544,6 +564,9 @@ def bind_all(dev_list, driver, force=False):
 
     for d in dev_list:
         bind_one(d, driver, force)
+        # if we're binding to vfio-pci, set the IOMMU user/group ownership if one was specified
+        if driver == "vfio-pci" and (uid or gid):
+            own_one(d, uid, gid)
 
     # For kernels < 3.15 when binding devices to a generic driver
     # (i.e. one that doesn't have a PCI ID table) using new_id, some devices
@@ -697,6 +720,8 @@ def parse_args():
     global force_flag
     global noiommu_flag
     global args
+    global vfio_uid
+    global vfio_gid
 
     parser = argparse.ArgumentParser(
         description='Utility to bind and unbind devices from Linux kernel',
@@ -746,6 +771,12 @@ def parse_args():
         '--noiommu-mode',
         action='store_true',
         help="If IOMMU is not available, enable no IOMMU mode for VFIO drivers")
+    parser.add_argument(
+        "-U", "--uid", help="For VFIO, specify the UID to set IOMMU group ownership"
+    )
+    parser.add_argument(
+        "-G", "--gid", help="For VFIO, specify the GID to set IOMMU group ownership"
+    )
     parser.add_argument(
         '--force',
         action='store_true',
@@ -778,6 +809,10 @@ def parse_args():
         b_flag = opt.bind
     elif opt.unbind:
         b_flag = "none"
+    if opt.uid:
+        vfio_uid = opt.uid
+    if opt.gid:
+        vfio_gid = opt.gid
     args = opt.devices
 
     if not b_flag and not status_flag:
@@ -805,11 +840,13 @@ def do_arg_actions():
     global status_flag
     global force_flag
     global args
+    global vfio_uid
+    global vfio_gid
 
     if b_flag in ["none", "None"]:
         unbind_all(args, force_flag)
     elif b_flag is not None:
-        bind_all(args, b_flag, force_flag)
+        bind_all(args, b_flag, vfio_uid, vfio_gid, force_flag)
     if status_flag:
         if b_flag is not None:
             clear_data()
-- 
2.43.5

Re: [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO

[Bruce Richardson]

On Tue, Nov 26, 2024 at 03:02:38PM +0000, Anatoly Burakov wrote:
> Currently, when binding a device to VFIO, the UID/GID for the device will
> always stay as system default (`root`). Yet, when running DPDK as non-root
> user, one has to change the UID/GID of the device to match the user's
> UID/GID to use the device.
> 
> This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
> the device when binding it to VFIO.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO

[Robin Jarry]

Hi Anatoly,

Anatoly Burakov, Nov 26, 2024 at 16:02:
> Currently, when binding a device to VFIO, the UID/GID for the device will
> always stay as system default (`root`). Yet, when running DPDK as non-root
> user, one has to change the UID/GID of the device to match the user's
> UID/GID to use the device.
>
> This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
> the device when binding it to VFIO.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>
> Notes:
>     v1 -> v2:
>     - Replaced hard exit with an error printout

Sorry I had missed that particular detail.

I don't think this should only print a warning. Otherwise, the user has 
no way to detect if the operation failed.

>  usertools/dpdk-devbind.py | 41 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
> index f2a2a9a12f..496d0e90e8 100755
> --- a/usertools/dpdk-devbind.py
> +++ b/usertools/dpdk-devbind.py
> @@ -8,6 +8,8 @@
>  import subprocess
>  import argparse
>  import platform
> +import grp
> +import pwd

We may already be past this but could you try to sort these imports 
alphabetically?

>  
>  from glob import glob
>  from os.path import exists, basename
> @@ -108,6 +110,8 @@
>  status_flag = False
>  force_flag = False
>  noiommu_flag = False
> +vfio_uid = ""
> +vfio_gid = ""

These are supposed to be integers. Initialize them to -1.

>  args = []
>  
>  
> @@ -463,6 +467,22 @@ def bind_one(dev_id, driver, force):
>                       % (dev_id, filename, err))
>  
>  
> +def own_one(dev_id, uid, gid):
> +    """Set the IOMMU group ownership for a device"""
> +    # find IOMMU group for a particular device
> +    iommu_grp_base_path = os.path.join("/sys/bus/pci/devices", dev_id, "iommu_group")
> +    try:
> +        iommu_grp = os.path.basename(os.readlink(iommu_grp_base_path))
> +        # we found IOMMU group, now find the device
> +        dev_path = os.path.join("/dev/vfio", iommu_grp)
> +        # set the ownership
> +        _uid = pwd.getpwnam(uid).pw_uid if uid else -1
> +        _gid = grp.getgrnam(gid).gr_gid if gid else -1

The validity of these values should be checked when parsing command line 
arguments.

> +        os.chown(dev_path, _uid, _gid)
> +    except OSError as err:
> +        print(f"Error: failed to read IOMMU group for {dev_id}: {err}")

Remove the try/except block and let the error bubble up the stack. This 
probably does not require a dedicated function. Moreover, the name 
own_one() is ambiguous.

> +
> +
>  def unbind_all(dev_list, force=False):
>      """Unbind method, takes a list of device locations"""
>  
> @@ -512,7 +532,7 @@ def check_noiommu_mode():
>      print("Warning: enabling unsafe no IOMMU mode for VFIO drivers")
>  
>  
> -def bind_all(dev_list, driver, force=False):
> +def bind_all(dev_list, driver, uid, gid, force=False):

Not required. These are global variables.

>      """Bind method, takes a list of device locations"""
>      global devices
>  
> @@ -544,6 +564,9 @@ def bind_all(dev_list, driver, force=False):
>  
>      for d in dev_list:
>          bind_one(d, driver, force)
> +        # if we're binding to vfio-pci, set the IOMMU user/group ownership if one was specified
> +        if driver == "vfio-pci" and (uid or gid):

           if driver == "vfio-pci" and (vfio_uid != -1 or vfio_gid != -1):

> +            own_one(d, uid, gid)

It will be better to store the chmod code path here:

               iommu_grp = os.path.join("/sys/bus/pci/devices", d, "iommu_group")
               iommu_grp = os.path.basename(os.readlink(iommu_grp))
               os.chown(os.path.join("/dev/vfio", iommu_grp), vfio_uid, vfio_gid)

>  
>      # For kernels < 3.15 when binding devices to a generic driver
>      # (i.e. one that doesn't have a PCI ID table) using new_id, some devices
> @@ -697,6 +720,8 @@ def parse_args():
>      global force_flag
>      global noiommu_flag
>      global args
> +    global vfio_uid
> +    global vfio_gid
>  
>      parser = argparse.ArgumentParser(
>          description='Utility to bind and unbind devices from Linux kernel',
> @@ -746,6 +771,12 @@ def parse_args():
>          '--noiommu-mode',
>          action='store_true',
>          help="If IOMMU is not available, enable no IOMMU mode for VFIO drivers")
> +    parser.add_argument(
> +        "-U", "--uid", help="For VFIO, specify the UID to set IOMMU group ownership"

In order to fail early if an invalid user name is passed, add these two 
lines:

           type=lambda u: pwd.getpwnam(u).pw_uid,
           default=-1,

> +    )
> +    parser.add_argument(
> +        "-G", "--gid", help="For VFIO, specify the GID to set IOMMU group ownership"

In order to fail early if an invalid group name is passed, add these two 
lines:

           type=lambda g: grp.getgrnam(g).gr_gid,
           default=-1,

> +    )
>      parser.add_argument(
>          '--force',
>          action='store_true',
> @@ -778,6 +809,10 @@ def parse_args():
>          b_flag = opt.bind
>      elif opt.unbind:
>          b_flag = "none"
> +    if opt.uid:
> +        vfio_uid = opt.uid
> +    if opt.gid:
> +        vfio_gid = opt.gid

No need for ifs here, the default values are set to -1 which means: "use 
default" for os.chmod().

       vfio_uid = opt.uid
       vfio_gid = opt.gid

>      args = opt.devices
>  
>      if not b_flag and not status_flag:
> @@ -805,11 +840,13 @@ def do_arg_actions():
>      global status_flag
>      global force_flag
>      global args
> +    global vfio_uid
> +    global vfio_gid

The global keyword is not required here.

>  
>      if b_flag in ["none", "None"]:
>          unbind_all(args, force_flag)
>      elif b_flag is not None:
> -        bind_all(args, b_flag, force_flag)
> +        bind_all(args, b_flag, vfio_uid, vfio_gid, force_flag)

Not required. These are global variables.

>      if status_flag:
>          if b_flag is not None:
>              clear_data()
> -- 
> 2.43.5

Thanks.