* [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO @ 2024-09-02 13:57 Anatoly Burakov 2024-09-03 9:11 ` Burakov, Anatoly 2024-11-26 15:02 ` [PATCH v2 " Anatoly Burakov 0 siblings, 2 replies; 5+ messages in thread From: Anatoly Burakov @ 2024-09-02 13:57 UTC (permalink / raw) To: dev, Robin Jarry 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO 2024-09-02 13:57 [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO Anatoly Burakov @ 2024-09-03 9:11 ` Burakov, Anatoly 2024-11-26 15:02 ` [PATCH v2 " Anatoly Burakov 1 sibling, 0 replies; 5+ messages in thread From: Burakov, Anatoly @ 2024-09-03 9:11 UTC (permalink / raw) To: dev, Robin Jarry 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO 2024-09-02 13:57 [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO Anatoly Burakov 2024-09-03 9:11 ` Burakov, Anatoly @ 2024-11-26 15:02 ` Anatoly Burakov 2024-11-26 15:24 ` Bruce Richardson 2024-11-26 16:15 ` Robin Jarry 1 sibling, 2 replies; 5+ messages in thread From: Anatoly Burakov @ 2024-11-26 15:02 UTC (permalink / raw) To: dev, Robin Jarry 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO 2024-11-26 15:02 ` [PATCH v2 " Anatoly Burakov @ 2024-11-26 15:24 ` Bruce Richardson 2024-11-26 16:15 ` Robin Jarry 1 sibling, 0 replies; 5+ messages in thread From: Bruce Richardson @ 2024-11-26 15:24 UTC (permalink / raw) To: Anatoly Burakov; +Cc: dev, Robin Jarry 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO 2024-11-26 15:02 ` [PATCH v2 " Anatoly Burakov 2024-11-26 15:24 ` Bruce Richardson @ 2024-11-26 16:15 ` Robin Jarry 1 sibling, 0 replies; 5+ messages in thread From: Robin Jarry @ 2024-11-26 16:15 UTC (permalink / raw) To: Anatoly Burakov, dev 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-26 16:15 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-02 13:57 [PATCH v1 1/1] usertools/devbind: allow changing UID/GID for VFIO Anatoly Burakov 2024-09-03 9:11 ` Burakov, Anatoly 2024-11-26 15:02 ` [PATCH v2 " Anatoly Burakov 2024-11-26 15:24 ` Bruce Richardson 2024-11-26 16:15 ` Robin Jarry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).