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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 11/26/2024 5:15 PM, Robin Jarry wrote:
> 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.

Sure, I'll change it back.

>>  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.

Actually, the pwd.getpwnam() accepts strings not integers, but yeah, 
technically these are supposed to be integers. I'll change that.

> 
>>  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.

Sure, I'll move this check somewhere close to init.

> 
>> +        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.

We do the same thing for other errors (e.g. in bind_one) so I'm not sure 
if we want to let it bubble up the stack - we don't catch any exceptions 
anywhere up the stack. Current implementation, however deficient from 
error handling point of view, is consistent with the rest of the script.

>>      # 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,
> 

Guido doesn't like lambdas :D


-- 
Thanks,
Anatoly