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

["Robin Jarry" <rjarry@redhat.com>]

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.