* [RFC 0/2] nfp driver fixes
@ 2018-04-12 22:22 Aaron Conole
2018-04-12 22:22 ` [RFC 1/2] nfp: unlink the appropriate lock file Aaron Conole
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Aaron Conole @ 2018-04-12 22:22 UTC (permalink / raw)
To: dev; +Cc: Alejandro Lucero
Two fixes, one which is fairly obvious (1/2), the other which may
allow support of non-root users. These patches are only compile tested
which is why they are submitted as RFC. After a proper test, will
resubmit them as PATCH (with any suggested / recommended changes).
Aaron Conole (2):
nfp: unlink the appropriate lock file
nfp: allow for non-root user
drivers/net/nfp/nfp_nfpu.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFC 1/2] nfp: unlink the appropriate lock file 2018-04-12 22:22 [RFC 0/2] nfp driver fixes Aaron Conole @ 2018-04-12 22:22 ` Aaron Conole 2018-04-13 7:31 ` Alejandro Lucero 2018-04-12 22:22 ` [RFC 2/2] nfp: allow for non-root user Aaron Conole 2018-04-13 7:26 ` [RFC 0/2] nfp driver fixes Alejandro Lucero 2 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-12 22:22 UTC (permalink / raw) To: dev; +Cc: Alejandro Lucero The nfpu_close needs to unlink the lock file associated with the nfp descriptor, not lock file 0. Fixes: d12206e00590 ("net/nfp: add NSP user space interface") Cc: stable@dpdk.org Signed-off-by: Aaron Conole <aconole@redhat.com> --- drivers/net/nfp/nfp_nfpu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c index f11afef35..2ed985ff4 100644 --- a/drivers/net/nfp/nfp_nfpu.c +++ b/drivers/net/nfp/nfp_nfpu.c @@ -101,8 +101,12 @@ nfpu_open(struct rte_pci_device *pci_dev, nfpu_desc_t *desc, int nfp) int nfpu_close(nfpu_desc_t *desc) { + char lockname[30]; + rte_free(desc->nspu); close(desc->lock); - unlink("/var/lock/nfp0"); + + snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); + unlink(lockname); return 0; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC 1/2] nfp: unlink the appropriate lock file 2018-04-12 22:22 ` [RFC 1/2] nfp: unlink the appropriate lock file Aaron Conole @ 2018-04-13 7:31 ` Alejandro Lucero 2018-04-13 13:24 ` Aaron Conole 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-13 7:31 UTC (permalink / raw) To: Aaron Conole; +Cc: dev Although the patch is correct, the truth is there was not option for locks but "nfp0", because the PMD did not support more than one NFP card. After commit with CPP support, the PMD can support more than one NFP card, but this interface is not supported anymore and this file was removed. is this patch coming from some code revision or from a problem you found? On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > The nfpu_close needs to unlink the lock file associated with the > nfp descriptor, not lock file 0. > > Fixes: d12206e00590 ("net/nfp: add NSP user space interface") > > Cc: stable@dpdk.org > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > drivers/net/nfp/nfp_nfpu.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > index f11afef35..2ed985ff4 100644 > --- a/drivers/net/nfp/nfp_nfpu.c > +++ b/drivers/net/nfp/nfp_nfpu.c > @@ -101,8 +101,12 @@ nfpu_open(struct rte_pci_device *pci_dev, nfpu_desc_t > *desc, int nfp) > int > nfpu_close(nfpu_desc_t *desc) > { > + char lockname[30]; > + > rte_free(desc->nspu); > close(desc->lock); > - unlink("/var/lock/nfp0"); > + > + snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > + unlink(lockname); > return 0; > } > -- > 2.14.3 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/2] nfp: unlink the appropriate lock file 2018-04-13 7:31 ` Alejandro Lucero @ 2018-04-13 13:24 ` Aaron Conole 0 siblings, 0 replies; 22+ messages in thread From: Aaron Conole @ 2018-04-13 13:24 UTC (permalink / raw) To: Alejandro Lucero; +Cc: dev Alejandro Lucero <alejandro.lucero@netronome.com> writes: > Although the patch is correct, the truth is there was not option for locks but "nfp0", because > the PMD did not support more than one NFP card. > > After commit with CPP support, the PMD can support more than one NFP card, but this > interface is not supported anymore and this file was removed. > > is this patch coming from some code revision or from a problem you found? Just a code review - we were reported a problem initializing the driver, and in looking at the lock/unlock code, it seems that it was always releasing the nfp0 lock. Good to know that in practice it likely wouldn't cause real harm. I intend to resubmit this as is. Thanks, Alejandro! > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > > The nfpu_close needs to unlink the lock file associated with the > nfp descriptor, not lock file 0. > > Fixes: d12206e00590 ("net/nfp: add NSP user space interface") > > Cc: stable@dpdk.org > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > drivers/net/nfp/nfp_nfpu.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > index f11afef35..2ed985ff4 100644 > --- a/drivers/net/nfp/nfp_nfpu.c > +++ b/drivers/net/nfp/nfp_nfpu.c > @@ -101,8 +101,12 @@ nfpu_open(struct rte_pci_device *pci_dev, nfpu_desc_t *desc, > int nfp) > int > nfpu_close(nfpu_desc_t *desc) > { > + char lockname[30]; > + > rte_free(desc->nspu); > close(desc->lock); > - unlink("/var/lock/nfp0"); > + > + snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > + unlink(lockname); > return 0; > } > -- > 2.14.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 2/2] nfp: allow for non-root user 2018-04-12 22:22 [RFC 0/2] nfp driver fixes Aaron Conole 2018-04-12 22:22 ` [RFC 1/2] nfp: unlink the appropriate lock file Aaron Conole @ 2018-04-12 22:22 ` Aaron Conole 2018-04-13 7:37 ` Alejandro Lucero 2018-04-13 7:26 ` [RFC 0/2] nfp driver fixes Alejandro Lucero 2 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-12 22:22 UTC (permalink / raw) To: dev; +Cc: Alejandro Lucero Currently, the nfp lock files are taken from the global lock file location, which will work when the user is running as root. However, some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) run as a non-root user. Signed-off-by: Aaron Conole <aconole@redhat.com> --- drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c index 2ed985ff4..ae2e07220 100644 --- a/drivers/net/nfp/nfp_nfpu.c +++ b/drivers/net/nfp/nfp_nfpu.c @@ -18,6 +18,22 @@ #define NFP_CFG_EXP_BAR 7 #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" + +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ +static void +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) +{ + const char *dir = "/var/lock"; + const char *home_dir = getenv("HOME"); + + if (getuid() != 0 && home_dir != NULL) + dir = home_dir; + + /* use current prefix as file path */ + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, + desc->nfp); +} /* There could be other NFP userspace tools using the NSP interface. * Make sure there is no other process using it and locking the access for @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) struct flock lock; char lockname[30]; - memset(&lock, 0, sizeof(lock)); - - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH */ desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) rte_free(desc->nspu); close(desc->lock); - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); - unlink(lockname); + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); return 0; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-12 22:22 ` [RFC 2/2] nfp: allow for non-root user Aaron Conole @ 2018-04-13 7:37 ` Alejandro Lucero 2018-04-13 13:31 ` Aaron Conole 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-13 7:37 UTC (permalink / raw) To: Aaron Conole; +Cc: dev Again, this patch is correct, but because NFP PMD needs to access /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and these files have just read/write accesses for root, I do not know if this is really necessary. Being honest, I have not used a DPDK app with NFP PMD and not being root. Does it work with non-root users and other PMDs with same requirements regarding sysfs resource files? On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > Currently, the nfp lock files are taken from the global lock file > location, which will work when the user is running as root. However, > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > run as a non-root user. > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > index 2ed985ff4..ae2e07220 100644 > --- a/drivers/net/nfp/nfp_nfpu.c > +++ b/drivers/net/nfp/nfp_nfpu.c > @@ -18,6 +18,22 @@ > #define NFP_CFG_EXP_BAR 7 > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > + > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > +static void > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > +{ > + const char *dir = "/var/lock"; > + const char *home_dir = getenv("HOME"); > + > + if (getuid() != 0 && home_dir != NULL) > + dir = home_dir; > + > + /* use current prefix as file path */ > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > + desc->nfp); > +} > > /* There could be other NFP userspace tools using the NSP interface. > * Make sure there is no other process using it and locking the access for > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > struct flock lock; > char lockname[30]; > > - memset(&lock, 0, sizeof(lock)); > - > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH > */ > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > rte_free(desc->nspu); > close(desc->lock); > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > - unlink(lockname); > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > return 0; > } > -- > 2.14.3 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-13 7:37 ` Alejandro Lucero @ 2018-04-13 13:31 ` Aaron Conole 2018-04-13 15:31 ` Alejandro Lucero 0 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-13 13:31 UTC (permalink / raw) To: Alejandro Lucero; +Cc: dev, Adrien Mazarguil Alejandro Lucero <alejandro.lucero@netronome.com> writes: > Again, this patch is correct, but because NFP PMD needs to access > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and these files have just > read/write accesses for root, I do not know if this is really necessary. > > Being honest, I have not used a DPDK app with NFP PMD and not being root. Does it work > with non-root users and other PMDs with same requirements regarding sysfs resource files? We do run as non-root user definitely with Intel PMDs. I'm not very sure about other vendors, but I think mlx pmd runs as non-root user (and it was modified to move off of sysfs for that reason[1]). I'll continue to push for more information from the testing side to find out though. [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > > Currently, the nfp lock files are taken from the global lock file > location, which will work when the user is running as root. However, > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > run as a non-root user. > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > index 2ed985ff4..ae2e07220 100644 > --- a/drivers/net/nfp/nfp_nfpu.c > +++ b/drivers/net/nfp/nfp_nfpu.c > @@ -18,6 +18,22 @@ > #define NFP_CFG_EXP_BAR 7 > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > + > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > +static void > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > +{ > + const char *dir = "/var/lock"; > + const char *home_dir = getenv("HOME"); > + > + if (getuid() != 0 && home_dir != NULL) > + dir = home_dir; > + > + /* use current prefix as file path */ > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > + desc->nfp); > +} > > /* There could be other NFP userspace tools using the NSP interface. > * Make sure there is no other process using it and locking the access for > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > struct flock lock; > char lockname[30]; > > - memset(&lock, 0, sizeof(lock)); > - > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH */ > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > rte_free(desc->nspu); > close(desc->lock); > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > - unlink(lockname); > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > return 0; > } > -- > 2.14.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-13 13:31 ` Aaron Conole @ 2018-04-13 15:31 ` Alejandro Lucero 2018-04-17 15:44 ` Alejandro Lucero 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-13 15:31 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Adrien Mazarguil On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> wrote: > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > Again, this patch is correct, but because NFP PMD needs to access > > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and these > files have just > > read/write accesses for root, I do not know if this is really necessary. > > > > Being honest, I have not used a DPDK app with NFP PMD and not being > root. Does it work > > with non-root users and other PMDs with same requirements regarding > sysfs resource files? > > We do run as non-root user definitely with Intel PMDs. > > I'm not very sure about other vendors, but I think mlx pmd runs as > non-root user (and it was modified to move off of sysfs for that > reason[1]). > > It is possible to not rely on sysfs resource files if device is attached to VFIO, but I think that is a must with UIO. > I'll continue to push for more information from the testing side to find > out though. > > [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> > wrote: > > > > Currently, the nfp lock files are taken from the global lock file > > location, which will work when the user is running as root. However, > > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > > run as a non-root user. > > > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > --- > > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > > index 2ed985ff4..ae2e07220 100644 > > --- a/drivers/net/nfp/nfp_nfpu.c > > +++ b/drivers/net/nfp/nfp_nfpu.c > > @@ -18,6 +18,22 @@ > > #define NFP_CFG_EXP_BAR 7 > > > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > > + > > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > > +static void > > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > > +{ > > + const char *dir = "/var/lock"; > > + const char *home_dir = getenv("HOME"); > > + > > + if (getuid() != 0 && home_dir != NULL) > > + dir = home_dir; > > + > > + /* use current prefix as file path */ > > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > > + desc->nfp); > > +} > > > > /* There could be other NFP userspace tools using the NSP interface. > > * Make sure there is no other process using it and locking the access > for > > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > > struct flock lock; > > char lockname[30]; > > > > - memset(&lock, 0, sizeof(lock)); > > - > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | > S_IWOTH */ > > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > > rte_free(desc->nspu); > > close(desc->lock); > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > - unlink(lockname); > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > return 0; > > } > > -- > > 2.14.3 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-13 15:31 ` Alejandro Lucero @ 2018-04-17 15:44 ` Alejandro Lucero 2018-04-17 15:54 ` Alejandro Lucero 2018-04-17 15:54 ` Thomas Monjalon 0 siblings, 2 replies; 22+ messages in thread From: Alejandro Lucero @ 2018-04-17 15:44 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon I have seen that VFIO also requires explicitly to set the right permissions for non-root users to VFIO groups under /dev/vfio. I assume then that running OVS or other DPDK apps as non-root is possible, although requiring those explicit permissions changes, and therefore this patch is necessary. Adding stable@ and Thomas for discussing how can this be added to stable DPDK versions even if this is not going to be a patch for current DPDK version. Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero < alejandro.lucero@netronome.com> wrote: > > > On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> wrote: > >> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >> >> > Again, this patch is correct, but because NFP PMD needs to access >> > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and >> these files have just >> > read/write accesses for root, I do not know if this is really necessary. >> > >> > Being honest, I have not used a DPDK app with NFP PMD and not being >> root. Does it work >> > with non-root users and other PMDs with same requirements regarding >> sysfs resource files? >> >> We do run as non-root user definitely with Intel PMDs. >> >> I'm not very sure about other vendors, but I think mlx pmd runs as >> non-root user (and it was modified to move off of sysfs for that >> reason[1]). >> >> > It is possible to not rely on sysfs resource files if device is attached > to VFIO, but I think that is a must with UIO. > > > >> I'll continue to push for more information from the testing side to find >> out though. >> >> [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html >> >> > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> >> wrote: >> > >> > Currently, the nfp lock files are taken from the global lock file >> > location, which will work when the user is running as root. However, >> > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) >> > run as a non-root user. >> > >> > Signed-off-by: Aaron Conole <aconole@redhat.com> >> > --- >> > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- >> > 1 file changed, 18 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c >> > index 2ed985ff4..ae2e07220 100644 >> > --- a/drivers/net/nfp/nfp_nfpu.c >> > +++ b/drivers/net/nfp/nfp_nfpu.c >> > @@ -18,6 +18,22 @@ >> > #define NFP_CFG_EXP_BAR 7 >> > >> > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 >> > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" >> > + >> > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ >> > +static void >> > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) >> > +{ >> > + const char *dir = "/var/lock"; >> > + const char *home_dir = getenv("HOME"); >> > + >> > + if (getuid() != 0 && home_dir != NULL) >> > + dir = home_dir; >> > + >> > + /* use current prefix as file path */ >> > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, >> > + desc->nfp); >> > +} >> > >> > /* There could be other NFP userspace tools using the NSP interface. >> > * Make sure there is no other process using it and locking the >> access for >> > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) >> > struct flock lock; >> > char lockname[30]; >> > >> > - memset(&lock, 0, sizeof(lock)); >> > - >> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >> desc->nfp); >> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >> > >> > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | >> S_IWOTH */ >> > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); >> > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) >> > rte_free(desc->nspu); >> > close(desc->lock); >> > >> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >> desc->nfp); >> > - unlink(lockname); >> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >> > return 0; >> > } >> > -- >> > 2.14.3 >> > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 15:44 ` Alejandro Lucero @ 2018-04-17 15:54 ` Alejandro Lucero 2018-04-17 19:19 ` Aaron Conole 2018-04-17 15:54 ` Thomas Monjalon 1 sibling, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-17 15:54 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon I was just wondering, if device device PCI sysfs resource files or VFIO group /dev files require to change permissions for non-root users, does it not make sense to adjust also /var/lock in the system? On Tue, Apr 17, 2018 at 4:44 PM, Alejandro Lucero < alejandro.lucero@netronome.com> wrote: > I have seen that VFIO also requires explicitly to set the right > permissions for non-root users to VFIO groups under /dev/vfio. > > I assume then that running OVS or other DPDK apps as non-root is possible, > although requiring those explicit permissions changes, and therefore this > patch is necessary. > > Adding stable@ and Thomas for discussing how can this be added to stable > DPDK versions even if this is not going to be a patch for current DPDK > version. > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > > On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero < > alejandro.lucero@netronome.com> wrote: > >> >> >> On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> wrote: >> >>> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >>> >>> > Again, this patch is correct, but because NFP PMD needs to access >>> > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and >>> these files have just >>> > read/write accesses for root, I do not know if this is really >>> necessary. >>> > >>> > Being honest, I have not used a DPDK app with NFP PMD and not being >>> root. Does it work >>> > with non-root users and other PMDs with same requirements regarding >>> sysfs resource files? >>> >>> We do run as non-root user definitely with Intel PMDs. >>> >>> I'm not very sure about other vendors, but I think mlx pmd runs as >>> non-root user (and it was modified to move off of sysfs for that >>> reason[1]). >>> >>> >> It is possible to not rely on sysfs resource files if device is attached >> to VFIO, but I think that is a must with UIO. >> >> >> >>> I'll continue to push for more information from the testing side to find >>> out though. >>> >>> [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html >>> >>> > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> >>> wrote: >>> > >>> > Currently, the nfp lock files are taken from the global lock file >>> > location, which will work when the user is running as root. However, >>> > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) >>> > run as a non-root user. >>> > >>> > Signed-off-by: Aaron Conole <aconole@redhat.com> >>> > --- >>> > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- >>> > 1 file changed, 18 insertions(+), 5 deletions(-) >>> > >>> > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c >>> > index 2ed985ff4..ae2e07220 100644 >>> > --- a/drivers/net/nfp/nfp_nfpu.c >>> > +++ b/drivers/net/nfp/nfp_nfpu.c >>> > @@ -18,6 +18,22 @@ >>> > #define NFP_CFG_EXP_BAR 7 >>> > >>> > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 >>> > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" >>> > + >>> > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ >>> > +static void >>> > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) >>> > +{ >>> > + const char *dir = "/var/lock"; >>> > + const char *home_dir = getenv("HOME"); >>> > + >>> > + if (getuid() != 0 && home_dir != NULL) >>> > + dir = home_dir; >>> > + >>> > + /* use current prefix as file path */ >>> > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, >>> > + desc->nfp); >>> > +} >>> > >>> > /* There could be other NFP userspace tools using the NSP interface. >>> > * Make sure there is no other process using it and locking the >>> access for >>> > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) >>> > struct flock lock; >>> > char lockname[30]; >>> > >>> > - memset(&lock, 0, sizeof(lock)); >>> > - >>> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >>> desc->nfp); >>> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >>> > >>> > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | >>> S_IWOTH */ >>> > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); >>> > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) >>> > rte_free(desc->nspu); >>> > close(desc->lock); >>> > >>> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >>> desc->nfp); >>> > - unlink(lockname); >>> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >>> > return 0; >>> > } >>> > -- >>> > 2.14.3 >>> >> >> > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 15:54 ` Alejandro Lucero @ 2018-04-17 19:19 ` Aaron Conole 2018-04-18 10:53 ` Alejandro Lucero 0 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-17 19:19 UTC (permalink / raw) To: Alejandro Lucero; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon Alejandro Lucero <alejandro.lucero@netronome.com> writes: > I was just wondering, if device device PCI sysfs resource files or VFIO group /dev files require to change > permissions for non-root users, does it not make sense to adjust also /var/lock in the system? For the /dev, we use udev rules - so the correct individual vfio device files get assigned the correct permissions. No such mechanism exists for /var/lock as far as I can tell. Ex. see: https://github.com/openvswitch/ovs/blob/master/rhel/usr_lib_udev_rules.d_91-vfio.rules Maybe something similar exists that we could use to generate the lock file automatically? > On Tue, Apr 17, 2018 at 4:44 PM, Alejandro Lucero <alejandro.lucero@netronome.com> wrote: > > I have seen that VFIO also requires explicitly to set the right permissions for non-root users to VFIO > groups under /dev/vfio. > > I assume then that running OVS or other DPDK apps as non-root is possible, although requiring > those explicit permissions changes, and therefore this patch is necessary. > > Adding stable@ and Thomas for discussing how can this be added to stable DPDK versions even if > this is not going to be a patch for current DPDK version. > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero <alejandro.lucero@netronome.com> wrote: > > On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> wrote: > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > Again, this patch is correct, but because NFP PMD needs to access > > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and these files have > just > > read/write accesses for root, I do not know if this is really necessary. > > > > Being honest, I have not used a DPDK app with NFP PMD and not being root. Does it > work > > with non-root users and other PMDs with same requirements regarding sysfs resource > files? > > We do run as non-root user definitely with Intel PMDs. > > I'm not very sure about other vendors, but I think mlx pmd runs as > non-root user (and it was modified to move off of sysfs for that > reason[1]). > > It is possible to not rely on sysfs resource files if device is attached to VFIO, but I think that is a > must with UIO. > > > I'll continue to push for more information from the testing side to find > out though. > > [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > > > > Currently, the nfp lock files are taken from the global lock file > > location, which will work when the user is running as root. However, > > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > > run as a non-root user. > > > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > --- > > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > > index 2ed985ff4..ae2e07220 100644 > > --- a/drivers/net/nfp/nfp_nfpu.c > > +++ b/drivers/net/nfp/nfp_nfpu.c > > @@ -18,6 +18,22 @@ > > #define NFP_CFG_EXP_BAR 7 > > > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > > + > > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > > +static void > > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > > +{ > > + const char *dir = "/var/lock"; > > + const char *home_dir = getenv("HOME"); > > + > > + if (getuid() != 0 && home_dir != NULL) > > + dir = home_dir; > > + > > + /* use current prefix as file path */ > > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > > + desc->nfp); > > +} > > > > /* There could be other NFP userspace tools using the NSP interface. > > * Make sure there is no other process using it and locking the access for > > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > > struct flock lock; > > char lockname[30]; > > > > - memset(&lock, 0, sizeof(lock)); > > - > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH */ > > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > > rte_free(desc->nspu); > > close(desc->lock); > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > > - unlink(lockname); > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > return 0; > > } > > -- > > 2.14.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 19:19 ` Aaron Conole @ 2018-04-18 10:53 ` Alejandro Lucero 2018-04-18 12:32 ` Aaron Conole 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-18 10:53 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon On Tue, Apr 17, 2018 at 8:19 PM, Aaron Conole <aconole@redhat.com> wrote: > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > I was just wondering, if device device PCI sysfs resource files or VFIO > group /dev files require to change > > permissions for non-root users, does it not make sense to adjust also > /var/lock in the system? > > For the /dev, we use udev rules - so the correct individual vfio device > files get assigned the correct permissions. No such mechanism exists > for /var/lock as far as I can tell. > > Ex. see: > > https://github.com/openvswitch/ovs/blob/master/ > rhel/usr_lib_udev_rules.d_91-vfio.rules > > Maybe something similar exists that we could use to generate the lock > file automatically? > What about /sysfs/bus/pci/device/$PCI_DEV/resource file? Is RH forcing OVS DPDK to only work if the host has IOMMU support? > > > On Tue, Apr 17, 2018 at 4:44 PM, Alejandro Lucero < > alejandro.lucero@netronome.com> wrote: > > > > I have seen that VFIO also requires explicitly to set the right > permissions for non-root users to VFIO > > groups under /dev/vfio. > > > > I assume then that running OVS or other DPDK apps as non-root is > possible, although requiring > > those explicit permissions changes, and therefore this patch is > necessary. > > > > Adding stable@ and Thomas for discussing how can this be added to > stable DPDK versions even if > > this is not going to be a patch for current DPDK version. > > > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > > > On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero < > alejandro.lucero@netronome.com> wrote: > > > > On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> > wrote: > > > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > > > Again, this patch is correct, but because NFP PMD needs to access > > > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and > these files have > > just > > > read/write accesses for root, I do not know if this is really > necessary. > > > > > > Being honest, I have not used a DPDK app with NFP PMD and not being > root. Does it > > work > > > with non-root users and other PMDs with same requirements regarding > sysfs resource > > files? > > > > We do run as non-root user definitely with Intel PMDs. > > > > I'm not very sure about other vendors, but I think mlx pmd runs as > > non-root user (and it was modified to move off of sysfs for that > > reason[1]). > > > > It is possible to not rely on sysfs resource files if device is > attached to VFIO, but I think that is a > > must with UIO. > > > > > > I'll continue to push for more information from the testing side to find > > out though. > > > > [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > > > > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> > wrote: > > > > > > Currently, the nfp lock files are taken from the global lock file > > > location, which will work when the user is running as root. However, > > > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > > > run as a non-root user. > > > > > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > > --- > > > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > > > index 2ed985ff4..ae2e07220 100644 > > > --- a/drivers/net/nfp/nfp_nfpu.c > > > +++ b/drivers/net/nfp/nfp_nfpu.c > > > @@ -18,6 +18,22 @@ > > > #define NFP_CFG_EXP_BAR 7 > > > > > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > > > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > > > + > > > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > > > +static void > > > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > > > +{ > > > + const char *dir = "/var/lock"; > > > + const char *home_dir = getenv("HOME"); > > > + > > > + if (getuid() != 0 && home_dir != NULL) > > > + dir = home_dir; > > > + > > > + /* use current prefix as file path */ > > > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > > > + desc->nfp); > > > +} > > > > > > /* There could be other NFP userspace tools using the NSP interface. > > > * Make sure there is no other process using it and locking the > access for > > > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > > > struct flock lock; > > > char lockname[30]; > > > > > > - memset(&lock, 0, sizeof(lock)); > > > - > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | > S_IWOTH */ > > > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > > > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > > > rte_free(desc->nspu); > > > close(desc->lock); > > > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > > - unlink(lockname); > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > return 0; > > > } > > > -- > > > 2.14.3 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-18 10:53 ` Alejandro Lucero @ 2018-04-18 12:32 ` Aaron Conole 2018-04-19 6:05 ` Alejandro Lucero 0 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-18 12:32 UTC (permalink / raw) To: Alejandro Lucero; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon Alejandro Lucero <alejandro.lucero@netronome.com> writes: > On Tue, Apr 17, 2018 at 8:19 PM, Aaron Conole <aconole@redhat.com> wrote: > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > I was just wondering, if device device PCI sysfs resource files or VFIO group /dev files > require to change > > permissions for non-root users, does it not make sense to adjust also /var/lock in the > system? > > For the /dev, we use udev rules - so the correct individual vfio device > files get assigned the correct permissions. No such mechanism exists > for /var/lock as far as I can tell. > > Ex. see: > > https://github.com/openvswitch/ovs/blob/master/rhel/usr_lib_udev_rules.d_91-vfio.rules > > > Maybe something similar exists that we could use to generate the lock > file automatically? > > What about /sysfs/bus/pci/device/$PCI_DEV/resource file? > > Is RH forcing OVS DPDK to only work if the host has IOMMU support? Yes. > > On Tue, Apr 17, 2018 at 4:44 PM, Alejandro Lucero > <alejandro.lucero@netronome.com> wrote: > > > > I have seen that VFIO also requires explicitly to set the right permissions for non-root > users to VFIO > > groups under /dev/vfio. > > > > I assume then that running OVS or other DPDK apps as non-root is possible, > although requiring > > those explicit permissions changes, and therefore this patch is necessary. > > > > Adding stable@ and Thomas for discussing how can this be added to stable DPDK > versions even if > > this is not going to be a patch for current DPDK version. > > > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > > > On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero > <alejandro.lucero@netronome.com> wrote: > > > > On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> wrote: > > > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > > > Again, this patch is correct, but because NFP PMD needs to access > > > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and these files > have > > just > > > read/write accesses for root, I do not know if this is really necessary. > > > > > > Being honest, I have not used a DPDK app with NFP PMD and not being root. Does > it > > work > > > with non-root users and other PMDs with same requirements regarding sysfs > resource > > files? > > > > We do run as non-root user definitely with Intel PMDs. > > > > I'm not very sure about other vendors, but I think mlx pmd runs as > > non-root user (and it was modified to move off of sysfs for that > > reason[1]). > > > > It is possible to not rely on sysfs resource files if device is attached to VFIO, but I > think that is a > > must with UIO. > > > > > > I'll continue to push for more information from the testing side to find > > out though. > > > > [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > > > > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > > > > > > Currently, the nfp lock files are taken from the global lock file > > > location, which will work when the user is running as root. However, > > > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) > > > run as a non-root user. > > > > > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > > --- > > > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c > > > index 2ed985ff4..ae2e07220 100644 > > > --- a/drivers/net/nfp/nfp_nfpu.c > > > +++ b/drivers/net/nfp/nfp_nfpu.c > > > @@ -18,6 +18,22 @@ > > > #define NFP_CFG_EXP_BAR 7 > > > > > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > > > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > > > + > > > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > > > +static void > > > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) > > > +{ > > > + const char *dir = "/var/lock"; > > > + const char *home_dir = getenv("HOME"); > > > + > > > + if (getuid() != 0 && home_dir != NULL) > > > + dir = home_dir; > > > + > > > + /* use current prefix as file path */ > > > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > > > + desc->nfp); > > > +} > > > > > > /* There could be other NFP userspace tools using the NSP interface. > > > * Make sure there is no other process using it and locking the access for > > > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > > > struct flock lock; > > > char lockname[30]; > > > > > > - memset(&lock, 0, sizeof(lock)); > > > - > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH */ > > > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > > > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > > > rte_free(desc->nspu); > > > close(desc->lock); > > > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", desc->nfp); > > > - unlink(lockname); > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > return 0; > > > } > > > -- > > > 2.14.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-18 12:32 ` Aaron Conole @ 2018-04-19 6:05 ` Alejandro Lucero 2018-04-20 14:12 ` [dpdk-stable] " Ferruh Yigit 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-19 6:05 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon On Wed, Apr 18, 2018 at 1:32 PM, Aaron Conole <aconole@redhat.com> wrote: > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > On Tue, Apr 17, 2018 at 8:19 PM, Aaron Conole <aconole@redhat.com> > wrote: > > > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > > > I was just wondering, if device device PCI sysfs resource files or > VFIO group /dev files > > require to change > > > permissions for non-root users, does it not make sense to adjust also > /var/lock in the > > system? > > > > For the /dev, we use udev rules - so the correct individual vfio device > > files get assigned the correct permissions. No such mechanism exists > > for /var/lock as far as I can tell. > > > > Ex. see: > > > > https://github.com/openvswitch/ovs/blob/master/ > rhel/usr_lib_udev_rules.d_91-vfio.rules > > > > > > Maybe something similar exists that we could use to generate the lock > > file automatically? > > > > What about /sysfs/bus/pci/device/$PCI_DEV/resource file? > > > > Is RH forcing OVS DPDK to only work if the host has IOMMU support? > > Yes. > Ok then. It makes sense now to apply this patch to stable versions. Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > > > On Tue, Apr 17, 2018 at 4:44 PM, Alejandro Lucero > > <alejandro.lucero@netronome.com> wrote: > > > > > > I have seen that VFIO also requires explicitly to set the right > permissions for non-root > > users to VFIO > > > groups under /dev/vfio. > > > > > > I assume then that running OVS or other DPDK apps as non-root is > possible, > > although requiring > > > those explicit permissions changes, and therefore this patch is > necessary. > > > > > > Adding stable@ and Thomas for discussing how can this be added to > stable DPDK > > versions even if > > > this is not going to be a patch for current DPDK version. > > > > > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > > > > > On Fri, Apr 13, 2018 at 4:31 PM, Alejandro Lucero > > <alejandro.lucero@netronome.com> wrote: > > > > > > On Fri, Apr 13, 2018 at 2:31 PM, Aaron Conole <aconole@redhat.com> > wrote: > > > > > > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > > > > > Again, this patch is correct, but because NFP PMD needs to access > > > > /sys/bus/pci/devices/$DEVICE_PCI_STRING/resource$RESOURCE_ID, and > these files > > have > > > just > > > > read/write accesses for root, I do not know if this is really > necessary. > > > > > > > > Being honest, I have not used a DPDK app with NFP PMD and not > being root. Does > > it > > > work > > > > with non-root users and other PMDs with same requirements > regarding sysfs > > resource > > > files? > > > > > > We do run as non-root user definitely with Intel PMDs. > > > > > > I'm not very sure about other vendors, but I think mlx pmd runs as > > > non-root user (and it was modified to move off of sysfs for that > > > reason[1]). > > > > > > It is possible to not rely on sysfs resource files if device is > attached to VFIO, but I > > think that is a > > > must with UIO. > > > > > > > > > I'll continue to push for more information from the testing side to > find > > > out though. > > > > > > [1]: http://dpdk.org/ml/archives/dev/2018-February/090586.html > > > > > > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> > wrote: > > > > > > > > Currently, the nfp lock files are taken from the global lock file > > > > location, which will work when the user is running as root. > However, > > > > some distributions and applications (notably ovs 2.8+ on > RHEL/Fedora) > > > > run as a non-root user. > > > > > > > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > > > --- > > > > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- > > > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/nfp/nfp_nfpu.c > b/drivers/net/nfp/nfp_nfpu.c > > > > index 2ed985ff4..ae2e07220 100644 > > > > --- a/drivers/net/nfp/nfp_nfpu.c > > > > +++ b/drivers/net/nfp/nfp_nfpu.c > > > > @@ -18,6 +18,22 @@ > > > > #define NFP_CFG_EXP_BAR 7 > > > > > > > > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 > > > > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" > > > > + > > > > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ > > > > +static void > > > > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t > *desc) > > > > +{ > > > > + const char *dir = "/var/lock"; > > > > + const char *home_dir = getenv("HOME"); > > > > + > > > > + if (getuid() != 0 && home_dir != NULL) > > > > + dir = home_dir; > > > > + > > > > + /* use current prefix as file path */ > > > > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, > > > > + desc->nfp); > > > > +} > > > > > > > > /* There could be other NFP userspace tools using the NSP > interface. > > > > * Make sure there is no other process using it and locking the > access for > > > > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) > > > > struct flock lock; > > > > char lockname[30]; > > > > > > > > - memset(&lock, 0, sizeof(lock)); > > > > - > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > > > > > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH > | S_IWOTH */ > > > > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); > > > > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) > > > > rte_free(desc->nspu); > > > > close(desc->lock); > > > > > > > > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", > desc->nfp); > > > > - unlink(lockname); > > > > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); > > > > return 0; > > > > } > > > > -- > > > > 2.14.3 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [dpdk-stable] [RFC 2/2] nfp: allow for non-root user 2018-04-19 6:05 ` Alejandro Lucero @ 2018-04-20 14:12 ` Ferruh Yigit 2018-04-20 14:56 ` Aaron Conole 0 siblings, 1 reply; 22+ messages in thread From: Ferruh Yigit @ 2018-04-20 14:12 UTC (permalink / raw) To: Alejandro Lucero, Aaron Conole Cc: dev, Adrien Mazarguil, stable, Thomas Monjalon On 4/19/2018 7:05 AM, Alejandro Lucero wrote: > On Wed, Apr 18, 2018 at 1:32 PM, Aaron Conole <aconole@redhat.com> wrote: > >> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >> >>> On Tue, Apr 17, 2018 at 8:19 PM, Aaron Conole <aconole@redhat.com> >> wrote: >>> >>> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >>> >>> > I was just wondering, if device device PCI sysfs resource files or >> VFIO group /dev files >>> require to change >>> > permissions for non-root users, does it not make sense to adjust also >> /var/lock in the >>> system? >>> >>> For the /dev, we use udev rules - so the correct individual vfio device >>> files get assigned the correct permissions. No such mechanism exists >>> for /var/lock as far as I can tell. >>> >>> Ex. see: >>> >>> https://github.com/openvswitch/ovs/blob/master/ >> rhel/usr_lib_udev_rules.d_91-vfio.rules >>> >>> >>> Maybe something similar exists that we could use to generate the lock >>> file automatically? >>> >>> What about /sysfs/bus/pci/device/$PCI_DEV/resource file? >>> >>> Is RH forcing OVS DPDK to only work if the host has IOMMU support? >> >> Yes. >> > > Ok then. It makes sense now to apply this patch to stable versions. > > Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> Since the target is the stable tree, I will drop them from patchwork as not applicable. Can you please send v1 of the patch to the stable mail list, it can be good idea to cc stable maintainers as well. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [dpdk-stable] [RFC 2/2] nfp: allow for non-root user 2018-04-20 14:12 ` [dpdk-stable] " Ferruh Yigit @ 2018-04-20 14:56 ` Aaron Conole 0 siblings, 0 replies; 22+ messages in thread From: Aaron Conole @ 2018-04-20 14:56 UTC (permalink / raw) To: Ferruh Yigit Cc: Alejandro Lucero, dev, Adrien Mazarguil, stable, Thomas Monjalon Ferruh Yigit <ferruh.yigit@intel.com> writes: > On 4/19/2018 7:05 AM, Alejandro Lucero wrote: >> On Wed, Apr 18, 2018 at 1:32 PM, Aaron Conole <aconole@redhat.com> wrote: >> >>> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >>> >>>> On Tue, Apr 17, 2018 at 8:19 PM, Aaron Conole <aconole@redhat.com> >>> wrote: >>>> >>>> Alejandro Lucero <alejandro.lucero@netronome.com> writes: >>>> >>>> > I was just wondering, if device device PCI sysfs resource files or >>> VFIO group /dev files >>>> require to change >>>> > permissions for non-root users, does it not make sense to adjust also >>> /var/lock in the >>>> system? >>>> >>>> For the /dev, we use udev rules - so the correct individual vfio device >>>> files get assigned the correct permissions. No such mechanism exists >>>> for /var/lock as far as I can tell. >>>> >>>> Ex. see: >>>> >>>> https://github.com/openvswitch/ovs/blob/master/ >>> rhel/usr_lib_udev_rules.d_91-vfio.rules >>>> >>>> >>>> Maybe something similar exists that we could use to generate the lock >>>> file automatically? >>>> >>>> What about /sysfs/bus/pci/device/$PCI_DEV/resource file? >>>> >>>> Is RH forcing OVS DPDK to only work if the host has IOMMU support? >>> >>> Yes. >>> >> >> Ok then. It makes sense now to apply this patch to stable versions. >> >> Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com> > > Since the target is the stable tree, I will drop them from patchwork as not > applicable. > > Can you please send v1 of the patch to the stable mail list, it can be good idea > to cc stable maintainers as well. Will do. I'll spin into a proper series and submit next week. Sorry for the extra noise, Ferruh. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 15:44 ` Alejandro Lucero 2018-04-17 15:54 ` Alejandro Lucero @ 2018-04-17 15:54 ` Thomas Monjalon 2018-04-17 16:24 ` Alejandro Lucero 1 sibling, 1 reply; 22+ messages in thread From: Thomas Monjalon @ 2018-04-17 15:54 UTC (permalink / raw) To: Alejandro Lucero; +Cc: Aaron Conole, dev, Adrien Mazarguil, stable 17/04/2018 17:44, Alejandro Lucero: > Adding stable@ and Thomas for discussing how can this be added to stable > DPDK versions even if this is not going to be a patch for current DPDK > version. I don't understand. This patch won't enter in 18.05? Why do you think this patch is candidate for stable but not master? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 15:54 ` Thomas Monjalon @ 2018-04-17 16:24 ` Alejandro Lucero 2018-04-17 19:06 ` Thomas Monjalon 0 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-17 16:24 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Aaron Conole, dev, Adrien Mazarguil, stable On Tue, Apr 17, 2018 at 4:54 PM, Thomas Monjalon <thomas@monjalon.net> wrote: > 17/04/2018 17:44, Alejandro Lucero: > > Adding stable@ and Thomas for discussing how can this be added to stable > > DPDK versions even if this is not going to be a patch for current DPDK > > version. > > I don't understand. > This patch won't enter in 18.05? > Why do you think this patch is candidate for stable but not master? > > > Because all that code has been removed between 18.02 and 18.05: http://dpdk.org/ml/archives/dev/2018-March/093655.html I guess this had not happen yet, and stable versions only pull patches from vanilla. Am I right? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 2/2] nfp: allow for non-root user 2018-04-17 16:24 ` Alejandro Lucero @ 2018-04-17 19:06 ` Thomas Monjalon 0 siblings, 0 replies; 22+ messages in thread From: Thomas Monjalon @ 2018-04-17 19:06 UTC (permalink / raw) To: Alejandro Lucero; +Cc: Aaron Conole, dev, Adrien Mazarguil, stable 17/04/2018 18:24, Alejandro Lucero: > On Tue, Apr 17, 2018 at 4:54 PM, Thomas Monjalon <thomas@monjalon.net> > wrote: > > > 17/04/2018 17:44, Alejandro Lucero: > > > Adding stable@ and Thomas for discussing how can this be added to stable > > > DPDK versions even if this is not going to be a patch for current DPDK > > > version. > > > > I don't understand. > > This patch won't enter in 18.05? > > Why do you think this patch is candidate for stable but not master? > > > Because all that code has been removed between 18.02 and 18.05: > > http://dpdk.org/ml/archives/dev/2018-March/093655.html > > I guess this had not happen yet, and stable versions only pull patches from > vanilla. Am I right? Yes patches are cherry-picked from master. But when the backport is too much difficult, a patch can be sent directly to stable@dpdk.org. I don't know whether it already happened to fix a removed code. You need to make sure that the maintainers of stable branches will pick it. There is no special process, it's all a matter of communication :) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 0/2] nfp driver fixes 2018-04-12 22:22 [RFC 0/2] nfp driver fixes Aaron Conole 2018-04-12 22:22 ` [RFC 1/2] nfp: unlink the appropriate lock file Aaron Conole 2018-04-12 22:22 ` [RFC 2/2] nfp: allow for non-root user Aaron Conole @ 2018-04-13 7:26 ` Alejandro Lucero 2018-04-13 13:23 ` Aaron Conole 2 siblings, 1 reply; 22+ messages in thread From: Alejandro Lucero @ 2018-04-13 7:26 UTC (permalink / raw) To: Aaron Conole; +Cc: dev Hi Aaron, Thanks for this patches. I'm afraid these are not applicable for current NFP driver after commit "net/nfp: add NFP CPP support <http://dpdk.org/browse/next/dpdk-next-net/commit/?id=c21e3c53870799c379f8c2ced940e757e8820a5f> " which has been accepted in dpdk-net-next. However, those could be valid for stable versions. I have comments on both patches. On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > Two fixes, one which is fairly obvious (1/2), the other which may > allow support of non-root users. These patches are only compile tested > which is why they are submitted as RFC. After a proper test, will > resubmit them as PATCH (with any suggested / recommended changes). > > Aaron Conole (2): > nfp: unlink the appropriate lock file > nfp: allow for non-root user > > drivers/net/nfp/nfp_nfpu.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > -- > 2.14.3 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 0/2] nfp driver fixes 2018-04-13 7:26 ` [RFC 0/2] nfp driver fixes Alejandro Lucero @ 2018-04-13 13:23 ` Aaron Conole 2018-04-13 15:36 ` Alejandro Lucero 0 siblings, 1 reply; 22+ messages in thread From: Aaron Conole @ 2018-04-13 13:23 UTC (permalink / raw) To: Alejandro Lucero; +Cc: dev, Eelco Chaudron, Pablo Cascon Alejandro Lucero <alejandro.lucero@netronome.com> writes: > Hi Aaron, > > Thanks for this patches. > > I'm afraid these are not applicable for current NFP driver after commit Okay. > "net/nfp: add NFP CPP support" > > which has been accepted in dpdk-net-next. I think nfp_acquire_process_lock() can be modified as I did in 2/2, but I noticed that there's some reliance on various sysfs files (and I think you point this out in your response to 2/2 as well) and that may be problematic for us with ovs2.8+. I'll do some more digging. Thanks Alejandro! > However, those could be valid for stable versions. I have comments on both patches. Cool. As I noted, I haven't tested them yet, but once I get time to test them I will pull in any feedback and resubmit. I guess if I do, they should really just be for stable fixes? Not sure how that would work. > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> wrote: > > Two fixes, one which is fairly obvious (1/2), the other which may > allow support of non-root users. These patches are only compile tested > which is why they are submitted as RFC. After a proper test, will > resubmit them as PATCH (with any suggested / recommended changes). > > Aaron Conole (2): > nfp: unlink the appropriate lock file > nfp: allow for non-root user > > drivers/net/nfp/nfp_nfpu.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > -- > 2.14.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 0/2] nfp driver fixes 2018-04-13 13:23 ` Aaron Conole @ 2018-04-13 15:36 ` Alejandro Lucero 0 siblings, 0 replies; 22+ messages in thread From: Alejandro Lucero @ 2018-04-13 15:36 UTC (permalink / raw) To: Aaron Conole; +Cc: dev, Eelco Chaudron, Pablo Cascon, stable On Fri, Apr 13, 2018 at 2:23 PM, Aaron Conole <aconole@redhat.com> wrote: > Alejandro Lucero <alejandro.lucero@netronome.com> writes: > > > Hi Aaron, > > > > Thanks for this patches. > > > > I'm afraid these are not applicable for current NFP driver after commit > > Okay. > > > "net/nfp: add NFP CPP support" > > > > which has been accepted in dpdk-net-next. > > I think nfp_acquire_process_lock() can be modified as I did in 2/2, but > I noticed that there's some reliance on various sysfs files (and I think > you point this out in your response to 2/2 as well) and that may be > problematic for us with ovs2.8+. I'll do some more digging. > If OVS always relies on VFIO, then it is fine and this patch makes sense. > > Thanks Alejandro! > > > However, those could be valid for stable versions. I have comments on > both patches. > > Cool. As I noted, I haven't tested them yet, but once I get time to > test them I will pull in any feedback and resubmit. I guess if I do, > they should really just be for stable fixes? Not sure how that would > work. > > Not sure how this needs to be done. Stable versions pull from main branch just for bug fixing applied patches. Maybe it is worth to discuss. Adding stable email. > > On Fri, Apr 13, 2018 at 12:22 AM, Aaron Conole <aconole@redhat.com> > wrote: > > > > Two fixes, one which is fairly obvious (1/2), the other which may > > allow support of non-root users. These patches are only compile tested > > which is why they are submitted as RFC. After a proper test, will > > resubmit them as PATCH (with any suggested / recommended changes). > > > > Aaron Conole (2): > > nfp: unlink the appropriate lock file > > nfp: allow for non-root user > > > > drivers/net/nfp/nfp_nfpu.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > -- > > 2.14.3 > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-04-20 14:56 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-12 22:22 [RFC 0/2] nfp driver fixes Aaron Conole 2018-04-12 22:22 ` [RFC 1/2] nfp: unlink the appropriate lock file Aaron Conole 2018-04-13 7:31 ` Alejandro Lucero 2018-04-13 13:24 ` Aaron Conole 2018-04-12 22:22 ` [RFC 2/2] nfp: allow for non-root user Aaron Conole 2018-04-13 7:37 ` Alejandro Lucero 2018-04-13 13:31 ` Aaron Conole 2018-04-13 15:31 ` Alejandro Lucero 2018-04-17 15:44 ` Alejandro Lucero 2018-04-17 15:54 ` Alejandro Lucero 2018-04-17 19:19 ` Aaron Conole 2018-04-18 10:53 ` Alejandro Lucero 2018-04-18 12:32 ` Aaron Conole 2018-04-19 6:05 ` Alejandro Lucero 2018-04-20 14:12 ` [dpdk-stable] " Ferruh Yigit 2018-04-20 14:56 ` Aaron Conole 2018-04-17 15:54 ` Thomas Monjalon 2018-04-17 16:24 ` Alejandro Lucero 2018-04-17 19:06 ` Thomas Monjalon 2018-04-13 7:26 ` [RFC 0/2] nfp driver fixes Alejandro Lucero 2018-04-13 13:23 ` Aaron Conole 2018-04-13 15:36 ` Alejandro Lucero
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.