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

[Alejandro Lucero <alejandro.lucero@netronome.com>]

On Wed, May 9, 2018 at 6:53 PM, Aaron Conole <aconole@redhat.com> wrote:

> Alejandro Lucero <alejandro.lucero@netronome.com> writes:
>
> > I have been thinking about this, and there was something that did not
> seem right to me, although I
> > could not explain what exactly. But this was because we have been
> thinking about VFIO and we have
> > forgotten UIO. The point is, the lock is not required with VFIO but it
> is with UIO, and I'm afraid the
> > way we are trying to solve the non-root user problem is not the right
> one.
>
> Okay, makes sense.
>
> > With VFIO the BARs mapping is done through the kernel VFIO driver, so
> once the device is bound to
> > the driver, and someone tries to use that device, the VFIO driver
> ensures there will not be another
> > user trying to access the device. However, with UIO the driver is not
> doing the BAR mapping but it is
> > the app using the sysfs resource files for that device. It could be, and
> in fact it is easy to happen, two
> > DPDK apps using the same device, because with DPDK apps there is no
> awareness of what other
> > DPDK apps are doing. It is in this case where the lock is required, and
> creating the lock file in the
> > user's home directory is not going to help.
>
> Also makes sense.
>
> > I know RH is just interested in using VFIO, but there are other
> potential NFP PMD users who may
> > want to use UIO instead, and the current solution does not offer the
> right protection in that case.
> > Maybe the lock patch could be created based on the driver the device is
> bound to, leaving /var/lock
> > for UIO and the suggested path for the VFIO case. Or even no lock at all
> for the VFIO case.
>
> Well, yes and no.  For instance, in the uio case, they will be running
> most likely as the root user (because to run as non-root in the uio case
> would cause other problems).  And in that case, $HOME for all
> applications will be root, yes?  I think then, it won't matter.
>
>
I think "most likely" is not enough. If RH wants to run OVS as non-root,
don't you think there could be other people with same necessities? and
using UIO because IOMMU is not available?


> I do agree with no lock for the vfio case would be best.  Is it relevant
> for the newer NFP driver?  I haven't had a look yet.  If so, then I
> think your proposals are fine there.
>
> For the older one, I only know of one patch that needs to go in (which
> fixes the accidentally lost unlink() call).  After that, I wouldn't
> expect more changes in this area (since each one risks stability of the
> code base for a driver that has been replaced).
>
> > On Mon, Apr 30, 2018 at 6:20 PM, 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.
> >
> >  Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> >  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
>