[RFC 0/2] nfp driver fixes

[RFC 0/2] nfp driver fixes

[Aaron Conole]

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

[RFC 1/2] nfp: unlink the appropriate lock file

[Aaron Conole]

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

[RFC 2/2] nfp: allow for non-root user

[Aaron Conole]

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

Re: [RFC 0/2] nfp driver fixes

[Alejandro Lucero]

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

Re: [RFC 1/2] nfp: unlink the appropriate lock file

[Alejandro Lucero]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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

Re: [RFC 0/2] nfp driver fixes

[Aaron Conole]

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

Re: [RFC 1/2] nfp: unlink the appropriate lock file

[Aaron Conole]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Aaron Conole]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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
>

Re: [RFC 0/2] nfp driver fixes

[Alejandro Lucero]

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
>

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Thomas Monjalon]

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?

Re: [RFC 2/2] nfp: allow for non-root user

[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?

Re: [RFC 2/2] nfp: allow for non-root user

[Thomas Monjalon]

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 :)

Re: [RFC 2/2] nfp: allow for non-root user

[Aaron Conole]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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
>

Re: [RFC 2/2] nfp: allow for non-root user

[Aaron Conole]

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

Re: [RFC 2/2] nfp: allow for non-root user

[Alejandro Lucero]

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
>

Re: [dpdk-stable] [RFC 2/2] nfp: allow for non-root user

[Ferruh Yigit]

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.

Re: [dpdk-stable] [RFC 2/2] nfp: allow for non-root user

[Aaron Conole]

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.